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

[bugzilla-daemon at webkit.org]

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

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

This patch is in good shape.  Please address Alexey's comments and consider
moving the hex bit mashing into WTF before landing.  I'm happy to take a look
at an updated patch, but you can also use your judgement if you feel like
you've addressed all the comments from this iteration.

> Source/WebCore/platform/text/QuotedPrintable.cpp:96
> +	   if (!isLastCharacter && currentLineLength >
maximumLineLengthToFitCharacter) {

The !isLastCharacter branch confuses me slightly here.	Does that mean we can
exceed the maximumLineLength on the last character of the buffer?  For example,
what if we currently have 76 characters in the line and we have an last
character that requires encoding.  That would seem to push us up to 79
characters.  Maybe that's ok because we're aiming for < 80 ?

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

If these functions don't exist in ASCIIType.h, maybe it would make senes to add
them there?  They seem more general purpose than this file.

>> Source/WebCore/platform/text/QuotedPrintable.cpp:154
>> +	    int upperValue = toASCIIHexValue(upperCharacter);
>> +	    int lowerValue = toASCIIHexValue(lowerCharacter);
>> +	    char decodedCharacter = static_cast<char>(((upperValue << 4) &
0xF0) | (lowerValue & 0x0F));
> 
> I'm not sure if this masking is helpful. toASCIIHexValue returns a sane value
already (although its implementation and return type look strange to me).

Same with this function.