Load data URI images in an async way according to spec

Load data URI images in an async way according to spec

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.

This is bringing https://codereview.chromium.org/1256233002/ to the Chromium repo.

BUG=514206
Committed: https://crrev.com/91aa6fb579a1f656ef274ebe73e426db99b5f52f
Cr-Commit-Position: refs/heads/master@{#363444}

[Yoav Weiss, 2015-12-03 13:15:07]

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

[Yoav Weiss, 2015-12-03 13:15:07]

Bringing back https://codereview.chromium.org/1256233002/ to life. This CL makes
sure that data URIs are loaded async according to spec.

That would avoid situations where users see "flash of broken image" and would
align our implementation with everyone else's.

[pdr., 2015-12-03 20:12:37]

On 2015/12/03 at 13:15:07, yoav wrote:
> Bringing back https://codereview.chromium.org/1256233002/ to life. This CL
makes sure that data URIs are loaded async according to spec.
> 
> That would avoid situations where users see "flash of broken image" and would
align our implementation with everyone else's.

I still think we're conflating two issues here:
1) data urls loading synchronously. For the reasons in
https://github.com/ResponsiveImagesCG/picture-element/issues/223#issuecomment...
I'm still skeptical that this is a spec violation or even a bug, particularly
from the user's perspective.
2) async timing issues with picture source selection in the presence of broken
images.

I'm really not dead-set on my position on #1, I just think we should be looking
at that issue by itself.

For #2, we still return cached images synchronously so I think the bug will
still exist. For example, couldn't we cache a broken image, or synchronously
fail for cross-origin reasons, or even just lose the data race with a
filesystem-loaded broken image?

What about just delaying showing a broken image until all pending source
requests have finished at which point we know the image is truly broken?

[Yoav Weiss, 2015-12-03 20:50:41]

On 2015/12/03 20:12:37, pdr wrote:
> On 2015/12/03 at 13:15:07, yoav wrote:
> > Bringing back https://codereview.chromium.org/1256233002/ to life. This CL
> makes sure that data URIs are loaded async according to spec.
> > 
> > That would avoid situations where users see "flash of broken image" and
would
> align our implementation with everyone else's.
> 
> I still think we're conflating two issues here:
> 1) data urls loading synchronously. For the reasons in
>
https://github.com/ResponsiveImagesCG/picture-element/issues/223#issuecomment...
> I'm still skeptical that this is a spec violation or even a bug, particularly
> from the user's perspective.

I hoped I addressed your skepticism at
https://github.com/ResponsiveImagesCG/picture-element/issues/223#issuecomment...

Yes, in Safari images are loaded synchronously, but I'm hoping that won't be the
case for much longer. Now that proper microtasks have landed, async image
loading support is probably next: https://bugs.webkit.org/show_bug.cgi?id=134488

All other browsers now load images (including data URIs) async, and it would be
better for compat to align with that behavior.

> 2) async timing issues with picture source selection in the presence of broken
> images.
> 
> I'm really not dead-set on my position on #1, I just think we should be
looking
> at that issue by itself.
> 
> For #2, we still return cached images synchronously so I think the bug will
> still exist.

There could still be a "flash of cached image", yes. We would need to figure out
a mechanism that prevents that.

>  For example, couldn't we cache a broken image, or synchronously
> fail for cross-origin reasons, or even just lose the data race with a
> filesystem-loaded broken image?
> 
> What about just delaying showing a broken image until all pending source
> requests have finished at which point we know the image is truly broken?

From my perspective, this CL addresses #1 and we would still need to address #2
as a separate issue, even if the CL mitigates #2 to some extent.

[pdr., 2015-12-04 01:17:46]

pdr@chromium.org changed reviewers:
+ esprehn@chromium.org

[pdr., 2015-12-04 01:17:47]

I see, this is the proposal to switch to synchronous datauri images. I
misunderstood and thought it was for #2.

Can you think of a case where we would be one frame "behind" because of async
image loading, or cases where this would cause 2 update styles where only one
was present before? I tried some simple testcases and didn't observe an effect
with or without this patch due to the loads being kicked off before updatestyle.
AFAIK, layout cannot depend on a css image property either.

If this patch has no effect on performance and aligns our behavior with other
browsers, I support this after all.

[Yoav Weiss, 2015-12-07 07:14:54]

On 2015/12/04 01:17:47, pdr wrote:
> I see, this is the proposal to switch to synchronous datauri images. I
> misunderstood and thought it was for #2.
> 
> Can you think of a case where we would be one frame "behind" because of async
> image loading, or cases where this would cause 2 update styles where only one
> was present before? I tried some simple testcases and didn't observe an effect
> with or without this patch due to the loads being kicked off before
updatestyle.
> AFAIK, layout cannot depend on a css image property either.

The data URL is loaded at the next microtask, so I don't think that there are
scenarios where this will result in a double updatestyle.

> 
> If this patch has no effect on performance and aligns our behavior with other
> browsers, I support this after all.

[pdr., 2015-12-07 07:17:05]

On 2015/12/07 at 07:14:54, yoav wrote:
> On 2015/12/04 01:17:47, pdr wrote:
> > I see, this is the proposal to switch to synchronous datauri images. I
> > misunderstood and thought it was for #2.
> > 
> > Can you think of a case where we would be one frame "behind" because of
async
> > image loading, or cases where this would cause 2 update styles where only
one
> > was present before? I tried some simple testcases and didn't observe an
effect
> > with or without this patch due to the loads being kicked off before
updatestyle.
> > AFAIK, layout cannot depend on a css image property either.
> 
> The data URL is loaded at the next microtask, so I don't think that there are
scenarios where this will result in a double updatestyle.
> 
> > 
> > If this patch has no effect on performance and aligns our behavior with
other
> > browsers, I support this after all.

LGTM

[Yoav Weiss, 2015-12-07 07:41:24]

The CQ bit was checked by yoav@yoav.ws

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

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1492303003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1492303003/20001

[commit-bot: I haz the power, 2015-12-07 08:46:40]

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

[commit-bot: I haz the power, 2015-12-07 08:46:40]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Yoav Weiss, 2015-12-07 08:53:19]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2015-12-07 08:54:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1492303003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1492303003/20001

[commit-bot: I haz the power, 2015-12-07 10:54:49]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-12-07 10:55:56]

Description was changed from

==========
Load data URI images in an async way according to spec

According to
https://github.com/ResponsiveImagesCG/picture-element/issues/223#issuecomment...
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.

This is bringing https://codereview.chromium.org/1256233002/ to the Chromium
repo.

BUG=514206
==========

to

==========
Load data URI images in an async way according to spec

According to
https://github.com/ResponsiveImagesCG/picture-element/issues/223#issuecomment...
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.

This is bringing https://codereview.chromium.org/1256233002/ to the Chromium
repo.

BUG=514206

Committed: https://crrev.com/91aa6fb579a1f656ef274ebe73e426db99b5f52f
Cr-Commit-Position: refs/heads/master@{#363444}
==========

[commit-bot: I haz the power, 2015-12-07 10:55:56]

Patchset 2 (id:??) landed as
https://crrev.com/91aa6fb579a1f656ef274ebe73e426db99b5f52f
Cr-Commit-Position: refs/heads/master@{#363444}

[Yoav Weiss, 2015-12-16 07:31:15]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1525793006/ by yoav@yoav.ws.

The reason for reverting is: Perf regressions:
https://code.google.com/p/chromium/issues/detail?id=569023.