[webkit-reviews] review granted: [Bug 148363] Implement Subresource Integrity (SRI) : [Attachment 309508] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Sam Weinig <sam at webkit.org>'s
request for review:
Bug 148363: Implement Subresource Integrity (SRI)
https://bugs.webkit.org/show_bug.cgi?id=148363

Attachment 309508: Patch

https://bugs.webkit.org/attachment.cgi?id=309508&action=review




--- Comment #72 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 309508
  --> https://bugs.webkit.org/attachment.cgi?id=309508
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=309508&action=review

> Source/WebCore/loader/DocumentThreadableLoader.cpp:381
> +	   if (options().filteringPolicy == ResponseFilteringPolicy::Disable) {

I am not sure we need this case now but it seems fine to have it.
We should really make the default filteringPolicy Enable and not Disable.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:388
> +		   if (tainting == ResourceResponse::Tainting::Opaque) {

Can this case actually happen?
If we have integrity, we should be using cors or same-origin and never end up
with Tainting::Opaque.
Can we place an ASSERT here at least?

> Source/WebCore/loader/DocumentThreadableLoader.cpp:398
> +		   m_client->didReceiveResponse(identifier, response);

Probably not needed here as well either.
ASSERT(response.type() == ResourceResponse::Type::Default) should be good
enough.

> LayoutTests/ChangeLog:10
> +	   Platform Tests. Additional tests for more CORS combinations have
been added.

Can you file a bug about removing these tests and updating WPT ones?
Maybe adding a FIXME in the tests as well?