Collapse images disallowed by the Safe Browsing Subresource Filter.

Collapse images disallowed by the Safe Browsing Subresource Filter.

When the ImageResource being loaded into an HTMLImageElement is disallowed by
subresource filtering, the element is now collapsed in the layout (instead of
showing fallback content).

This is achieved by the following changes:

 1) Internally, access check related methods in FetchContext, ResourceLoader
    and ResourceFetcher have been refactored so that they now return a
    ResourceRequestBlockedReason enumerator instead of a boolean value.

 2) ResourceError now has an |m_shouldCollapseInitiator| field. When a load is
    disallowed because of subresource filtering, the ResourceError instance set
    on the failed Resource will now has this boolean set.

 3) HTMLImageElement now has three states: showing primary content, showing
    fallback content, and collapsed. When the |m_shouldCollapseInitiator| is
    set on its ImageResource, it enters this new, third state.

BUG=609747
Committed: https://crrev.com/3c67d0c8a79c59b3afa1d73761387fb3be82a0bd
Cr-Commit-Position: refs/heads/master@{#436917}

[engedy, 2016-11-30 15:09:21]

Description was changed from

==========
Introduce collapsed state for image elements.

BUG=None
==========

to

==========
Collapse images disallowed by the Safe Browsing Subresource Filter.

When the ImageResource being loaded into an HTMLImageElement is disallowed by 
subresource filtering, the element is now collapsed in the layout (instead of
showing fallback content).

This is achieved by the following changes:
 1) Internally, access check related methods in FetchContext, ResourceLoader
    and ResourceFetcher have been refactored so that they now return a
    ResourceRequestBlockedReason enumerator instead of a boolean value.
 2) ResourceError now has an |m_shouldCollapseInitiator| field. When a load is
    disallowed because of subresource filtering, the ResourceError instance set
    on the failed Resource will now has this boolean set.
 3) HTMLImageElement now has three states: showing primary content, showing
    fallback content, and collapsed. When the |m_shouldCollapseInitiator| is
    set on its ImageResource, it enters this new, third state.

BUG=609747
==========

[engedy, 2016-11-30 15:09:21]

engedy@chromium.org changed reviewers:
+ japhet@chromium.org, mkwst@chromium.org

[engedy, 2016-11-30 15:09:39]

Description was changed from

==========
Collapse images disallowed by the Safe Browsing Subresource Filter.

When the ImageResource being loaded into an HTMLImageElement is disallowed by 
subresource filtering, the element is now collapsed in the layout (instead of
showing fallback content).

This is achieved by the following changes:
 1) Internally, access check related methods in FetchContext, ResourceLoader
    and ResourceFetcher have been refactored so that they now return a
    ResourceRequestBlockedReason enumerator instead of a boolean value.
 2) ResourceError now has an |m_shouldCollapseInitiator| field. When a load is
    disallowed because of subresource filtering, the ResourceError instance set
    on the failed Resource will now has this boolean set.
 3) HTMLImageElement now has three states: showing primary content, showing
    fallback content, and collapsed. When the |m_shouldCollapseInitiator| is
    set on its ImageResource, it enters this new, third state.

BUG=609747
==========

to

==========
Collapse images disallowed by the Safe Browsing Subresource Filter.

When the ImageResource being loaded into an HTMLImageElement is disallowed by 
subresource filtering, the element is now collapsed in the layout (instead of
showing fallback content).

This is achieved by the following changes:

 1) Internally, access check related methods in FetchContext, ResourceLoader
    and ResourceFetcher have been refactored so that they now return a
    ResourceRequestBlockedReason enumerator instead of a boolean value.

 2) ResourceError now has an |m_shouldCollapseInitiator| field. When a load is
    disallowed because of subresource filtering, the ResourceError instance set
    on the failed Resource will now has this boolean set.

 3) HTMLImageElement now has three states: showing primary content, showing
    fallback content, and collapsed. When the |m_shouldCollapseInitiator| is
    set on its ImageResource, it enters this new, third state.

