Move data URI image resources to be async loaded.

Move data URI image resources to be async loaded.

According to https://github.com/ResponsiveImagesCG/picture-element/issues/223#issuecomment-69772765
we seem to be the only browser that loads data URIs immediately (as if they were cached).
It was introduced as part of the move to async image loading: https://codereview.chromium.org/200923002

This is now causing issues: https://code.google.com/p/chromium/issues/detail?id=469131

This CL changes this behavior to be spec compliant and load data URIs asynchronously, like other resources.

BUG=514206

[Yoav Weiss, 2015-07-27 13:51:47]

yoav@yoav.ws changed reviewers:
+ mkwst@chromium.org

[Yoav Weiss, 2015-07-27 13:53:36]

Hey Mike,

Apparently we're Doing The Wring Thing™ when it comes to loading of data URIs as
images.
Changing it would be a Web exposed change. Do you think it warrants a PSA?

Cheers :)
Yoav

[Yoav Weiss, 2015-07-27 13:53:57]

On 2015/07/27 13:53:36, Yoav Weiss wrote:
> Hey Mike,
> 
> Apparently we're Doing The Wring Thing™ when it comes to loading of data URIs
as
> images.
> Changing it would be a Web exposed change. Do you think it warrants a PSA?
> 
> Cheers :)
> Yoav

s/Wring/Wrong :|

[Mike West, 2015-07-27 14:24:49]

Code change looks reasonable, ActivityLoggerTest.RequestResource says it might
not be. Flake?

LGTM if not, otherwise, please fix it. :)

[Yoav Weiss, 2015-07-27 14:25:20]

On 2015/07/27 13:53:57, Yoav Weiss wrote:
> On 2015/07/27 13:53:36, Yoav Weiss wrote:
> > Hey Mike,
> > 
> > Apparently we're Doing The Wring Thing™ when it comes to loading of data
URIs
> as
> > images.
> > Changing it would be a Web exposed change. Do you think it warrants a PSA?
> > 
> > Cheers :)
> > Yoav
> 
> s/Wring/Wrong :|

Looks like this breaks some tests. I'll investigate further.

[Yoav Weiss, 2015-07-27 14:30:43]

On 2015/07/27 14:24:49, Mike West wrote:
> Code change looks reasonable, ActivityLoggerTest.RequestResource says it might
> not be. Flake?
> 
> LGTM if not, otherwise, please fix it. :)

The activityLogger explicitly tests that a resource load is immediately
triggered.
I'll fix the test to correspond with image loading behavior.

[Mike West, 2015-07-27 19:12:37]

On 2015/07/27 at 14:30:43, yoav wrote:
> On 2015/07/27 14:24:49, Mike West wrote:
> > Code change looks reasonable, ActivityLoggerTest.RequestResource says it
might
> > not be. Flake?
> > 
> > LGTM if not, otherwise, please fix it. :)
> 
> The activityLogger explicitly tests that a resource load is immediately
triggered.
> I'll fix the test to correspond with image loading behavior.

Great.

[Yoav Weiss, 2015-07-27 21:01:00]

On 2015/07/27 19:12:37, Mike West wrote:
> On 2015/07/27 at 14:30:43, yoav wrote:
> > On 2015/07/27 14:24:49, Mike West wrote:
> > > Code change looks reasonable, ActivityLoggerTest.RequestResource says it
> might
> > > not be. Flake?
> > > 
> > > LGTM if not, otherwise, please fix it. :)
> > 
> > The activityLogger explicitly tests that a resource load is immediately
> triggered.
> > I'll fix the test to correspond with image loading behavior.
> 
> Great.

We still have a bunch of failures, since apparently some layout tests rely on
the old behavior.

[Yoav Weiss, 2015-07-27 22:12:02]

yoav@yoav.ws changed reviewers:
+ pdr@chromium.org

[Yoav Weiss, 2015-07-27 22:12:20]

On 2015/07/27 21:01:00, Yoav Weiss wrote:
> On 2015/07/27 19:12:37, Mike West wrote:
> > On 2015/07/27 at 14:30:43, yoav wrote:
> > > On 2015/07/27 14:24:49, Mike West wrote:
> > > > Code change looks reasonable, ActivityLoggerTest.RequestResource says it
> > might
> > > > not be. Flake?
> > > > 
> > > > LGTM if not, otherwise, please fix it. :)
> > > 
> > > The activityLogger explicitly tests that a resource load is immediately
> > triggered.
> > > I'll fix the test to correspond with image loading behavior.
> > 
> > Great.
> 
> We still have a bunch of failures, since apparently some layout tests rely on
> the old behavior.

