[webkit-reviews] review requested: [Bug 7169] Support exporting of MHTML web archives : [Attachment 95469] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has asked  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 Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=95469&action=review

> Source/JavaScriptCore/wtf/DateMath.cpp:183
> +static String numberTo2CharacterLongString(int number)

I'd suggest twoDigitStringFromNumber().

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

I'd say "make", not "to".

Why are all the arguments int, not unsigned?

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:129
> +    struct timeval timeValue;
> +    struct timezone timeZone;

No need for "struct" in C++.

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:131
> +	   struct tm* tm = gmtime(&(timeValue.tv_sec));

No need for "struct" in C++. Please use a full name for the variable itself.

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:137
> +	     LOG_ERROR("Failed to retrieve time.");
> +    } else
> +	   LOG_ERROR("Failed to retrieve time and time zone.");

There is no early return, so the values should be initialized to avoid having
random data in them.

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:142
> +   
stringBuilder.append(replaceNonPrintableCharacters(frame->document()->title()))
;

Please add a comment here, explaining that IE replaces these with question
marks.

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:161
> +    for (Vector<PageSerializer::Resource>::const_iterator iterator =
resources.begin(); iterator != resources.end(); ++iterator) {

As a matter of coding style, we don't use iterators on vectors. Please just use
an unsigned index.

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:188
> +	       ASSERT(contentEncoding == base64);

Although it's correct in this case, comparing C strings for pointer equality
raises red flags, and is better to avoid.

> Source/WebCore/page/PageSerializer.cpp:212
> +	   // FIXME: it seems iframe used as image trigger this. We should deal
with them correctly.

If these (among) others trigger the assertion, there is no need to say that
they "seem" to.