BUG=609747
==========

[engedy, 2016-11-30 15:10:14]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 15:10:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[engedy, 2016-11-30 15:11:05]

Patchset #1 (id:1) has been deleted

[engedy, 2016-11-30 15:15:06]

Hey @Mike, @Nate, could you see the CL description, and please take an initial
look?

I more-or-less followed what you suggested some time ago, still, please do let
me know if you can think of a better approach today, and also any edge cases I
may have missed.

[Mike West, 2016-11-30 15:57:35]

I like this. And Nate is OOO, so let's land it and see what explodes.

LGTM % nits.

https://codereview.chromium.org/2535383003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/2535383003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:91:
ResourceRequestBlockedReason::None) {
Nit: this is pretty ugly, but I'm not sure I have a better solution.

https://codereview.chromium.org/2535383003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/2535383003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:857: m_contentType ==
ContentType::Collapsed)
Nit: {} for multi-line conditionals.

https://codereview.chromium.org/2535383003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLImageElement.h (right):

https://codereview.chromium.org/2535383003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLImageElement.h:201: ContentType
m_contentType;
Nit: "LayoutDisposition", perhaps? `ContentType` means too many things already
(`Content-Type` HTTP header, for instance).

[commit-bot: I haz the power, 2016-11-30 16:30:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 16:30:43]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[engedy, 2016-11-30 16:41:42]

engedy@chromium.org changed reviewers:
+ dominicc@chromium.org

[engedy, 2016-11-30 16:41:43]

@Dominic, would you be the right person to review changes to:
  third_party/WebKit/Source/core/html/?

[engedy, 2016-11-30 17:35:59]

Patchset #2 (id:40001) has been deleted

[engedy, 2016-11-30 17:37:17]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[engedy, 2016-11-30 17:37:39]

https://codereview.chromium.org/2535383003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/2535383003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:91:
ResourceRequestBlockedReason::None) {
On 2016/11/30 15:57:35, Mike West (slow) wrote:
> Nit: this is pretty ugly, but I'm not sure I have a better solution.

Introduced a helper variable, not sure it helped much.

https://codereview.chromium.org/2535383003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/2535383003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:857: m_contentType ==
ContentType::Collapsed)
On 2016/11/30 15:57:35, Mike West (slow) wrote:
> Nit: {} for multi-line conditionals.

Done.

https://codereview.chromium.org/2535383003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLImageElement.h (right):

https://codereview.chromium.org/2535383003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLImageElement.h:201: ContentType
m_contentType;
On 2016/11/30 15:57:35, Mike West (slow) wrote:
> Nit: "LayoutDisposition", perhaps? `ContentType` means too many things already
> (`Content-Type` HTTP header, for instance).

Good point. LayoutDisposition sounds good, done.

[commit-bot: I haz the power, 2016-11-30 17:37:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 19:02:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 19:02:49]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[engedy, 2016-12-01 15:27:00]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 15:27:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 17:33:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 17:33:37]

Dry run: This issue passed the CQ dry run.

[engedy, 2016-12-01 18:29:21]

Patchset #3 (id:80001) has been deleted

[engedy, 2016-12-01 22:12:59]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 22:13:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[engedy, 2016-12-01 22:20:15]

Okay, infinitely nested fallback content fixed. I added regression test for good
measure, although I'm not sure if `fast` is the right directory. In any case,
everything's looking green now.

@Dominic, could you please take a look to make sure no other edge cases are
being overlooked?

[commit-bot: I haz the power, 2016-12-02 00:21:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 00:21:09]

Dry run: This issue passed the CQ dry run.

[dominicc (has gone to gerrit), 2016-12-05 03:30:56]

Sorry for the slow response, some comments inline.

