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

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

This is getting close.	A few minor points (and some questions) below.	Thanks!


> Source/WebCore/CMakeLists.txt:1950
> +	   loader/archive/mhtml/MIMEHeader.cpp

I thought this got moved to platform/network...

> Source/WebCore/GNUmakefile.list.am:2008
> +	Source/WebCore/loader/archive/mhtml/MIMEHeader.cpp \
> +	Source/WebCore/loader/archive/mhtml/MIMEHeader.h \

Same here

> Source/WebCore/WebCore.gypi:3352
> +	       'loader/archive/mhtml/MHTMLArchive.h',

The headers only need to appear once (in the first group of files).

> Source/WebCore/loader/MainResourceLoader.cpp:261
> +	   bool isRemoteWebArchive =
(equalIgnoringCase("application/x-webarchive", mimeType) ||
equalIgnoringCase("message/rfc822", mimeType))
> +	       && !m_substituteData.isValid() && !url.isLocalFile();

does message/rfc822 need to be behind ENABLE(MHTML) ?

> Source/WebCore/loader/archive/Archive.h:47
> +    virtual ~Archive() { }

Virtual functions should be out-of-line.

> Source/WebCore/loader/archive/ArchiveResourceCollection.cpp:62
> +	       m_subframes.set((*iFrame)->mainResource()->url().string(),
iFrame->get());

Is the variable really called iFrame ?	*cries*

> Source/WebCore/loader/archive/mhtml/MHTMLParser.cpp:121
> +    if (mimeType != "text/html" && mimeType != "application/xhtml+xml" &&
mimeType != "image/svg+xml")  {

This line doesn't feel right.  Is there some function in WebCore we should be
calling that knows about the set of MIME types you're trying to select here?

> Source/WebCore/loader/archive/mhtml/MHTMLParser.cpp:140
> +    ASSERT(endOfPartBoundary.isEmpty() == endOfDocumentBoundary.isEmpty());

I meant that we should add ASSERT(!endOfArchiveReached) here, right?

> Source/WebCore/loader/archive/mhtml/MHTMLParser.cpp:183
> +    return ArchiveResource::create(contentBuffer, KURL(ParsedURLString,
mimeHeader.contentLocation()),

KURL(ParsedURLString, mimeHeader.contentLocation()) doesn't seem quite right
either.  Is contentLocation always an absolute URL?

> Source/WebCore/loader/archive/mhtml/MIMEHeader.h:46
> +class MIMEHeader : public RefCounted<MIMEHeader> {

Am I confused?	I thought we did this class already...