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

[bugzilla-daemon at webkit.org]

Adam Barth <abarth 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 92138: Patch
https://bugs.webkit.org/attachment.cgi?id=92138&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=92138&action=review

This looks good.  I just have an epic number of naming nits.  For some reason
WebKit is supper excited about name variables and functions with names that
explain their meaning.	The general thought process is that names are more
valuable than comments.  If you find yourself writing a comment to explain what
a variable or function means, then you should try deleting that comment and
changing the name to contain the information you should have put in the
comment.

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

lineMaximumSize => maximumLineLength

> Source/WebCore/platform/text/QuotedPrintable.cpp:43
> +static const char eol[] = "\r\n";

crlfLineEnding ?

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

I'd remove the comment and change the name to something that explains what the
function does:

lengthOfLineEndingAtIndex

Also, unsigned should probably be size_t, right?

> Source/WebCore/platform/text/QuotedPrintable.cpp:47
> +{

Can you add an assert that index < length ?

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

unsigned => size_t (that's what in.size() returns, right?)

> Source/WebCore/platform/text/QuotedPrintable.cpp:69
> +    // The number of characters in the current line.
> +    unsigned charCount = 0;

I'd remove the comment and rename charCount -> currentLineLength

> Source/WebCore/platform/text/QuotedPrintable.cpp:71
> +	   bool lastChar = (i == length - 1);

lastChar => isLastCharacter.  (lastChar would be the last character itself)

> Source/WebCore/platform/text/QuotedPrintable.cpp:84
> +	       int eolLen = eolLengthAtIndex(input, i, length);

eolLen	=> lengthOfLineEnding

> Source/WebCore/platform/text/QuotedPrintable.cpp:94
> +	   unsigned minCharsNeeded = requiresEncoding ? lineMaximumSize - 4 :
lineMaximumSize - 2;

It would be helpful to name these constants.  Also minCharsNeeded should be
renamed to something that doesn't use abbreviations.  I'm not really sure what
it does, so it's hard for me to recommend a name.

> Source/WebCore/platform/text/QuotedPrintable.cpp:105
> +	       result.append(hexTable[static_cast<int>((c >> 4) & 0xF)]);
> +	       result.append(hexTable[static_cast<int>(c & 0x0F)]);

I think we already have code that does this bit munging.  Maybe toASCIIHexValue
in ASCIIType.h?

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

Shouldn't this take a string as input?	It seems odd that it's not the opposite
of encode.

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

unsigned => size_t

> Source/WebCore/platform/text/QuotedPrintable.cpp:127
> +	   char character1 = data[i];

character1 => currentCharacter

> Source/WebCore/platform/text/QuotedPrintable.cpp:129
> +	       // Regular character, append as is.

No need for this comment.

> Source/WebCore/platform/text/QuotedPrintable.cpp:140
> +	   char character2 = data[++i];
> +	   char character3 = data[++i];

character2 => upperCharacter
character3 => lowerCharacter

Those are sort of lame name suggestions, but something with words is better
than using numerals.

> Source/WebCore/platform/text/QuotedPrintable.cpp:142
> +	       // Soft line break, ignored.

I'd remove this comment as well.

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

We must have code that does this assembly already, in which case you can call
it without creating these local variables with tricky names.