Mounir, is media team interested in reviewing changes to image like this?

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/dom/HTMLImageElement/fallback-image-moved-across-documents-expected.html
(right):

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/HTMLImageElement/fallback-image-moved-across-documents-expected.html:3:
<img src="non-existent">
I'm curious why and how the '.png' goes missing.

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/dom/HTMLImageElement/fallback-image-moved-across-documents.html
(right):

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/HTMLImageElement/fallback-image-moved-across-documents.html:1:
<!DOCTYPE html>
Let's use testharness.js for new layout tests!

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/HTMLImageElement/fallback-image-moved-across-documents.html:7:
// Fallback content is implemented by means of shadow DOM, with a dumbed-down
Nice comment explaining the intent of the test and how it relates to the
implementation.

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/HTMLImageElement/fallback-image-moved-across-documents.html:12:
// image icon inside, fail, and end up with infinitely nested fallback content.
I'm curious--how would loading the broken image icon fail?

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/HTMLImageElement/fallback-image-moved-across-documents.html:20:
document.body.appendChild(nestedDocument.body.removeChild(i));
Removes happen implicitly as part of apppend, appendChild, etc., so maybe you
could simplify this by just doing document.body.append(i).

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/subresource_filter/image-allow-disallow-transitions.html
(right):

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/subresource_filter/image-allow-disallow-transitions.html:42:
for (let initialState of Object.keys(INITIAL_URLS)) {
Maybe

for (let [initialState, initialUrl] of Object.entries(INITIAL_URLS)) {

is more succinct since you don't have to mention INITIAL_URLS again below; same
for FINAL_URLS.

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/subresource_filter/image-allow-disallow-transitions.html:52:
if (window.testRunner && --numPendingImages == 0)
If you use testharness you could consider using the event->promise mapping thing
they have, promise_test, and Promise.all to join them instead of managing an
explicit counter this way.

On the other hand, this is pretty readable, so up to you! Might be interesting
to see if it is a lot simpler one way or the other.

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/subresource_filter/image-disallowed.html
(right):

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/subresource_filter/image-disallowed.html:17:
assert_equals(i.clientWidth, 100, "Images that are not disallowed are displayed
at their natural width.");
Consider 'should be' instead of 'are'. When an assertion fails and you see text
like

Images that are not disallowed are displayed at their natural width.

there's a grain of doubt whether the 'are' is describing what did happen, or
what was desired to happen. Whereas

Images that are not disallowed should be displayed at their natural width.

removes that doubt.

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/subresource_filter/image-disallowed.html:35:
document.body.appendChild(i);
Consider using 'append', which is a wee bit shorter and hence more readable.

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (left):

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:352: if
(m_useFallbackContent)
What do you think about using a switch here? Then it's crystal clear you cover
three cases:

Fallback -> block flow
Primary -> image
Collapsed -> dcheck

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:832:
m_layoutDisposition = LayoutDisposition::Collapsed;
This code smells a tiny bit, it looks like when you set m_layoutDisposition you
should call reattachContent right after/soon after. Can we make that a setter
which ties those operations together into unit?

If that's not *always* the case, can we add some unit tests and integration
tests exercising when it's different?

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:836:
setUseFallbackContent();
Isn't this just ensureFallbackForGeneratedContent?

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:858: if
(m_layoutDisposition == LayoutDisposition::PrimaryContent ||
Again, I wonder if a switch with fallthrough would be easier to read? It teaches
the reader what the three exhaustive cases are even if they don't have the
definition of LayoutDisposition at the front of their mind.

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLImageElement.h (right):

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.h:145: PrimaryContent,
It would be splendid to have some comments explaining what these are! Maybe a
link to a relevant spec on the enum itself would be swell.

[engedy, 2016-12-06 15:25:51]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 15:26:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[engedy, 2016-12-06 15:27:57]

All done, please take another look.

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/dom/HTMLImageElement/fallback-image-moved-across-documents-expected.html
(right):

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/HTMLImageElement/fallback-image-moved-across-documents-expected.html:3:
<img src="non-existent">
On 2016/12/05 at 03:30:55, dominicc wrote:
> I'm curious why and how the '.png' goes missing.

I have made the URL consistent across tests. Let me know if you'd prefer some
PHP/Perl to explicitly return 404 instead.

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/dom/HTMLImageElement/fallback-image-moved-across-documents.html
(right):

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/HTMLImageElement/fallback-image-moved-across-documents.html:1:
<!DOCTYPE html>
On 2016/12/05 03:30:55, dominicc wrote:
> Let's use testharness.js for new layout tests!

Done.

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/HTMLImageElement/fallback-image-moved-across-documents.html:7:
// Fallback content is implemented by means of shadow DOM, with a dumbed-down
On 2016/12/05 03:30:55, dominicc wrote:
> Nice comment explaining the intent of the test and how it relates to the
> implementation.

Thanks!

By the way, are tests in these folder Blink-specific or are they upstreamed to
any Web Platform test suites? I am not sure this test makes sense for any other
rendering engine.

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/HTMLImageElement/fallback-image-moved-across-documents.html:12:
// image icon inside, fail, and end up with infinitely nested fallback content.
On 2016/12/05 03:30:55, dominicc wrote:
> I'm curious--how would loading the broken image icon fail?

Done, I have extended the explanation.

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/HTMLImageElement/fallback-image-moved-across-documents.html:20:
document.body.appendChild(nestedDocument.body.removeChild(i));
On 2016/12/05 at 03:30:55, dominicc wrote:
> Removes happen implicitly as part of apppend, appendChild, etc., so maybe you
could simplify this by just doing document.body.append(i).

Done.

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/subresource_filter/image-allow-disallow-transitions.html
(right):

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/subresource_filter/image-allow-disallow-transitions.html:42:
for (let initialState of Object.keys(INITIAL_URLS)) {
On 2016/12/05 03:30:55, dominicc wrote:
> Maybe
> 
> for (let [initialState, initialUrl] of Object.entries(INITIAL_URLS)) {
> 
> is more succinct since you don't have to mention INITIAL_URLS again below;
same
> for FINAL_URLS.

Done.

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/subresource_filter/image-allow-disallow-transitions.html:52:
if (window.testRunner && --numPendingImages == 0)
On 2016/12/05 03:30:55, dominicc wrote:
> If you use testharness you could consider using the event->promise mapping
thing
> they have, promise_test, and Promise.all to join them instead of managing an
> explicit counter this way.
> 
> On the other hand, this is pretty readable, so up to you! Might be interesting
> to see if it is a lot simpler one way or the other.

Done.

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/subresource_filter/image-disallowed.html
(right):

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/subresource_filter/image-disallowed.html:17:
assert_equals(i.clientWidth, 100, "Images that are not disallowed are displayed
at their natural width.");
On 2016/12/05 at 03:30:55, dominicc wrote:
> Consider 'should be' instead of 'are'. When an assertion fails and you see
text like
> 
> Images that are not disallowed are displayed at their natural width.
> 
> there's a grain of doubt whether the 'are' is describing what did happen, or
what was desired to happen. Whereas
> 
> Images that are not disallowed should be displayed at their natural width.
> 
> removes that doubt.

Good point, done everywhere.

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/subresource_filter/image-disallowed.html:35:
document.body.appendChild(i);
On 2016/12/05 at 03:30:55, dominicc wrote:
> Consider using 'append', which is a wee bit shorter and hence more readable.

Done everywhere.

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (left):

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:352: if
(m_useFallbackContent)
On 2016/12/05 03:30:55, dominicc wrote:
> What do you think about using a switch here? Then it's crystal clear you cover
> three cases:
> 
> Fallback -> block flow
> Primary -> image
> Collapsed -> dcheck

Done.

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:832:
m_layoutDisposition = LayoutDisposition::Collapsed;
On 2016/12/05 03:30:55, dominicc wrote:
> This code smells a tiny bit, it looks like when you set m_layoutDisposition
you
> should call reattachContent right after/soon after. Can we make that a setter
> which ties those operations together into unit?
> 
> If that's not *always* the case, can we add some unit tests and integration
> tests exercising when it's different?

It's always the case, done some refactoring. WDYT?

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:836:
setUseFallbackContent();
On 2016/12/05 03:30:55, dominicc wrote:
> Isn't this just ensureFallbackForGeneratedContent?

Refactored this a bit too.  For now I don't want to introduce the collapsed
state to the code path from ensureFallbackForGeneratedContent(), I'll look into
that later.

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:858: if
(m_layoutDisposition == LayoutDisposition::PrimaryContent ||
On 2016/12/05 03:30:55, dominicc wrote:
> Again, I wonder if a switch with fallthrough would be easier to read? It
teaches
> the reader what the three exhaustive cases are even if they don't have the
> definition of LayoutDisposition at the front of their mind.

Done.

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLImageElement.h (right):

https://codereview.chromium.org/2535383003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.h:145: PrimaryContent,
On 2016/12/05 03:30:55, dominicc wrote:
> It would be splendid to have some comments explaining what these are! Maybe a
> link to a relevant spec on the enum itself would be swell.

Done.

[commit-bot: I haz the power, 2016-12-06 15:45:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-06 15:45:37]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[engedy, 2016-12-06 21:41:50]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 21:42:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-06 22:14:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-06 22:14:24]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[dominicc (has gone to gerrit), 2016-12-07 01:51:42]

Splendid work. core/html LGTM modulo some trivial suggestions inline.

https://codereview.chromium.org/2535383003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/2535383003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:357: switch
(m_layoutDisposition) {
Sadness, it seems the compiler doesn't work out that these cases are exhaustive.
I guess we need to add default and have LayoutDisposition::Collapsed fall
through to that.

https://codereview.chromium.org/2535383003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:828: // FIXME: The
special casing for generated content in createLayoutObject
TODO(whom): [what to fix] [when]

If this is just "this is sad but we're going to live with it" then I'd just
write a comment to that effect, no FIXME/TODO.

https://codereview.chromium.org/2535383003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:840: if
(imageLoader().image() &&
Seems OK, but maybe

bool explainingVar = that expression;
setLayoutDisposition(explainingVar ? ... : ...);

is both more succinct and more informative because of the name of explainingVar?

https://codereview.chromium.org/2535383003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLImageElement.h (right):

https://codereview.chromium.org/2535383003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.h:143: // Controls how an
image element appears in the layout. See:
Awesome :D thank you.

[engedy, 2016-12-07 08:20:09]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 08:20:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[engedy, 2016-12-07 09:37:41]

https://codereview.chromium.org/2535383003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/2535383003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:357: switch
(m_layoutDisposition) {
On 2016/12/07 01:51:42, dominicc wrote:
> Sadness, it seems the compiler doesn't work out that these cases are
exhaustive.
> I guess we need to add default and have LayoutDisposition::Collapsed fall
> through to that.

Done.

https://codereview.chromium.org/2535383003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:828: // FIXME: The
special casing for generated content in createLayoutObject
On 2016/12/07 01:51:42, dominicc wrote:
> TODO(whom): [what to fix] [when]
> 
> If this is just "this is sad but we're going to live with it" then I'd just
> write a comment to that effect, no FIXME/TODO.

At this point I am optimistic that this should be fixable. Filed and referenced
a bug.

https://codereview.chromium.org/2535383003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:840: if
(imageLoader().image() &&
On 2016/12/07 01:51:42, dominicc wrote:
> Seems OK, but maybe
> 
> bool explainingVar = that expression;
> setLayoutDisposition(explainingVar ? ... : ...);
> 
> is both more succinct and more informative because of the name of
explainingVar?

Done.

[engedy, 2016-12-07 09:37:56]

engedy@chromium.org changed reviewers:
- japhet@chromium.org

[engedy, 2016-12-07 09:44:31]

The CQ bit was unchecked by engedy@chromium.org

[engedy, 2016-12-07 09:44:32]

The CQ bit was checked by engedy@chromium.org

[engedy, 2016-12-07 09:44:32]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
dominicc@chromium.org
Link to the patchset: https://codereview.chromium.org/2535383003/#ps180001
(title: "Second round of comments from dominicc@.")

[commit-bot: I haz the power, 2016-12-07 09:44:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 10:05:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-07 10:05:50]

Failed to apply patch for
third_party/WebKit/Source/core/fetch/ResourceFetcher.h:
While running git apply --index -p1;
  error: patch failed:
third_party/WebKit/Source/core/fetch/ResourceFetcher.h:205
  error: third_party/WebKit/Source/core/fetch/ResourceFetcher.h: patch does not
apply

Patch:       third_party/WebKit/Source/core/fetch/ResourceFetcher.h
Index: third_party/WebKit/Source/core/fetch/ResourceFetcher.h
diff --git a/third_party/WebKit/Source/core/fetch/ResourceFetcher.h
b/third_party/WebKit/Source/core/fetch/ResourceFetcher.h
index
10991d53bc1c1f9a5faf09c75fcd50f4e2971b0d..08f9bdf0ef4db6524d769c27114f18f01481243e
100644
--- a/third_party/WebKit/Source/core/fetch/ResourceFetcher.h
+++ b/third_party/WebKit/Source/core/fetch/ResourceFetcher.h
@@ -117,7 +117,9 @@ class CORE_EXPORT ResourceFetcher
   void stopFetching();
   bool isFetching() const;
 
-  bool willFollowRedirect(Resource*, ResourceRequest&, const
ResourceResponse&);
+  ResourceRequestBlockedReason willFollowRedirect(Resource*,
+                                                  ResourceRequest&,
+                                                  const ResourceResponse&);
   enum DidFinishLoadingReason {
     DidFinishLoading,
     DidFinishFirstPartInMultipart
@@ -185,7 +187,8 @@ class CORE_EXPORT ResourceFetcher
                                   const ResourceFactory&,
                                   const SubstituteData&);
   Resource* resourceForBlockedRequest(const FetchRequest&,
-                                      const ResourceFactory&);
+                                      const ResourceFactory&,
+                                      ResourceRequestBlockedReason);
 
   // RevalidationPolicy enum values are used in UMAs https://crbug.com/579496.
   enum RevalidationPolicy { Use, Revalidate, Reload, Load };
@@ -205,7 +208,8 @@ class CORE_EXPORT ResourceFetcher
                        ResourceRequest&,
                        const ResourceResponse&,
                        const ResourceLoaderOptions&);
-  bool canAccessResponse(Resource*, const ResourceResponse&) const;
+  ResourceRequestBlockedReason canAccessResponse(Resource*,
+                                                 const ResourceResponse&)
const;
 
   bool resourceNeedsLoad(Resource*, const FetchRequest&, RevalidationPolicy);
   bool shouldDeferImageLoad(const KURL&) const;

[engedy, 2016-12-07 10:20:44]

The CQ bit was checked by engedy@chromium.org

[engedy, 2016-12-07 10:20:45]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
dominicc@chromium.org
Link to the patchset: https://codereview.chromium.org/2535383003/#ps200001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-12-07 10:21:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 11:49:15]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1481106044893400,
"parent_rev": "1fca32f8090d12996e6d9c8e91785c24004ee077", "commit_rev":
"8419803d19ee612b5199f92349ee6b2b0ca0056c"}

[commit-bot: I haz the power, 2016-12-07 11:49:43]

Description was changed from

==========
Collapse images disallowed by the Safe Browsing Subresource Filter.

When the ImageResource being loaded into an HTMLImageElement is disallowed by 
subresource filtering, the element is now collapsed in the layout (instead of
showing fallback content).

This is achieved by the following changes:

 1) Internally, access check related methods in FetchContext, ResourceLoader
    and ResourceFetcher have been refactored so that they now return a
    ResourceRequestBlockedReason enumerator instead of a boolean value.

 2) ResourceError now has an |m_shouldCollapseInitiator| field. When a load is
    disallowed because of subresource filtering, the ResourceError instance set
    on the failed Resource will now has this boolean set.

 3) HTMLImageElement now has three states: showing primary content, showing
    fallback content, and collapsed. When the |m_shouldCollapseInitiator| is
    set on its ImageResource, it enters this new, third state.

BUG=609747
==========

to

==========
Collapse images disallowed by the Safe Browsing Subresource Filter.

When the ImageResource being loaded into an HTMLImageElement is disallowed by 
subresource filtering, the element is now collapsed in the layout (instead of
showing fallback content).

This is achieved by the following changes:

 1) Internally, access check related methods in FetchContext, ResourceLoader
    and ResourceFetcher have been refactored so that they now return a
    ResourceRequestBlockedReason enumerator instead of a boolean value.

 2) ResourceError now has an |m_shouldCollapseInitiator| field. When a load is
    disallowed because of subresource filtering, the ResourceError instance set
    on the failed Resource will now has this boolean set.

 3) HTMLImageElement now has three states: showing primary content, showing
    fallback content, and collapsed. When the |m_shouldCollapseInitiator| is
    set on its ImageResource, it enters this new, third state.

BUG=609747
==========

[commit-bot: I haz the power, 2016-12-07 11:49:44]

Committed patchset #8 (id:200001)

[commit-bot: I haz the power, 2016-12-07 11:51:26]

Description was changed from

==========
Collapse images disallowed by the Safe Browsing Subresource Filter.

When the ImageResource being loaded into an HTMLImageElement is disallowed by 
subresource filtering, the element is now collapsed in the layout (instead of
showing fallback content).

This is achieved by the following changes:

 1) Internally, access check related methods in FetchContext, ResourceLoader
    and ResourceFetcher have been refactored so that they now return a
    ResourceRequestBlockedReason enumerator instead of a boolean value.

 2) ResourceError now has an |m_shouldCollapseInitiator| field. When a load is
    disallowed because of subresource filtering, the ResourceError instance set
    on the failed Resource will now has this boolean set.

 3) HTMLImageElement now has three states: showing primary content, showing
    fallback content, and collapsed. When the |m_shouldCollapseInitiator| is
    set on its ImageResource, it enters this new, third state.

