Fix loading of data: URI images wrt loadsImagesAutomatically setting

Fix loading of data: URI images wrt loadsImagesAutomatically setting

This fixes AwSettingsTest#testBlockNetworkImagesDoesNotBlockDataUrlImage
in Chrome broken after Blink r176642

BUG=356292, 387101
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177425

[eseidel, 2014-06-26 16:21:50]

Nate is high lord of the loader soup.

[mnaganov (inactive), 2014-06-26 16:24:18]

Thanks for routing the issue, Eric!

Nate, here is some context about this issue:
https://code.google.com/p/chromium/issues/detail?id=356292#c17

[eseidel, 2014-06-26 16:24:35]

https://codereview.chromium.org/355043002/diff/1/LayoutTests/loader/data-uri-...
File
LayoutTests/loader/data-uri-images-load-when-loading-images-automatically-disabled.html
(right):

https://codereview.chromium.org/355043002/diff/1/LayoutTests/loader/data-uri-...
LayoutTests/loader/data-uri-images-load-when-loading-images-automatically-disabled.html:7:
testRunner.waitUntilDone();
You shouldn't have to wait until done?  onload won't be sent until all
subresources are loaded...  We don't dump until onload is sent.

https://codereview.chromium.org/355043002/diff/1/LayoutTests/loader/data-uri-...
LayoutTests/loader/data-uri-images-load-when-loading-images-automatically-disabled.html:22:
setTimeout(timeout, 15000);
You souldn't need this.  content_shell knows how to time out itself. :)  I would
set this to *way* smaller if you want to have your own timeout.  Also, I think
images have an onerror handler you could use instead.

https://codereview.chromium.org/355043002/diff/1/Source/core/fetch/ResourceFe...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/355043002/diff/1/Source/core/fetch/ResourceFe...
Source/core/fetch/ResourceFetcher.cpp:874: ResourcePtr<Resource> resource = type
== Resource::Image && request.resourceRequest().url().protocolIsData() ?
Do we want to just push this logic down into createResource?

[mnaganov (inactive), 2014-06-26 16:42:40]

Argh, it looks like the change breaks "data:image/svg+xml" images, so I need to
reconsider it.

https://codereview.chromium.org/355043002/diff/1/LayoutTests/loader/data-uri-...
File
LayoutTests/loader/data-uri-images-load-when-loading-images-automatically-disabled.html
(right):

https://codereview.chromium.org/355043002/diff/1/LayoutTests/loader/data-uri-...
LayoutTests/loader/data-uri-images-load-when-loading-images-automatically-disabled.html:7:
testRunner.waitUntilDone();
On 2014/06/26 16:24:35, eseidel wrote:
> You shouldn't have to wait until done?  onload won't be sent until all
> subresources are loaded...  We don't dump until onload is sent.

Thanks for the tip! Indeed, this isn't needed.

https://codereview.chromium.org/355043002/diff/1/LayoutTests/loader/data-uri-...
LayoutTests/loader/data-uri-images-load-when-loading-images-automatically-disabled.html:22:
setTimeout(timeout, 15000);
On 2014/06/26 16:24:35, eseidel wrote:
> You souldn't need this.  content_shell knows how to time out itself. :)  I
would
> set this to *way* smaller if you want to have your own timeout.  Also, I think
> images have an onerror handler you could use instead.

Removed the timeout. I can't use onerror because loadsImagesAutomatically
doesn't forbid loading images, it just defers loading.

Anyway, the test now prints "PASS" on success, or leaves a message I have added
as a default body text.

https://codereview.chromium.org/355043002/diff/1/Source/core/fetch/ResourceFe...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/355043002/diff/1/Source/core/fetch/ResourceFe...
Source/core/fetch/ResourceFetcher.cpp:874: ResourcePtr<Resource> resource = type
== Resource::Image && request.resourceRequest().url().protocolIsData() ?
On 2014/06/26 16:24:35, eseidel wrote:
> Do we want to just push this logic down into createResource? 

