Load data URI images in an async way according to spec (take 3)

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 relanding https://codereview.chromium.org/1492303003 and
https://codereview.chromium.org/2173003002/

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

[Yoav Weiss, 2016-12-21 21:19:45]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 21:20:25]

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

[Yoav Weiss, 2016-12-21 21:24:38]

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

[Yoav Weiss, 2016-12-21 21:24:38]

Hey,

Finally got around to working on async data URI image loading again. This is
reviving https://codereview.chromium.org/2173003002/ which revived
https://codereview.chromium.org/1492303003
Let's hope third time's a charm...

On top of the previous patch, this also changed the devtools' screen capture
mechanism to be asynchronous, so that it won't break by this patch.

[commit-bot: I haz the power, 2016-12-21 22:25:04]

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

[commit-bot: I haz the power, 2016-12-21 22:25:05]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Yoav Weiss, 2016-12-21 23:07:26]

On 2016/12/21 21:24:38, Yoav Weiss wrote:
> Hey,
> 
> Finally got around to working on async data URI image loading again. This is
> reviving https://codereview.chromium.org/2173003002/ which revived
> https://codereview.chromium.org/1492303003
> Let's hope third time's a charm...
> 
> On top of the previous patch, this also changed the devtools' screen capture
> mechanism to be asynchronous, so that it won't break by this patch.

hmm, this seems to have broken a couple of tests :/

[Yoav Weiss, 2016-12-22 07:32:54]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 07:33:10]

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-22 08:39:53]

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

[commit-bot: I haz the power, 2016-12-22 08:39:53]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Yoav Weiss, 2016-12-22 10:22:20]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 10:22:40]

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-22 12:04:20]

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

[commit-bot: I haz the power, 2016-12-22 12:04:21]

Dry run: This issue passed the CQ dry run.

[Yoav Weiss, 2016-12-22 12:04:25]

On 2016/12/21 23:07:26, Yoav Weiss wrote:
> On 2016/12/21 21:24:38, Yoav Weiss wrote:
> > Hey,
> > 
> > Finally got around to working on async data URI image loading again. This is
> > reviving https://codereview.chromium.org/2173003002/ which revived
> > https://codereview.chromium.org/1492303003
> > Let's hope third time's a charm...
> > 
> > On top of the previous patch, this also changed the devtools' screen capture
> > mechanism to be asynchronous, so that it won't break by this patch.
> 
> hmm, this seems to have broken a couple of tests :/

And it's green!!

[pdr., 2016-12-22 21:19:41]

On 2016/12/22 at 12:04:25, yoav wrote:
> On 2016/12/21 23:07:26, Yoav Weiss wrote:
> > On 2016/12/21 21:24:38, Yoav Weiss wrote:
> > > Hey,
> > > 
> > > Finally got around to working on async data URI image loading again. This
is
> > > reviving https://codereview.chromium.org/2173003002/ which revived
> > > https://codereview.chromium.org/1492303003
> > > Let's hope third time's a charm...
> > > 
> > > On top of the previous patch, this also changed the devtools' screen
capture
> > > mechanism to be asynchronous, so that it won't break by this patch.
> > 
> > hmm, this seems to have broken a couple of tests :/
> 
> And it's green!!

LGTM!!

[Yoav Weiss, 2016-12-22 21:20:15]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2016-12-22 21:20:32]

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

[Yoav Weiss, 2016-12-22 21:24:57]

On 2016/12/22 21:19:41, pdr (OOO Dec 23 to Jan 2) wrote:
> On 2016/12/22 at 12:04:25, yoav wrote:
> > On 2016/12/21 23:07:26, Yoav Weiss wrote:
> > > On 2016/12/21 21:24:38, Yoav Weiss wrote:
> > > > Hey,
> > > > 
> > > > Finally got around to working on async data URI image loading again.
This
> is
> > > > reviving https://codereview.chromium.org/2173003002/ which revived
> > > > https://codereview.chromium.org/1492303003
> > > > Let's hope third time's a charm...
> > > > 
> > > > On top of the previous patch, this also changed the devtools' screen
> capture
> > > > mechanism to be asynchronous, so that it won't break by this patch.
> > > 
> > > hmm, this seems to have broken a couple of tests :/
> > 
> > And it's green!!
> 
> LGTM!!

Thanks! Fingers crossed that it sticks this time...

[commit-bot: I haz the power, 2016-12-22 21:27:28]

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

[commit-bot: I haz the power, 2016-12-22 21:27:29]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Yoav Weiss, 2016-12-22 21:32:16]

On 2016/12/22 21:27:29, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

pfeldman@ - can you review the devtools changes?

[dgozman, 2016-12-27 07:08:23]

dgozman@chromium.org changed reviewers:
+ dgozman@chromium.org

[dgozman, 2016-12-27 07:08:24]

https://codereview.chromium.org/2592113003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js
(right):

