[webkit-reviews] review granted: [Bug 7169] Support exporting of MHTML web archives : [Attachment 95469] 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 7169: Support exporting of MHTML web archives
https://bugs.webkit.org/show_bug.cgi?id=7169

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

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

This looks great.  A bunch of minor nits below.

> Source/JavaScriptCore/wtf/DateMath.cpp:1041
> +String toRFC2822DateString(int dayOfWeek, int day, int month, int year, int
hours, int minutes, int seconds, int utcOffset)

Much nicer!  Thanks.

> Source/JavaScriptCore/wtf/DateMath.h:65
> +String toRFC2822DateString(int dayOfWeek, int day, int month, int year, int
hours, int minutes, int seconds, int utcOffset);

Is it worth saying which of these are zero-based and which are one-based?  I
remember that being fairly confusing in POSIX.

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:59
> +    char randomValues[10];
> +    cryptographicallyRandomValues(&randomValues, 10);

I'd make the 10 a named constant (local to this function).

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:122
> +    pageSerializer.serialize(frame->page());

Should MHTMLArchive::generateMHTMLData take a Page rather than a Frame?

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:133
> +	     dateString = toRFC2822DateString(tm->tm_wday, tm->tm_mday,
tm->tm_mon, 1900 + tm->tm_year, tm->tm_hour, tm->tm_min, tm->tm_sec,
timeZone.tz_minuteswest);

bad indent

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:135
> +	     LOG_ERROR("Failed to retrieve time.");

In what situations could these fail?  Should these have ASSERT_NOT_REACHED?

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:156
> +    // We use utf8() below instead of ascii() as ascii() replaces CRLFs with
?? (we still only have put ASCII characters in it).

Should we assert that asciiString is all ASCII?

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:169
> +	   const char* contentEncoding = resource.mimeType.startsWith("text/")
? quotedPrintable : base64;

So application/xml will get base64, but text/html will get quotedPrintable. 
Maybe that's ok...  There's probably a helper function somewhere that will tell
you if a MIME type is XML.

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:194
> +		   size_t lineLength = std::min(encodedDataLength - index,
static_cast<size_t>(76));

I'd store 76 in a named constant (which will also let you avoid the
static_cast).

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:197
> +		   index += 76;

... because 76 recurs here.

> Source/WebKit/chromium/src/WebPageSerializer.cpp:208
> +    WebFrame* webFrame = static_cast<WebViewImpl*>(view)->mainFrame();
> +    Frame* frame = static_cast<WebFrameImpl*>(webFrame)->frame();
> +    RefPtr<SharedBuffer> mhtml = MHTMLArchive::generateMHTMLData(frame);

Yeah, see you're using mainFrame here.	That's a sign that you should really be
dealing with Pages.