[webkit-reviews] review denied: [Bug 7168] Support reading of MHTML (multipart/related) web archives : [Attachment 93970] 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 7168: Support reading of MHTML (multipart/related) web archives
https://bugs.webkit.org/show_bug.cgi?id=7168

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

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

This looks basically done.  The only major issue is the use of render-tree
tests.	I've also added some minor comments/questions below.  Thanks!

> LayoutTests/platform/chromium-mac/mhtml/multi_frames_ie-expected.txt:23
> +	 RenderPartObject {IFRAME} at (0,172) size 304x154 [border: (2px inset
#000000)]
> +	   layer at (0,0) size 300x150
> +	     RenderView at (0,0) size 300x150
> +	   layer at (0,0) size 300x150
> +	     RenderBlock {HTML} at (0,0) size 300x150
> +	       RenderBody {BODY} at (8,8) size 284x134

We don't need render tree tests for this feature.  We can either use text tests
or dump-as-markup tests.  Using render tree tests is too much of a maintenance
burden.  I feel like I've made this comment every time I've reviewed this
patch.

> Source/WebCore/WebCore.pro:3487
> +	   loader/archive/Archive.cpp \

This seems slightly misplaced, but I guess Qt doesn't build with WebArchive
support.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:-17019
> -				892CF207134C8BB300AAEDA1 /*
JSStorageInfoQuotaCallback.cpp */,
> -				892CF208134C8BB300AAEDA1 /*
JSStorageInfoQuotaCallback.h */,

These changes seem unrelated.  I was seeing them today too.  I'm not sure what
their deal is.

> Source/WebCore/loader/DocumentLoader.cpp:658
> +	   ASSERT(false);

ASSERT_NOT_REACHED().  You should be able to just remove the default case and
rely upon the compiler to warn you if you forget a type.

> Source/WebCore/loader/DocumentLoader.cpp:661
> +    return false;

It's strange that both branches in the Archive::WebArchive lead to returning
false.	Is that your intention?

> Source/WebCore/loader/archive/mhtml/MHTMLParser.cpp:125
> +    if (!MIMETypeRegistry::isSupportedNonImageMIMEType(mimeType) ||
MIMETypeRegistry::isSupportedJavaScriptMIMEType(mimeType) || mimeType ==
"text/css") { // Chromium considers CSS as a document suitable resource.

I'd just remove this comment.  Adding text/css here is a little goofy, but
probably harmless.

> Source/WebCore/platform/network/MIMEHeader.cpp:68
> +	   size_t semiColonIndex = line.find(L':');

No need for L here.

> Source/WebCore/platform/network/MIMEHeader.cpp:69
> +	   if (semiColonIndex == notFound || (semiColonIndex == line.length() -
1)) {

Why isn't

Foo-bar:

A key-value pair?  It just seems like the value is the empty string...

> Source/WebCore/platform/network/MIMEHeader.cpp:78
> +	   keyValuePairs.set(key, value.toString().stripWhiteSpace());

Yeah, especially that's then whitespace sensitive.  If you've got a trailing
space, then you end up with a key-value pair with an empty value.

> Source/WebCore/platform/network/MIMEHeader.cpp:88
> +	   ContentTypeParser contentTypeParser(makeString("Content-Type:",
mimeParametersIterator->second));

It's strange that you need to synthesize a fake key name here.

> Source/WebCore/platform/network/MIMEHeader.cpp:107
> +	   mimeHeader->m_contentTransferEncoding =
parseContentTransferEncoding(mimeParametersIterator->second);

This one works more nicely.

> Source/WebCore/platform/network/MIMEHeader.h:38
> +// FIXME: This class is a limited MIME parser used to parse the MIME headers
of MHTML files.

Super minor nit: I'd more this comment to right above the "class MIMEHeader"
line.