[webkit-reviews] review granted: [Bug 67983] CRASH under WebCore::ArchiveResourceCollection::addAllResources loading WebArchive : [Attachment 107138] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 67983: CRASH under WebCore::ArchiveResourceCollection::addAllResources
loading WebArchive
https://bugs.webkit.org/show_bug.cgi?id=67983

Attachment 107138: [PATCH] Proposed Fix
https://bugs.webkit.org/attachment.cgi?id=107138&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=107138&action=review


r=me but I think the test needs to be put in a different directory or somehow
otherwise skipped for non-webarchive platforms

> LayoutTests/ChangeLog:11
> +	   * loader/test-loading-archive-subresource-null-mimetype.html: Added.


This test needs to be skipped on the many platforms that do not support
CoreFoundation property list web archives. You should look for other similar
tests and see how that works. I’m guessing that the tests normally go inside
webarchive/loading rather than in loader to make sure they get skipped on
platforms that do not have web archives.

> Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:331
> +	   LOG(Archives, "LegacyWebArchive - Main Resource MIME Type is
required, but was null.");

The words Resource and Type should not be capitalized here.

> Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:351
> +	       RefPtr<ArchiveResource> subresource =
createResource(subresourceDict);
> +	       if (subresource)
> +		   addSubresource(subresource.release());

It would be nicer to define this variable inside the if.