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

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

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

Looks reasonable, but R- for the misplaced date code!

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:63
> +    md5.addBytes(reinterpret_cast<const uint8_t*>(&number), sizeof(double));


Yuck.  Why not cryptographicallyRandomValues ?	That will give you nicely
distributed bytes of whatever length you like.

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:65
> +    md5.checksum(md5Bytes);

What's the point of using MD5?	Just generate however many random bytes you
want.

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:87
> +static String currentDate()

This code really doesn't belong in this file.  Doesn't WTF have notions of date
and time?

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:158
> +PassRefPtr<SharedBuffer> MHTMLArchive::generateMHTMLData(const Frame& frame)


We usually pass Frame by pointer, not reference.

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:170
> +    CString asciiTitle = frame.document()->title().ascii();

What if the title contains \r\n ?

> Source/WebCore/loader/archive/mhtml/MHTMLArchive.cpp:176
> +    stringBuilder.append("\ttype=\"text/html\";\r\n");

What if the frame contains something besides text/html, such as image/svg?

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

I see.	ascii() will protect the title.  That seems fragile.

> Source/WebKit/chromium/src/WebPageSerializer.cpp:208
> +    RefPtr<SharedBuffer> mhtml = MHTMLArchive::generateMHTMLData(*frame);

Notice the goofy *frame here.

> Source/WebKit/chromium/src/WebPageSerializer.cpp:210
> +    // FIXME: we are copying all the data here. Idealy we would have a
WebSharedData().
> +    return WebCString(mhtml->data(), mhtml->size());

Ouch.  That seems expensive.