BUG=609747
==========

to

==========
Collapse images disallowed by the Safe Browsing Subresource Filter.

When the ImageResource being loaded into an HTMLImageElement is disallowed by
subresource filtering, the element is now collapsed in the layout (instead of
showing fallback content).

This is achieved by the following changes:

 1) Internally, access check related methods in FetchContext, ResourceLoader
    and ResourceFetcher have been refactored so that they now return a
    ResourceRequestBlockedReason enumerator instead of a boolean value.

 2) ResourceError now has an |m_shouldCollapseInitiator| field. When a load is
    disallowed because of subresource filtering, the ResourceError instance set
    on the failed Resource will now has this boolean set.

 3) HTMLImageElement now has three states: showing primary content, showing
    fallback content, and collapsed. When the |m_shouldCollapseInitiator| is
    set on its ImageResource, it enters this new, third state.

BUG=609747

Committed: https://crrev.com/3c67d0c8a79c59b3afa1d73761387fb3be82a0bd
Cr-Commit-Position: refs/heads/master@{#436917}
==========

[commit-bot: I haz the power, 2016-12-07 11:51:27]

Patchset 8 (id:??) landed as
https://crrev.com/3c67d0c8a79c59b3afa1d73761387fb3be82a0bd
Cr-Commit-Position: refs/heads/master@{#436917}