[webkit-reviews] review denied: [Bug 59834] WebCore should have QuotedPrintable encoding capabilities : [Attachment 91931] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Jay Civelli
<jcivelli at chromium.org>'s request for review:
Bug 59834: WebCore should have QuotedPrintable encoding capabilities
https://bugs.webkit.org/show_bug.cgi?id=59834

Attachment 91931: Patch
https://bugs.webkit.org/attachment.cgi?id=91931&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=91931&action=review

As Adam mentioned in bug 7168, this should be discussed on webkit-dev first. It
is not obvious to me if there is any value in supporting MHTML now.

> Source/WebCore/platform/text/QuotedPrintable.cpp:34
> +#include <wtf/text/CString.h>

CString isn't used in this file, as far as I can see.

> Source/WebCore/platform/text/QuotedPrintable.cpp:39
> +static const int maxCharPerLine = 76;

Plz don't abbr.

> Source/WebCore/platform/text/QuotedPrintable.cpp:45
> +static int hexDigitValue(char c)

Why not use the existing toASCIIHexValue()? I see that you want a check instead
of assertion, but it's still not good to add copy/pasted code, and returning a
failure in a magic value is not right.

Also, this should be marked inline.

> Source/WebCore/platform/text/QuotedPrintable.cpp:55
> +// Returns the length in characters of the EOL found at index, 0 if there
isn't one.
> +static int isEOL(const char* input, unsigned index, unsigned length)

This function name is obviously wrong, it doesn't even return a boolean like an
"is" function should.

> Source/WebCore/platform/text/QuotedPrintable.cpp:79
> +String quotedPrintableEncode(const char* input, unsigned length)

I can see how it is necessary to decode this atrocity to support reading MHTML
files created by other browsers, but can we avoid encoding like that? Are any
other encoding forms supported in MHTML?

There are many lengths in this function, so I think that the argument needs a
better name.

The types seem wrong. Ultimately, the source in Unicode, and encoding output is
ASCII, but types are exact opposite of that (see the question about decoding
routine though).

> Source/WebCore/platform/text/QuotedPrintable.cpp:82
> +    // The number of characters in the current line.Ve

Garbage at the end.

> Source/WebCore/platform/text/QuotedPrintable.cpp:83
> +    int charCount = 0;

As a count, this should be unsigned.

> Source/WebCore/platform/text/QuotedPrintable.cpp:88
> +	   // Whether this character can be inserted without encoding.
> +	   bool asIs = false;

You could say the same in the variable name.

> Source/WebCore/platform/text/QuotedPrintable.cpp:96
> +	   if (!asIs && (c == '\t' || c == ' ') && !lastChar && !isEOL(input, i
+ 1, length))
> +	       asIs = true;

Is it really that only ASCII characters are encoded as is? It seems like a
practical problem that non-ASCII text would be exploded in size.

> Source/WebCore/platform/text/QuotedPrintable.cpp:102
> +	       if (eolLen > 0) {
> +	       result.append(eol);

Bad indentation.

> Source/WebCore/platform/text/QuotedPrintable.cpp:114
> +	       result.append('=');
> +	   result.append(eol);
> +	       charCount = 0;

Bad indentation.

> Source/WebCore/platform/text/QuotedPrintable.cpp:141
> +bool quotedPrintableDecode(const char* data, unsigned len, Vector<char>&
out)

Please don't abbreviate; again, there are many different lengths here, and a
better name is needed for the argument.

How does MHTML encode non-ASCII characters - is it actually a post-processing
step after creating a byte stream?

> Source/WebCore/platform/text/QuotedPrintable.cpp:148
> +    for (unsigned i = 0; i < len; i++) {

Style nit: we prefer prefix increment and decrement when it doesn't matter.

> Source/WebCore/platform/text/QuotedPrintable.cpp:158
> +	       // Unfinished = sequence, append as is.
> +	       success = false;

Are we really interested in a failure like this? Is there anything the caller
can reasonably do, other than ignore the returned boolean value?

> Source/WebCore/platform/text/QuotedPrintable.cpp:173
> +	       success = false;

Ditto.

> Source/WebCore/platform/text/QuotedPrintable.cpp:181
> +	   char r = static_cast<char>(((i1 << 4) & 0xF0) | (i2 & 0x0F));

Please don't abbreviate.