https://codereview.chromium.org/2592113003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:457:
setTimeout(function() {
Should this be pageImage.onload = ... ?
Also, using arrow function eliminates the need for |scope|.

[Yoav Weiss, 2017-01-02 11:27:45]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2017-01-02 11:27:55]

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

[Yoav Weiss, 2017-01-02 11:28:26]

On 2016/12/27 07:08:24, dgozman wrote:
>
https://codereview.chromium.org/2592113003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js
> (right):
> 
>
https://codereview.chromium.org/2592113003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:457:
> setTimeout(function() {
> Should this be pageImage.onload = ... ?
> Also, using arrow function eliminates the need for |scope|.

Good point. Modified the tests to be onload based.

[Yoav Weiss, 2017-01-02 11:29:02]

On 2017/01/02 11:28:26, Yoav Weiss wrote:
> On 2016/12/27 07:08:24, dgozman wrote:
> >
>
https://codereview.chromium.org/2592113003/diff/40001/third_party/WebKit/Sour...
> > File
third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js
> > (right):
> > 
> >
>
https://codereview.chromium.org/2592113003/diff/40001/third_party/WebKit/Sour...
> >
third_party/WebKit/Source/devtools/front_end/emulation/DeviceModeView.js:457:
> > setTimeout(function() {
> > Should this be pageImage.onload = ... ?
> > Also, using arrow function eliminates the need for |scope|.
> 
> Good point. Modified the tests to be onload based.

As well as the actual code...

[commit-bot: I haz the power, 2017-01-02 12:53:46]

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

[commit-bot: I haz the power, 2017-01-02 12:53:47]

Dry run: This issue passed the CQ dry run.

[dgozman, 2017-01-02 16:56:17]

Should we change all data-url images to use onload?
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front...

Also, does this change mean that |complete| is never true synchronously?
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front...

I hope this doesn't affect third-party developers, but sometimes web platform
api changes which require DevTools frontend patch break more than expected.

[Yoav Weiss, 2017-01-02 17:09:04]

On 2017/01/02 16:56:17, dgozman wrote:
> Should we change all data-url images to use onload?
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front...

Yes, we should. I guess the question is, do we want to land all such changes as
part of this CL, or can they land separately.

> 
> Also, does this change mean that |complete| is never true synchronously?
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front...

Yeah.

> 
> I hope this doesn't affect third-party developers, but sometimes web platform
> api changes which require DevTools frontend patch break more than expected.

Open Web developers should not rely on synchronous data URI loading for images,
as it's not something that works across browsers. (Chrome-only according to
https://github.com/ResponsiveImagesCG/picture-element/issues/223#issuecomment...
)

[Yoav Weiss, 2017-01-02 20:31:59]

On 2017/01/02 17:09:04, Yoav Weiss wrote:
> On 2017/01/02 16:56:17, dgozman wrote:
> > Should we change all data-url images to use onload?
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front...
> 
> Yes, we should. I guess the question is, do we want to land all such changes
as
> part of this CL, or can they land separately.
> 
> > 
> > Also, does this change mean that |complete| is never true synchronously?
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front...
> 
> Yeah.
> 
> > 
> > I hope this doesn't affect third-party developers, but sometimes web
platform
> > api changes which require DevTools frontend patch break more than expected.
> 
> Open Web developers should not rely on synchronous data URI loading for
images,
> as it's not something that works across browsers. (Chrome-only according to
>
https://github.com/ResponsiveImagesCG/picture-element/issues/223#issuecomment...
> )

Fixed the devtools code you spotted that relied on sync loading of data URIs.
PTAL

[Yoav Weiss, 2017-01-02 20:37:19]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2017-01-02 20:37:30]

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, 2017-01-02 22:30:09]

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

[commit-bot: I haz the power, 2017-01-02 22:30:10]

Dry run: This issue passed the CQ dry run.

[dgozman, 2017-01-03 18:06:13]

Thank you for the fixes! lgtm

[Yoav Weiss, 2017-01-03 19:31:51]

On 2017/01/03 18:06:13, dgozman wrote:
> Thank you for the fixes! lgtm

Thanks for reviewing! :)

[Yoav Weiss, 2017-01-03 19:31:57]

The CQ bit was checked by yoav@yoav.ws

[Yoav Weiss, 2017-01-03 19:31:58]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2592113003/#ps80001
(title: "Fixed more devtools reliance on sync loading")

[commit-bot: I haz the power, 2017-01-03 19:32:48]

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, 2017-01-03 19:39:56]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483471917491870,
"parent_rev": "29bedbce44469fec68e0731d6c86c3497da41ece", "commit_rev":
"7b26aa0281870726aa08eedd24fd789ec6f79aba"}

[commit-bot: I haz the power, 2017-01-03 19:40:29]

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 relanding https://codereview.chromium.org/1492303003 and
https://codereview.chromium.org/2173003002/

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 relanding https://codereview.chromium.org/1492303003 and
https://codereview.chromium.org/2173003002/

BUG=514206

Review-Url: https://codereview.chromium.org/2592113003
==========

[commit-bot: I haz the power, 2017-01-03 19:40:33]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2017-01-03 19:42:48]

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 relanding https://codereview.chromium.org/1492303003 and
https://codereview.chromium.org/2173003002/

BUG=514206

Review-Url: https://codereview.chromium.org/2592113003
==========

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 relanding https://codereview.chromium.org/1492303003 and
https://codereview.chromium.org/2173003002/

BUG=514206

Committed: https://crrev.com/b6403174c6111ea0947dc7cf8b1eb8a6f9e8546c
Cr-Commit-Position: refs/heads/master@{#441184}
==========

[commit-bot: I haz the power, 2017-01-03 19:42:50]

Patchset 5 (id:??) landed as
https://crrev.com/b6403174c6111ea0947dc7cf8b1eb8a6f9e8546c
Cr-Commit-Position: refs/heads/master@{#441184}