I don't think so, because imageResourceFromDataURIRequest uses createResource by
itself so we will create an infinite loop :)

[mnaganov (inactive), 2014-06-26 17:02:24]

OK, SVG tests should work now. data:image/svg+xml is a special case within a
special case.

[eseidel, 2014-06-26 17:37:17]

https://codereview.chromium.org/355043002/diff/20001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/355043002/diff/20001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:154: if (!data || mimetype ==
"image/svg+xml")
Comments would be helpful. :)

https://codereview.chromium.org/355043002/diff/20001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:875: if (type == Resource::Image &&
request.resourceRequest().url().protocolIsData())
Are any other callers going to want this wrapping around
imageResource/createResource?  Should this be a in re-usable helper?

[mnaganov (inactive), 2014-06-27 15:13:59]

I think I have found a better approach -- just make
ResourceFetcher::requestPreload to use the same fetch function as ImageLoader
does.

BTW, I have noticed that ResourceFetcher::fetchUserCSSStyleSheet also contains
special logic that may be bypassed by ResourceFetcher::requestPreload. Maybe
this needs to be fixed too?

[mnaganov (inactive), 2014-06-30 08:03:50]

eseidel@, japhet@: Can you please take a look?

[Nate Chapin, 2014-07-01 19:12:43]

LGTM

https://codereview.chromium.org/355043002/diff/40001/LayoutTests/loader/data-...
File
LayoutTests/loader/data-uri-images-load-when-loading-images-automatically-disabled.html
(right):

https://codereview.chromium.org/355043002/diff/40001/LayoutTests/loader/data-...
LayoutTests/loader/data-uri-images-load-when-loading-images-automatically-disabled.html:6:
if (window.testRunner) {
Nit: No {}

[mnaganov (inactive), 2014-07-02 08:39:29]

Thanks, Nate!

I see that Eric has disabled the preloader again, but I'm anyway committing as
it adds a layout test and fixes preloader behaviour for images.

https://codereview.chromium.org/355043002/diff/40001/LayoutTests/loader/data-...
File
LayoutTests/loader/data-uri-images-load-when-loading-images-automatically-disabled.html
(right):

https://codereview.chromium.org/355043002/diff/40001/LayoutTests/loader/data-...
LayoutTests/loader/data-uri-images-load-when-loading-images-automatically-disabled.html:6:
if (window.testRunner) {
On 2014/07/01 19:12:43, Nate Chapin wrote:
> Nit: No {}

Done.

[mnaganov (inactive), 2014-07-02 08:39:40]

The CQ bit was checked by mnaganov@chromium.org

[commit-bot: I haz the power, 2014-07-02 08:40:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mnaganov@chromium.org/355043002/60001

[commit-bot: I haz the power, 2014-07-02 09:46:21]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[commit-bot: I haz the power, 2014-07-02 10:53:05]

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

[commit-bot: I haz the power, 2014-07-02 10:53:06]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[mnaganov (inactive), 2014-07-02 15:30:21]

The CQ bit was checked by mnaganov@chromium.org

[commit-bot: I haz the power, 2014-07-02 15:31:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mnaganov@chromium.org/355043002/80001

[commit-bot: I haz the power, 2014-07-02 16:15:56]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/13972)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/15093)

[commit-bot: I haz the power, 2014-07-02 16:26:43]

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

[commit-bot: I haz the power, 2014-07-02 16:26:44]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[mnaganov (inactive), 2014-07-02 16:55:30]

The CQ bit was checked by mnaganov@chromium.org

[commit-bot: I haz the power, 2014-07-02 16:57:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mnaganov@chromium.org/355043002/100001

[commit-bot: I haz the power, 2014-07-02 20:49:53]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/15136)

[commit-bot: I haz the power, 2014-07-02 21:23:25]

Change committed as 177425