Bug 63310 – MHTML archive loader should support binary parts

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jay Civelli]

The MHTML archive loader only supports the following encodings:
QuotedPrintable, Base64 and and 7bit.
It should also support binary for writing/reading archives, as it provides more compact files that are faster to load.

[Jay Civelli]

Created attachment 98629 [details]
Patch

[Adam Barth]

Comment on attachment 98629 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98629&action=review

> Source/WebCore/loader/archive/mhtml/MHTMLParser.cpp:168
> +         endOfArchiveReached = (nextChars[0] == '-' && nextChars[1] == '-');

Do you want to ASSERT(nextChars.size() == 2) before this line (to help the casual reader)?

> Source/WebCore/loader/archive/mhtml/MHTMLParser.cpp:180
> +            if (checkBoundary && (line == endOfPartBoundary || line == endOfDocumentBoundary)) {
> +                endOfArchiveReached = (line == endOfDocumentBoundary);

nit: Can we avoid doing this strcmp twice?

> Source/WebCore/platform/SharedBuffer.cpp:171
> +    if (!data.isEmpty())
> +        append(data.data(), data.size());

It looks like append(const char*, unsigned) might already understand length == 0, so this branch might not be needed.

> Source/WebKit/chromium/public/WebPageSerializer.h:64
>      // Serializes the WebView contents to a MHTML representation.
> -    WEBKIT_API static WebCString serializeToMHTML(WebView*);
> +    // Passing true for |useBinaryEncoding| will result in a smaller HTML file,
> +    // but it might not work on other browsers.
> +    WEBKIT_API static WebCString serializeToMHTML(WebView*, bool useBinaryEncoding = false);

Should this be an enum for the various supported encodings (even if we just support two today)?

[Adam Barth]

+fishd for WebKit API review.

[Jay Civelli]

Comment on attachment 98629 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98629&action=review

>> Source/WebCore/loader/archive/mhtml/MHTMLParser.cpp:168
>> +         endOfArchiveReached = (nextChars[0] == '-' && nextChars[1] == '-');
> 
> Do you want to ASSERT(nextChars.size() == 2) before this line (to help the casual reader)?

Done.

>> Source/WebCore/loader/archive/mhtml/MHTMLParser.cpp:180
>> +                endOfArchiveReached = (line == endOfDocumentBoundary);
> 
> nit: Can we avoid doing this strcmp twice?

Good idea, done.

>> Source/WebCore/platform/SharedBuffer.cpp:171
>> +        append(data.data(), data.size());
> 
> It looks like append(const char*, unsigned) might already understand length == 0, so this branch might not be needed.

Removed if.

>> Source/WebKit/chromium/public/WebPageSerializer.h:64
>> +    WEBKIT_API static WebCString serializeToMHTML(WebView*, bool useBinaryEncoding = false);
> 
> Should this be an enum for the various supported encodings (even if we just support two today)?

I don't expect any other encodings. We could change to an enum in the future in the unlikely event that we need an extra encoding.

[Jay Civelli]

Created attachment 98660 [details]
Patch

[Adam Barth]

Comment on attachment 98660 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98660&action=review

> Source/WebKit/chromium/public/WebPageSerializer.h:64
>      // Serializes the WebView contents to a MHTML representation.
> -    WEBKIT_API static WebCString serializeToMHTML(WebView*);
> +    // Passing true for |useBinaryEncoding| will result in a smaller HTML file,
> +    // but it might not work on other browsers.
> +    WEBKIT_API static WebCString serializeToMHTML(WebView*, bool useBinaryEncoding = false);

Ok.  I'm going to let fishd do the final review.  (Changing the WebKit API is a PITA, which is why I suggested an enum.)

[Darin Fisher (:fishd, Google)]

Comment on attachment 98660 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98660&action=review

>> Source/WebKit/chromium/public/WebPageSerializer.h:64
>> +    WEBKIT_API static WebCString serializeToMHTML(WebView*, bool useBinaryEncoding = false);
> 
> Ok.  I'm going to let fishd do the final review.  (Changing the WebKit API is a PITA, which is why I suggested an enum.)

Ordinarily, raw bool parameters like this don't help much with readability at the
callsite.  It would make the callsites more self-documenting if you passed an
enum value, or if you created a separate function:  serializeToMHTMLUsingBinaryEncoding.

[Jay Civelli]

Created attachment 98766 [details]
Patch

[Jay Civelli]

Comment on attachment 98660 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98660&action=review

>>> Source/WebKit/chromium/public/WebPageSerializer.h:64
>>> +    WEBKIT_API static WebCString serializeToMHTML(WebView*, bool useBinaryEncoding = false);
>> 
>> Ok.  I'm going to let fishd do the final review.  (Changing the WebKit API is a PITA, which is why I suggested an enum.)
> 
> Ordinarily, raw bool parameters like this don't help much with readability at the
> callsite.  It would make the callsites more self-documenting if you passed an
> enum value, or if you created a separate function:  serializeToMHTMLUsingBinaryEncoding.

OK, I created a separate function as I don't expect any other encoding in the future.

[Darin Fisher (:fishd, Google)]

Comment on attachment 98766 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98766&action=review

> Source/WebKit/chromium/src/WebPageSerializer.cpp:211
> +WebCString WebPageSerializer::serializeToMHTMLUsingBinaryEncoding(WebView* view)

why don't you make this method return WebData?  (that addresses the FIXME)

[Jay Civelli]

Comment on attachment 98766 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98766&action=review

>> Source/WebKit/chromium/src/WebPageSerializer.cpp:211
>> +WebCString WebPageSerializer::serializeToMHTMLUsingBinaryEncoding(WebView* view)
> 
> why don't you make this method return WebData?  (that addresses the FIXME)

I have that in another patch, that got r+ed, landed and broke a test and got rolled back.
In the meantime I landed a patch in Chromium that uses the API.
So my plan is to go the "#ifdef   bug_1234_fixed" road to address that in an upcoming patch.

[Darin Fisher (:fishd, Google)]

Comment on attachment 98766 [details]
Patch

OK, LGTM for WebKit API change.

[Darin Fisher (:fishd, Google)]

Comment on attachment 98766 [details]
Patch

Based on Adam's previous comments that this was just waiting for me to give final review, I'm marking this R=me.  I did not review the WebCore portions of this patch carefully, but I defer to Adam for that part.

[Adam Barth]

I'm happy to take responsibility for the WebCore parts.

[WebKit Review Bot]

Comment on attachment 98766 [details]
Patch

Clearing flags on attachment: 98766

Committed r89869: <http://trac.webkit.org/changeset/89869>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.