Seems like the sync behavior was added at
https://codereview.chromium.org/16433002 to resolve issues related to SVG.

[pdr., 2015-07-28 04:11:30]

On 2015/07/27 at 22:12:20, yoav wrote:
> On 2015/07/27 21:01:00, Yoav Weiss wrote:
> > On 2015/07/27 19:12:37, Mike West wrote:
> > > On 2015/07/27 at 14:30:43, yoav wrote:
> > > > On 2015/07/27 14:24:49, Mike West wrote:
> > > > > Code change looks reasonable, ActivityLoggerTest.RequestResource says
it
> > > might
> > > > > not be. Flake?
> > > > > 
> > > > > LGTM if not, otherwise, please fix it. :)
> > > > 
> > > > The activityLogger explicitly tests that a resource load is immediately
> > > triggered.
> > > > I'll fix the test to correspond with image loading behavior.
> > > 
> > > Great.
> > 
> > We still have a bunch of failures, since apparently some layout tests rely
on
> > the old behavior.
> 
> Seems like the sync behavior was added at
https://codereview.chromium.org/16433002 to resolve issues related to SVG.

Some history from that change... SVG images are a little special in that they
cannot load any subresources (no access to the network). The main idea in
https://codereview.chromium.org/16433002 was to immediately cache data uri
images without even hitting the (unavailable) network. I don't think the patch
actually required synchronous loads.

That said, why wouldn't we load an image synchronously? The actual decode will
happen on a separate thread so I don't think there's a perf reason not to. If
anything, not loading synchronously seems like a perf issue. I updated
https://github.com/ResponsiveImagesCG/picture-element/issues/223#issuecomment...
with some more info.

[Yoav Weiss, 2015-07-28 06:45:43]

On 2015/07/28 04:11:30, pdr wrote:
> On 2015/07/27 at 22:12:20, yoav wrote:
> > On 2015/07/27 21:01:00, Yoav Weiss wrote:
> > > On 2015/07/27 19:12:37, Mike West wrote:
> > > > On 2015/07/27 at 14:30:43, yoav wrote:
> > > > > On 2015/07/27 14:24:49, Mike West wrote:
> > > > > > Code change looks reasonable, ActivityLoggerTest.RequestResource
says
> it
> > > > might
> > > > > > not be. Flake?
> > > > > > 
> > > > > > LGTM if not, otherwise, please fix it. :)
> > > > > 
> > > > > The activityLogger explicitly tests that a resource load is
immediately
> > > > triggered.
> > > > > I'll fix the test to correspond with image loading behavior.
> > > > 
> > > > Great.
> > > 
> > > We still have a bunch of failures, since apparently some layout tests rely
> on
> > > the old behavior.
> > 
> > Seems like the sync behavior was added at
> https://codereview.chromium.org/16433002 to resolve issues related to SVG.
> 
> Some history from that change... SVG images are a little special in that they
> cannot load any subresources (no access to the network). The main idea in
> https://codereview.chromium.org/16433002 was to immediately cache data uri
> images without even hitting the (unavailable) network. I don't think the patch
> actually required synchronous loads.
> 
> That said, why wouldn't we load an image synchronously? The actual decode will
> happen on a separate thread so I don't think there's a perf reason not to. If
> anything, not loading synchronously seems like a perf issue. I updated
>
https://github.com/ResponsiveImagesCG/picture-element/issues/223#issuecomment...
> with some more info.

The reason this came up was a scenario with <picture> where the <img src>
contained a broken data URI image, where we see a Flash Of Wrong Image™.
That caused me to dig up the old picture issue, and see that we're the only
browser that does that.
The scenario above is not awful in itself, but if we can avoid it, it'd be
better.

With that said, I now understand that a Flash Of Wrong Image can also occur if
the <img src> resource is in memory cache. Let's take the discussion back to the
picture spec issue, wait for zcorpan to come back (couple of weeks), and then
we'd see.

[Yoav Weiss, 2015-12-03 09:53:30]

Closing this as iit is now on the wrong repo. Opened
https://codereview.chromium.org/1492303003/ instead