Add decode() functionality to image elements.

Add decode() functionality to image elements.

This allows developers to request a decode of an img element.
When the promise resolves, the compositor will keep the image
locked for two frames, ensuring that if appended into the DOM,
jank is minimized.

Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/LMV9dBG_1-I


CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2769823002
Cr-Commit-Position: refs/heads/master@{#476445}
Committed: https://chromium.googlesource.com/chromium/src/+/a840c6a20c62e5ad20ec4e3419370173566b2850

[vmpstr, 2017-03-22 20:30:54]

Description was changed from

==========
Add decode() functionality to image elements.
==========

to

==========
Add decode() functionality to image elements.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[domenic, 2017-03-23 07:49:19]

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

[domenic, 2017-03-23 07:49:20]

Lots of feedback on how to best write testharness.js promise tests. Coverage
overall looks pretty great though.

I realized we also need to support srcset, not just src. So probably tests for
that too.

https://codereview.chromium.org/2769823002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/external/wpt/image-decodes/image-decode.html
(right):

https://codereview.chromium.org/2769823002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/image-decodes/image-decode.html:22:
return img.decode().then(
You should just do `return img.decode()`. promise_test tests will succeed if and
only if the returned promise fulfills.

Actually, even better would be `return img.decode().then(arg =>
assert_equals(arg, undefined, "The promise must fulfill with undefined"))`

https://codereview.chromium.org/2769823002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/image-decodes/image-decode.html:60:
return img.decode().then(
The way to test promise rejections is with `return promise_rejects(t,
"EncodingError", img.decode())`. Here `t` is passed to your test function, i.e.
it becomes `promise_test(function(t) { ...`

https://codereview.chromium.org/2769823002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/image-decodes/image-decode.html:137:
return new Promise(function(resolve, _) {
This promise thing is best done via

```
const img = new Image();
img.src = "/images/green.png";
const promise1 = img.decode();
img.src = "/images/green.svg";
const promise2 = img.decode();
return Promise.all([
  promise_rejects(t, promise1, "EncodingError"),
  promise2
]);
```

[domenic, 2017-03-23 07:51:40]

Oh, we also want coverage that a different promise is returned every time even
for multiple calls in a row.

[vmpstr, 2017-03-23 21:18:33]

Description was changed from

==========
Add decode() functionality to image elements.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Add decode() functionality to image elements.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[vmpstr, 2017-03-23 21:19:34]

Thanks for the comments, that simplifies the tests a lot! I've updated them and
added both the srcset tests and checks for inequality of repeated decode
requests.

[domenic, 2017-03-27 10:24:20]

Another small test nit, but otherwise tests look good.

You'll need more reviewers for the rest of the code BTW; I am not a Blink
reviewer.

https://codereview.chromium.org/2769823002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/external/wpt/image-decodes/image-decode.html
(right):

https://codereview.chromium.org/2769823002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/image-decodes/image-decode.html:17:
promise_test(function() {
These test messages and/or the comments above each test should be the second arg
to promise_test, after the test function. If that gets fixed then IMO you don't
need to pass them to assert_equals.

[kouhei (in TOK), 2017-03-28 00:40:33]

kouhei@chromium.org changed reviewers:
+ hiroshige@chromium.org, kouhei@chromium.org

[kouhei (in TOK), 2017-03-28 00:40:34]

+hiroshige for ImageResource stuff. I wonder if this should be implemented by
ImageResourceContent level so that other content consumer can benefit from this
too?

[vmpstr, 2017-03-29 21:02:44]

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

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

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

[vmpstr, 2017-03-29 21:04:59]

Description was changed from

==========
Add decode() functionality to image elements.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Add decode() functionality to image elements. This allows
developers to request a decode of an img element. When
the promise resolves, the compositor will keep the image
locked for two frames, ensuring that if appended into
the DOM, jank is minimized.

Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/LMV9dBG_1-I


CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[vmpstr, 2017-03-29 21:04:59]

vmpstr@chromium.org changed reviewers:
+ enne@chromium.org, esprehn@chromium.org

[vmpstr, 2017-03-29 21:07:52]

Please take a look.

kouhei@, can you elaborate on your proposal for doing this at
ImageResourceContent level? (sorry, I'm only somewhat familiar with the Blink
side of image things).

[commit-bot: I haz the power, 2017-03-29 21:50:46]

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

[commit-bot: I haz the power, 2017-03-29 21:50:47]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[enne (OOO), 2017-03-30 14:30:15]

cc and content lgtm.  I don't know enough about Blink to really review the rest.

[hiroshige, 2017-03-31 08:22:59]

> ImageResourceContent
I think implementing the logic in ImageResourceContent is not necessary,
because:
- decode() is spec'ed in the HTML spec (i.e. related to HTMLImageElement and
ImageLoader), and the implementation works on blink::Image.
- The implementation uses |page->chromeClient()|, while ImageResourceContent and
blink::Image are not associated with a page.

(I'm commenting while looking at https://github.com/whatwg/html/pull/2332/files
as the corresponding spec)

Perhaps it is better to implement the blink-side logic mainly in ImageLoader,
but not so sure.

Pros for implementing in ImageLoader:
ImageLoader is the main class that implements the img element in the HTML spec.
For example, decode() spec says it triggers decoding when the image element
becomes 'completely available', which corresponds to
ImageLoader::imageNotifyFinished() (|m_imageComplete = true;|).
Adding the logic to ImageLoader::imageNotifyFinished() instead of
HTMLImageElement::defaultEventHandler() would make the impl to correspond more
directly to the spec.

Cons:
ImageLoader is already very complex and (probably) not completely spec
compliant.
If decode() is not needed for SVG image element etc., implementing the logic in
HTMLImageElement might make the implementation separated from ImageLoader's
complexity.

kouhei@, WDYT?

[hiroshige, 2017-03-31 08:24:24]

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

https://codereview.chromium.org/2769823002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:889: requestDecode();
This always causes ChromeClient's requestDecode() even the image's decode() is
not called. Doesn't this degrade the performance?

[vmpstr, 2017-03-31 17:00:03]

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

[vmpstr, 2017-03-31 17:00:16]

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

https://codereview.chromium.org/2769823002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:889: requestDecode();
On 2017/03/31 08:24:24, hiroshige wrote:
> This always causes ChromeClient's requestDecode() even the image's decode() is
> not called. Doesn't this degrade the performance?

This only passes here if line 885 is false, which means we have a decode promise
resolver, which means decode was indeed called. I'll add a comment to make it
more clear

[commit-bot: I haz the power, 2017-03-31 17:00:32]

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-03-31 17:41:43]

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

[commit-bot: I haz the power, 2017-03-31 17:41:44]

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

[vmpstr, 2017-03-31 18:42:05]

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

[commit-bot: I haz the power, 2017-03-31 18:42:31]

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-03-31 19:27:04]

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

[commit-bot: I haz the power, 2017-03-31 19:27:05]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[vmpstr, 2017-03-31 23:26:50]

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

[commit-bot: I haz the power, 2017-03-31 23:27:34]

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-04-01 00:12:34]

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

[commit-bot: I haz the power, 2017-04-01 00:12:35]

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

[vmpstr, 2017-04-03 19:02:58]

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

[commit-bot: I haz the power, 2017-04-03 19:03:51]

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-04-03 22:40:30]

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

[commit-bot: I haz the power, 2017-04-03 22:40:31]

Dry run: This issue passed the CQ dry run.

[kouhei (in TOK), 2017-04-04 02:55:02]

On 2017/03/31 08:22:59, hiroshige wrote:
> > ImageResourceContent
> I think implementing the logic in ImageResourceContent is not necessary,
> because:
> - decode() is spec'ed in the HTML spec (i.e. related to HTMLImageElement and
> ImageLoader), and the implementation works on blink::Image.
> - The implementation uses |page->chromeClient()|, while ImageResourceContent
and
> blink::Image are not associated with a page.
I'm not sure about this. My understanding is that while decoding pipeline
depends on the page's LayerTreeHost scheduler, the decoded data itself is page
independent.
cc team: Is this true?

> (I'm commenting while looking at
https://github.com/whatwg/html/pull/2332/files
> as the corresponding spec)
> 
> Perhaps it is better to implement the blink-side logic mainly in ImageLoader,
> but not so sure.
> 
> Pros for implementing in ImageLoader:
> ImageLoader is the main class that implements the img element in the HTML
spec.
> For example, decode() spec says it triggers decoding when the image element
> becomes 'completely available', which corresponds to
> ImageLoader::imageNotifyFinished() (|m_imageComplete = true;|).
> Adding the logic to ImageLoader::imageNotifyFinished() instead of
> HTMLImageElement::defaultEventHandler() would make the impl to correspond more
> directly to the spec.
> 
> Cons:
> ImageLoader is already very complex and (probably) not completely spec
> compliant.
> If decode() is not needed for SVG image element etc., implementing the logic
in
> HTMLImageElement might make the implementation separated from ImageLoader's
> complexity.
> 
> kouhei@, WDYT?
I'd implement this in ImageLoader.

[kouhei (in TOK), 2017-04-04 02:55:14]

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

https://codereview.chromium.org/2769823002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:162: for (auto&
resolver : m_decodePromiseResolvers)
Can we simply if (success) and do two for loops?

https://codereview.chromium.org/2769823002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:884: void
HTMLImageElement::defaultEventHandler(Event* event) {
This is hook too generic to use.
How about using {HTML,}ImageLoader::imageNotifyFinished hook for this?

[vmpstr, 2017-04-04 18:13:34]

On 2017/04/04 02:55:02, kouhei wrote:
> On 2017/03/31 08:22:59, hiroshige wrote:
> > > ImageResourceContent
> > I think implementing the logic in ImageResourceContent is not necessary,
> > because:
> > - decode() is spec'ed in the HTML spec (i.e. related to HTMLImageElement and
> > ImageLoader), and the implementation works on blink::Image.
> > - The implementation uses |page->chromeClient()|, while ImageResourceContent
> and
> > blink::Image are not associated with a page.
> I'm not sure about this. My understanding is that while decoding pipeline
> depends on the page's LayerTreeHost scheduler, the decoded data itself is page
> independent.
> cc team: Is this true?

I'm not sure what you mean by page independent here. The decoded data is stored
in the compositor's rasterization stack, which is associated with a specific
renderer, which in turn is associated with a page, afaik. To put it differently,
this decode is only accessible by content rasterization that happens in the
compositor. The requests that happen here only get a signal that "yes, the
compositor is good to go", so if ImageLoader is not aware of the compositor (via
chrome client), then I'm not entirely sure how to hook this up or whether it
makes sense there.

> 
> > (I'm commenting while looking at
> https://github.com/whatwg/html/pull/2332/files
> > as the corresponding spec)
> > 
> > Perhaps it is better to implement the blink-side logic mainly in
ImageLoader,
> > but not so sure.
> > 
> > Pros for implementing in ImageLoader:
> > ImageLoader is the main class that implements the img element in the HTML
> spec.
> > For example, decode() spec says it triggers decoding when the image element
> > becomes 'completely available', which corresponds to
> > ImageLoader::imageNotifyFinished() (|m_imageComplete = true;|).
> > Adding the logic to ImageLoader::imageNotifyFinished() instead of
> > HTMLImageElement::defaultEventHandler() would make the impl to correspond
more
> > directly to the spec.
> > 
> > Cons:
> > ImageLoader is already very complex and (probably) not completely spec
> > compliant.
> > If decode() is not needed for SVG image element etc., implementing the logic
> in
> > HTMLImageElement might make the implementation separated from ImageLoader's
> > complexity.
> > 
> > kouhei@, WDYT?
> I'd implement this in ImageLoader.

[vmpstr, 2017-04-04 20:52:22]

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

https://codereview.chromium.org/2769823002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:162: for (auto&
resolver : m_decodePromiseResolvers)
On 2017/04/04 02:55:14, kouhei wrote:
> Can we simply if (success) and do two for loops?

Done.

https://codereview.chromium.org/2769823002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:884: void
HTMLImageElement::defaultEventHandler(Event* event) {
On 2017/04/04 02:55:14, kouhei wrote:
> This is hook too generic to use.
> How about using {HTML,}ImageLoader::imageNotifyFinished hook for this?

Done. I forwarded the call here (to a new
HTMLImageElement::imageNotifyFinished). Let me know if you'd like more targeted
function names.

[esprehn, 2017-04-07 00:17:34]

esprehn@chromium.org changed reviewers:
+ dcheng@chromium.org
- esprehn@chromium.org

[dcheng, 2017-04-07 07:33:41]

I haven't reviewed this in detail yet, but I notice it's being plumbed through
WebViewImpl. I think that means it might not work if a Document in an OOPIF
calls decode().

We're still working on cleaning up the widget plumbing, but for now, you can
plumb through the LocalFrame& of the element, and use that to get to through
local root's WebFrameWidget (e.g. something like
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/ChromeClie...)

WebFrameWidgetBase is a shared base class that helps abstract out the difference
between the root WebWidget (which is a WebView) and the widgets for 'local
roots' (which are WebFrameWidgets). Since it already exposes the layer tree
view, I think the rest should be pretty straightforward.

[kouhei (in TOK), 2017-04-07 14:26:29]

Thanks!

On 2017/04/04 18:13:34, vmpstr wrote:
> On 2017/04/04 02:55:02, kouhei wrote:
> > On 2017/03/31 08:22:59, hiroshige wrote:
> > > > ImageResourceContent
> > > I think implementing the logic in ImageResourceContent is not necessary,
> > > because:
> > > - decode() is spec'ed in the HTML spec (i.e. related to HTMLImageElement
and
> > > ImageLoader), and the implementation works on blink::Image.
> > > - The implementation uses |page->chromeClient()|, while
ImageResourceContent
> > and
> > > blink::Image are not associated with a page.
> > I'm not sure about this. My understanding is that while decoding pipeline
> > depends on the page's LayerTreeHost scheduler, the decoded data itself is
page
> > independent.
> > cc team: Is this true?
> 
> I'm not sure what you mean by page independent here. The decoded data is
stored
> in the compositor's rasterization stack, which is associated with a specific
> renderer, which in turn is associated with a page, afaik. To put it
differently,
> this decode is only accessible by content rasterization that happens in the
> compositor. The requests that happen here only get a signal that "yes, the
> compositor is good to go", so if ImageLoader is not aware of the compositor
(via
> chrome client), then I'm not entirely sure how to hook this up or whether it
> makes sense there.
(I don't want to block this patch on this so please proceed with this CL.)
Can we use the decoded image across different frames in the same renderer
process?

> > > (I'm commenting while looking at
> > https://github.com/whatwg/html/pull/2332/files
> > > as the corresponding spec)
> > > 
> > > Perhaps it is better to implement the blink-side logic mainly in
> ImageLoader,
> > > but not so sure.
> > > 
> > > Pros for implementing in ImageLoader:
> > > ImageLoader is the main class that implements the img element in the HTML
> > spec.
> > > For example, decode() spec says it triggers decoding when the image
element
> > > becomes 'completely available', which corresponds to
> > > ImageLoader::imageNotifyFinished() (|m_imageComplete = true;|).
> > > Adding the logic to ImageLoader::imageNotifyFinished() instead of
> > > HTMLImageElement::defaultEventHandler() would make the impl to correspond
> more
> > > directly to the spec.
> > > 
> > > Cons:
> > > ImageLoader is already very complex and (probably) not completely spec
> > > compliant.
> > > If decode() is not needed for SVG image element etc., implementing the
logic
> > in
> > > HTMLImageElement might make the implementation separated from
ImageLoader's
> > > complexity.
> > > 
> > > kouhei@, WDYT?
> > I'd implement this in ImageLoader.

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

https://codereview.chromium.org/2769823002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:146:
m_weakPtrFactory.createWeakPtr()));
wrapWeakPersistent(this)?

[vmpstr, 2017-04-07 20:43:43]

> I haven't reviewed this in detail yet, but I notice it's being plumbed through
> WebViewImpl. I think that means it might not work if a Document in an OOPIF
> calls decode().
> 
> We're still working on cleaning up the widget plumbing, but for now, you can
> plumb through the LocalFrame& of the element, and use that to get to through
> local root's WebFrameWidget (e.g. something like
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/ChromeClie...)
> 
> WebFrameWidgetBase is a shared base class that helps abstract out the
difference
> between the root WebWidget (which is a WebView) and the widgets for 'local
> roots' (which are WebFrameWidgets). Since it already exposes the layer tree
> view, I think the rest should be pretty straightforward.

As discussed offline, I'll hold off on these changes until you have time to take
a closer look. Since these images don't have to be attached to the DOM when
decode is requested, I think there might be an issue using the local frame...

> Can we use the decoded image across different frames in the same renderer
> process?

I think I might have a wrong understanding of what "frame" means here. In terms
of compositor frame (ie, frames that change with time), the image is available
for a few frames after the request is satisfied. However, I suspect that's not
the frame you're talking about here :)

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

https://codereview.chromium.org/2769823002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:146:
m_weakPtrFactory.createWeakPtr()));
On 2017/04/07 14:26:28, kouhei wrote:
> wrapWeakPersistent(this)?

Hmm, we have to revoke the weak ptrs if the src of the image changes, since we
might get callbacks for the wrong image. Is that possible with weak persistent?

[dcheng, 2017-04-12 06:53:17]

> > Can we use the decoded image across different frames in the same renderer
> > process?
> 
> I think I might have a wrong understanding of what "frame" means here. In
terms
> of compositor frame (ie, frames that change with time), the image is available
> for a few frames after the request is satisfied. However, I suspect that's not
> the frame you're talking about here :)

I think the question is if you request decode() in one Document, can you append
the result to a different Document?

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

https://codereview.chromium.org/2769823002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:143:
page->chromeClient().requestDecode(
After reading this through, I think it's fine to just plumb through the
document's frame() here.

Then, as previously mentioned, ChromeClientImpl will use the frame to look up
the LocalRoot(), and then get the WebLocalFrameImpl from that, and then get its
WebFrameWidgetBase from that. From there, we can use the WebLayerTreeView to
enter cc.

https://codereview.chromium.org/2769823002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:146:
m_weakPtrFactory.createWeakPtr()));
On 2017/04/07 20:43:43, vmpstr wrote:
> On 2017/04/07 14:26:28, kouhei wrote:
> > wrapWeakPersistent(this)?
> 
> Hmm, we have to revoke the weak ptrs if the src of the image changes, since we
> might get callbacks for the wrong image. Is that possible with weak
persistent?

Other potential options:
- PostCancellableTask, which encapsulates this logic internally:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/WebTa...

- Use a sequence number when the source changes

(I am not sure how common it is to use WeakPtrFactory with Oilpan objects, but
it does seem a bit unusual)

[vmpstr, 2017-04-17 18:13:11]

PTAL

On 2017/04/12 at 06:53:17, dcheng wrote:
> > > Can we use the decoded image across different frames in the same renderer
> > > process?
> > 
> > I think I might have a wrong understanding of what "frame" means here. In
terms
> > of compositor frame (ie, frames that change with time), the image is
available
> > for a few frames after the request is satisfied. However, I suspect that's
not
> > the frame you're talking about here :)
> 
> I think the question is if you request decode() in one Document, can you
append the result to a different Document?

Oh I see. Yeah, as long as the document is serviced by the same compositor
instance, things will work as expected. I verified with chrishtr@ that this is
the case if you have an iframe and you attach the image to a different document.

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

https://codereview.chromium.org/2769823002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:143:
page->chromeClient().requestDecode(
On 2017/04/12 at 06:53:16, dcheng wrote:
> After reading this through, I think it's fine to just plumb through the
document's frame() here.
> 
> Then, as previously mentioned, ChromeClientImpl will use the frame to look up
the LocalRoot(), and then get the WebLocalFrameImpl from that, and then get its
WebFrameWidgetBase from that. From there, we can use the WebLayerTreeView to
enter cc.

Done. Let me know if this looks better...

https://codereview.chromium.org/2769823002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:146:
m_weakPtrFactory.createWeakPtr()));
On 2017/04/12 at 06:53:16, dcheng wrote:
> On 2017/04/07 20:43:43, vmpstr wrote:
> > On 2017/04/07 14:26:28, kouhei wrote:
> > > wrapWeakPersistent(this)?
> > 
> > Hmm, we have to revoke the weak ptrs if the src of the image changes, since
we
> > might get callbacks for the wrong image. Is that possible with weak
persistent?
> 
> Other potential options:
> - PostCancellableTask, which encapsulates this logic internally:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/WebTa...
> 
> - Use a sequence number when the source changes
> 
> (I am not sure how common it is to use WeakPtrFactory with Oilpan objects, but
it does seem a bit unusual)

Done, I used a sequence number here with wrapWeakPersistent.

[dcheng, 2017-04-17 18:48:02]

The plumbing largely LG, modulo a null check but:

On 2017/04/17 18:13:11, vmpstr wrote:
> PTAL
> 
> On 2017/04/12 at 06:53:17, dcheng wrote:
> > > > Can we use the decoded image across different frames in the same
renderer
> > > > process?
> > > 
> > > I think I might have a wrong understanding of what "frame" means here. In
> terms
> > > of compositor frame (ie, frames that change with time), the image is
> available
> > > for a few frames after the request is satisfied. However, I suspect that's
> not
> > > the frame you're talking about here :)
> > 
> > I think the question is if you request decode() in one Document, can you
> append the result to a different Document?
> 
> Oh I see. Yeah, as long as the document is serviced by the same compositor
> instance, things will work as expected. I verified with chrishtr@ that this is
> the case if you have an iframe and you attach the image to a different
document.

I think this is going to be problematic for OOPIF, where it's possible for
same-origin documents to not share the same compositor. If we have a frame tree
like:


   A (main)
    \
     B (child)
      \
       A (grandchild)


The two frames on A are same-origin but will have different compositors in
OOPIF. What would be involved in making it work across different compositors?
(+kenrb)

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

https://codereview.chromium.org/2769823002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:140: LocalFrame* frame
= GetDocument().GetFrame();
Hmm. I think we might need to null check here.

I think JS could remove the iframe it's running in, and then request decode. In
that case, I think |frame| will be null here.

[vmpstr, 2017-04-17 20:51:25]

> I think this is going to be problematic for OOPIF, where it's possible for
same-origin documents to not share the same compositor. If we have a frame tree
like:
> 
> 
>    A (main)
>     \
>      B (child)
>       \
>        A (grandchild)
> 
> 
> The two frames on A are same-origin but will have different compositors in
OOPIF. What would be involved in making it work across different compositors?
(+kenrb)

I talked to Chris (+chrihstr) about this and although the situation is possible,
it's unclear whether it's possible to get an image that is decoded on main
attached to grandchild (or vice versa). Do you know how that would be possible?

We don't really have any plans on making compositor specific caches be
transferable to other compositors. As somewhat of an aside, the decode can
succeed on one compositor and a different compositor can attach the exact same
url image to its content, which will just be a cache miss in the compositor.
This means that if there are subtle situations where it's possible, the only
downside is that the performance benefit provided by the decode api would be
eliminated (ie, things shouldn't "break")

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

https://codereview.chromium.org/2769823002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:140: LocalFrame* frame
= GetDocument().GetFrame();
On 2017/04/17 at 18:48:02, dcheng wrote:
> Hmm. I think we might need to null check here.
> 
> I think JS could remove the iframe it's running in, and then request decode.
In that case, I think |frame| will be null here.

Done.

[esprehn, 2017-04-17 20:59:52]

Description was changed from

==========
Add decode() functionality to image elements. This allows
developers to request a decode of an img element. When
the promise resolves, the compositor will keep the image
locked for two frames, ensuring that if appended into
the DOM, jank is minimized.

Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/LMV9dBG_1-I


CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Add decode() functionality to image elements.

This allows developers to request a decode of an img element.
When the promise resolves, the compositor will keep the image
locked for two frames, ensuring that if appended into the DOM,
jank is minimized.

Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/LMV9dBG_1-I


CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[dcheng, 2017-04-17 21:01:37]

On 2017/04/17 20:51:25, vmpstr wrote:
> > I think this is going to be problematic for OOPIF, where it's possible for
> same-origin documents to not share the same compositor. If we have a frame
tree
> like:
> > 
> > 
> >    A (main)
> >     \
> >      B (child)
> >       \
> >        A (grandchild)
> > 
> > 
> > The two frames on A are same-origin but will have different compositors in
> OOPIF. What would be involved in making it work across different compositors?
> (+kenrb)
> 
> I talked to Chris (+chrihstr) about this and although the situation is
possible,
> it's unclear whether it's possible to get an image that is decoded on main
> attached to grandchild (or vice versa). Do you know how that would be
possible?

Actually this isn't OOPIF specific -- origin A could call window.open(), which
creates a new window with origin A. When the promise succeeds, we could call
appendChild() on any same-origin document: nothing prevents an image element
created in one document from being adopted by another document. As long as we
handle this case correctly (which from below it sounds like we probably will, as
it's just a cache miss), then that's fine.

> 
> We don't really have any plans on making compositor specific caches be
> transferable to other compositors. As somewhat of an aside, the decode can
> succeed on one compositor and a different compositor can attach the exact same
> url image to its content, which will just be a cache miss in the compositor.
> This means that if there are subtle situations where it's possible, the only
> downside is that the performance benefit provided by the decode api would be
> eliminated (ie, things shouldn't "break")
> 
>
https://codereview.chromium.org/2769823002/diff/280001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):
> 
>
https://codereview.chromium.org/2769823002/diff/280001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/html/HTMLImageElement.cpp:140: LocalFrame*
frame
> = GetDocument().GetFrame();
> On 2017/04/17 at 18:48:02, dcheng wrote:
> > Hmm. I think we might need to null check here.
> > 
> > I think JS could remove the iframe it's running in, and then request decode.
> In that case, I think |frame| will be null here.
> 
> Done.

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

https://codereview.chromium.org/2769823002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:138:
!GetImageLoader().GetImage()->GetImage() || !frame) {
It would be nice to have coverage for the !frame case as well.

[vmpstr, 2017-04-17 21:59:23]

On 2017/04/17 at 21:01:37, dcheng wrote:
> On 2017/04/17 20:51:25, vmpstr wrote:
> > > I think this is going to be problematic for OOPIF, where it's possible for
> > same-origin documents to not share the same compositor. If we have a frame
tree
> > like:
> > > 
> > > 
> > >    A (main)
> > >     \
> > >      B (child)
> > >       \
> > >        A (grandchild)
> > > 
> > > 
> > > The two frames on A are same-origin but will have different compositors in
> > OOPIF. What would be involved in making it work across different
compositors?
> > (+kenrb)
> > 
> > I talked to Chris (+chrihstr) about this and although the situation is
possible,
> > it's unclear whether it's possible to get an image that is decoded on main
> > attached to grandchild (or vice versa). Do you know how that would be
possible?
> 
> Actually this isn't OOPIF specific -- origin A could call window.open(), which
creates a new window with origin A. When the promise succeeds, we could call
appendChild() on any same-origin document: nothing prevents an image element
created in one document from being adopted by another document. As long as we
handle this case correctly (which from below it sounds like we probably will, as
it's just a cache miss), then that's fine.
> 
> > 
> > We don't really have any plans on making compositor specific caches be
> > transferable to other compositors. As somewhat of an aside, the decode can
> > succeed on one compositor and a different compositor can attach the exact
same
> > url image to its content, which will just be a cache miss in the compositor.
> > This means that if there are subtle situations where it's possible, the only
> > downside is that the performance benefit provided by the decode api would be
> > eliminated (ie, things shouldn't "break")
> > 
> >
https://codereview.chromium.org/2769823002/diff/280001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):
> > 
> >
https://codereview.chromium.org/2769823002/diff/280001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/html/HTMLImageElement.cpp:140: LocalFrame*
frame
> > = GetDocument().GetFrame();
> > On 2017/04/17 at 18:48:02, dcheng wrote:
> > > Hmm. I think we might need to null check here.
> > > 
> > > I think JS could remove the iframe it's running in, and then request
decode.
> > In that case, I think |frame| will be null here.
> > 
> > Done.
> 
>
https://codereview.chromium.org/2769823002/diff/300001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):
> 
>
https://codereview.chromium.org/2769823002/diff/300001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/html/HTMLImageElement.cpp:138:
!GetImageLoader().GetImage()->GetImage() || !frame) {
> It would be nice to have coverage for the !frame case as well.

I've verified that it works locally via:

<!doctype HTML>

<iframe id="frame" srcdoc="iframe"></iframe>
<div id="container"></div>
<script>
onload = function() {
  var frame = document.getElementById("frame");
  var img = frame.contentDocument.createElement("img");
  img.src = "cat.jpg";
  img.onload = function() {
    frame.parentNode.removeChild(frame);
    img.decode().then(function() {});
  }
};
</script>

At the point of HTMLImageElement::RequestDecode the frame is nullptr. However,
although we call reject on the resolver, js doesn't see any of that... I'm not
quite sure how to test this automatically. domenic@ do you have any suggestions?

[domenic, 2017-04-17 22:09:27]

On 2017/04/17 at 21:59:23, vmpstr wrote:
> On 2017/04/17 at 21:01:37, dcheng wrote:
> > On 2017/04/17 20:51:25, vmpstr wrote:
> > > > I think this is going to be problematic for OOPIF, where it's possible
for
> > > same-origin documents to not share the same compositor. If we have a frame
tree
> > > like:
> > > > 
> > > > 
> > > >    A (main)
> > > >     \
> > > >      B (child)
> > > >       \
> > > >        A (grandchild)
> > > > 
> > > > 
> > > > The two frames on A are same-origin but will have different compositors
in
> > > OOPIF. What would be involved in making it work across different
compositors?
> > > (+kenrb)
> > > 
> > > I talked to Chris (+chrihstr) about this and although the situation is
possible,
> > > it's unclear whether it's possible to get an image that is decoded on main
> > > attached to grandchild (or vice versa). Do you know how that would be
possible?
> > 
> > Actually this isn't OOPIF specific -- origin A could call window.open(),
which creates a new window with origin A. When the promise succeeds, we could
call appendChild() on any same-origin document: nothing prevents an image
element created in one document from being adopted by another document. As long
as we handle this case correctly (which from below it sounds like we probably
will, as it's just a cache miss), then that's fine.
> > 
> > > 
> > > We don't really have any plans on making compositor specific caches be
> > > transferable to other compositors. As somewhat of an aside, the decode can
> > > succeed on one compositor and a different compositor can attach the exact
same
> > > url image to its content, which will just be a cache miss in the
compositor.
> > > This means that if there are subtle situations where it's possible, the
only
> > > downside is that the performance benefit provided by the decode api would
be
> > > eliminated (ie, things shouldn't "break")
> > > 
> > >
https://codereview.chromium.org/2769823002/diff/280001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):
> > > 
> > >
https://codereview.chromium.org/2769823002/diff/280001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/html/HTMLImageElement.cpp:140: LocalFrame*
frame
> > > = GetDocument().GetFrame();
> > > On 2017/04/17 at 18:48:02, dcheng wrote:
> > > > Hmm. I think we might need to null check here.
> > > > 
> > > > I think JS could remove the iframe it's running in, and then request
decode.
> > > In that case, I think |frame| will be null here.
> > > 
> > > Done.
> > 
> >
https://codereview.chromium.org/2769823002/diff/300001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):
> > 
> >
https://codereview.chromium.org/2769823002/diff/300001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/html/HTMLImageElement.cpp:138:
!GetImageLoader().GetImage()->GetImage() || !frame) {
> > It would be nice to have coverage for the !frame case as well.
> 
> I've verified that it works locally via:
> 
> <!doctype HTML>
> 
> <iframe id="frame" srcdoc="iframe"></iframe>
> <div id="container"></div>
> <script>
> onload = function() {
>   var frame = document.getElementById("frame");
>   var img = frame.contentDocument.createElement("img");
>   img.src = "cat.jpg";
>   img.onload = function() {
>     frame.parentNode.removeChild(frame);
>     img.decode().then(function() {});
>   }
> };
> </script>
> 
> At the point of HTMLImageElement::RequestDecode the frame is nullptr. However,
although we call reject on the resolver, js doesn't see any of that... I'm not
quite sure how to test this automatically. domenic@ do you have any suggestions?

I'm a little lost. What js-observable behavior are we trying to test? Is
"calling reject on the resolver" the same as calling reject on the promise? In
that case you should be able to use the second argument to then() to catch
exceptions. In web platform tests you can use promise_rejects.

[vmpstr, 2017-04-17 23:31:20]

On 2017/04/17 at 22:09:27, domenic wrote:
> On 2017/04/17 at 21:59:23, vmpstr wrote:
> > On 2017/04/17 at 21:01:37, dcheng wrote:
> > > On 2017/04/17 20:51:25, vmpstr wrote:
> > > > > I think this is going to be problematic for OOPIF, where it's possible
for
> > > > same-origin documents to not share the same compositor. If we have a
frame tree
> > > > like:
> > > > > 
> > > > > 
> > > > >    A (main)
> > > > >     \
> > > > >      B (child)
> > > > >       \
> > > > >        A (grandchild)
> > > > > 
> > > > > 
> > > > > The two frames on A are same-origin but will have different
compositors in
> > > > OOPIF. What would be involved in making it work across different
compositors?
> > > > (+kenrb)
> > > > 
> > > > I talked to Chris (+chrihstr) about this and although the situation is
possible,
> > > > it's unclear whether it's possible to get an image that is decoded on
main
> > > > attached to grandchild (or vice versa). Do you know how that would be
possible?
> > > 
> > > Actually this isn't OOPIF specific -- origin A could call window.open(),
which creates a new window with origin A. When the promise succeeds, we could
call appendChild() on any same-origin document: nothing prevents an image
element created in one document from being adopted by another document. As long
as we handle this case correctly (which from below it sounds like we probably
will, as it's just a cache miss), then that's fine.
> > > 
> > > > 
> > > > We don't really have any plans on making compositor specific caches be
> > > > transferable to other compositors. As somewhat of an aside, the decode
can
> > > > succeed on one compositor and a different compositor can attach the
exact same
> > > > url image to its content, which will just be a cache miss in the
compositor.
> > > > This means that if there are subtle situations where it's possible, the
only
> > > > downside is that the performance benefit provided by the decode api
would be
> > > > eliminated (ie, things shouldn't "break")
> > > > 
> > > >
https://codereview.chromium.org/2769823002/diff/280001/third_party/WebKit/Sou...
> > > > File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):
> > > > 
> > > >
https://codereview.chromium.org/2769823002/diff/280001/third_party/WebKit/Sou...
> > > > third_party/WebKit/Source/core/html/HTMLImageElement.cpp:140:
LocalFrame* frame
> > > > = GetDocument().GetFrame();
> > > > On 2017/04/17 at 18:48:02, dcheng wrote:
> > > > > Hmm. I think we might need to null check here.
> > > > > 
> > > > > I think JS could remove the iframe it's running in, and then request
decode.
> > > > In that case, I think |frame| will be null here.
> > > > 
> > > > Done.
> > > 
> > >
https://codereview.chromium.org/2769823002/diff/300001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):
> > > 
> > >
https://codereview.chromium.org/2769823002/diff/300001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/html/HTMLImageElement.cpp:138:
!GetImageLoader().GetImage()->GetImage() || !frame) {
> > > It would be nice to have coverage for the !frame case as well.
> > 
> > I've verified that it works locally via:
> > 
> > <!doctype HTML>
> > 
> > <iframe id="frame" srcdoc="iframe"></iframe>
> > <div id="container"></div>
> > <script>
> > onload = function() {
> >   var frame = document.getElementById("frame");
> >   var img = frame.contentDocument.createElement("img");
> >   img.src = "cat.jpg";
> >   img.onload = function() {
> >     frame.parentNode.removeChild(frame);
> >     img.decode().then(function() {});
> >   }
> > };
> > </script>
> > 
> > At the point of HTMLImageElement::RequestDecode the frame is nullptr.
However, although we call reject on the resolver, js doesn't see any of that...
I'm not quite sure how to test this automatically. domenic@ do you have any
suggestions?
> 
> I'm a little lost. What js-observable behavior are we trying to test? Is
"calling reject on the resolver" the same as calling reject on the promise? In
that case you should be able to use the second argument to then() to catch
exceptions. In web platform tests you can use promise_rejects.

Hm, I guess there's nothing that's js observable other than Blink doesn't crash
heh.

[dcheng, 2017-04-17 23:42:19]

On 2017/04/17 23:31:20, vmpstr wrote:
> On 2017/04/17 at 22:09:27, domenic wrote:
> > On 2017/04/17 at 21:59:23, vmpstr wrote:
> > > On 2017/04/17 at 21:01:37, dcheng wrote:
> > > > On 2017/04/17 20:51:25, vmpstr wrote:
> > > > > > I think this is going to be problematic for OOPIF, where it's
possible
> for
> > > > > same-origin documents to not share the same compositor. If we have a
> frame tree
> > > > > like:
> > > > > > 
> > > > > > 
> > > > > >    A (main)
> > > > > >     \
> > > > > >      B (child)
> > > > > >       \
> > > > > >        A (grandchild)
> > > > > > 
> > > > > > 
> > > > > > The two frames on A are same-origin but will have different
> compositors in
> > > > > OOPIF. What would be involved in making it work across different
> compositors?
> > > > > (+kenrb)
> > > > > 
> > > > > I talked to Chris (+chrihstr) about this and although the situation is
> possible,
> > > > > it's unclear whether it's possible to get an image that is decoded on
> main
> > > > > attached to grandchild (or vice versa). Do you know how that would be
> possible?
> > > > 
> > > > Actually this isn't OOPIF specific -- origin A could call window.open(),
> which creates a new window with origin A. When the promise succeeds, we could
> call appendChild() on any same-origin document: nothing prevents an image
> element created in one document from being adopted by another document. As
long
> as we handle this case correctly (which from below it sounds like we probably
> will, as it's just a cache miss), then that's fine.
> > > > 
> > > > > 
> > > > > We don't really have any plans on making compositor specific caches be
> > > > > transferable to other compositors. As somewhat of an aside, the decode
> can
> > > > > succeed on one compositor and a different compositor can attach the
> exact same
> > > > > url image to its content, which will just be a cache miss in the
> compositor.
> > > > > This means that if there are subtle situations where it's possible,
the
> only
> > > > > downside is that the performance benefit provided by the decode api
> would be
> > > > > eliminated (ie, things shouldn't "break")
> > > > > 
> > > > >
>
https://codereview.chromium.org/2769823002/diff/280001/third_party/WebKit/Sou...
> > > > > File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):
> > > > > 
> > > > >
>
https://codereview.chromium.org/2769823002/diff/280001/third_party/WebKit/Sou...
> > > > > third_party/WebKit/Source/core/html/HTMLImageElement.cpp:140:
> LocalFrame* frame
> > > > > = GetDocument().GetFrame();
> > > > > On 2017/04/17 at 18:48:02, dcheng wrote:
> > > > > > Hmm. I think we might need to null check here.
> > > > > > 
> > > > > > I think JS could remove the iframe it's running in, and then request
> decode.
> > > > > In that case, I think |frame| will be null here.
> > > > > 
> > > > > Done.
> > > > 
> > > >
>
https://codereview.chromium.org/2769823002/diff/300001/third_party/WebKit/Sou...
> > > > File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):
> > > > 
> > > >
>
https://codereview.chromium.org/2769823002/diff/300001/third_party/WebKit/Sou...
> > > > third_party/WebKit/Source/core/html/HTMLImageElement.cpp:138:
> !GetImageLoader().GetImage()->GetImage() || !frame) {
> > > > It would be nice to have coverage for the !frame case as well.
> > > 
> > > I've verified that it works locally via:
> > > 
> > > <!doctype HTML>
> > > 
> > > <iframe id="frame" srcdoc="iframe"></iframe>
> > > <div id="container"></div>
> > > <script>
> > > onload = function() {
> > >   var frame = document.getElementById("frame");
> > >   var img = frame.contentDocument.createElement("img");
> > >   img.src = "cat.jpg";
> > >   img.onload = function() {
> > >     frame.parentNode.removeChild(frame);
> > >     img.decode().then(function() {});
> > >   }
> > > };
> > > </script>
> > > 
> > > At the point of HTMLImageElement::RequestDecode the frame is nullptr.
> However, although we call reject on the resolver, js doesn't see any of
that...
> I'm not quite sure how to test this automatically. domenic@ do you have any
> suggestions?
> > 
> > I'm a little lost. What js-observable behavior are we trying to test? Is
> "calling reject on the resolver" the same as calling reject on the promise? In
> that case you should be able to use the second argument to then() to catch
> exceptions. In web platform tests you can use promise_rejects.
> 
> Hm, I guess there's nothing that's js observable other than Blink doesn't
crash
> heh.

We should still reject the promise, right?

[vmpstr, 2017-04-17 23:45:18]

On 2017/04/17 at 23:42:19, dcheng wrote:
> On 2017/04/17 23:31:20, vmpstr wrote:
> > On 2017/04/17 at 22:09:27, domenic wrote:
> > > On 2017/04/17 at 21:59:23, vmpstr wrote:
> > > > On 2017/04/17 at 21:01:37, dcheng wrote:
> > > > > On 2017/04/17 20:51:25, vmpstr wrote:
> > > > > > > I think this is going to be problematic for OOPIF, where it's
possible
> > for
> > > > > > same-origin documents to not share the same compositor. If we have a
> > frame tree
> > > > > > like:
> > > > > > > 
> > > > > > > 
> > > > > > >    A (main)
> > > > > > >     \
> > > > > > >      B (child)
> > > > > > >       \
> > > > > > >        A (grandchild)
> > > > > > > 
> > > > > > > 
> > > > > > > The two frames on A are same-origin but will have different
> > compositors in
> > > > > > OOPIF. What would be involved in making it work across different
> > compositors?
> > > > > > (+kenrb)
> > > > > > 
> > > > > > I talked to Chris (+chrihstr) about this and although the situation
is
> > possible,
> > > > > > it's unclear whether it's possible to get an image that is decoded
on
> > main
> > > > > > attached to grandchild (or vice versa). Do you know how that would
be
> > possible?
> > > > > 
> > > > > Actually this isn't OOPIF specific -- origin A could call
window.open(),
> > which creates a new window with origin A. When the promise succeeds, we
could
> > call appendChild() on any same-origin document: nothing prevents an image
> > element created in one document from being adopted by another document. As
long
> > as we handle this case correctly (which from below it sounds like we
probably
> > will, as it's just a cache miss), then that's fine.
> > > > > 
> > > > > > 
> > > > > > We don't really have any plans on making compositor specific caches
be
> > > > > > transferable to other compositors. As somewhat of an aside, the
decode
> > can
> > > > > > succeed on one compositor and a different compositor can attach the
> > exact same
> > > > > > url image to its content, which will just be a cache miss in the
> > compositor.
> > > > > > This means that if there are subtle situations where it's possible,
the
> > only
> > > > > > downside is that the performance benefit provided by the decode api
> > would be
> > > > > > eliminated (ie, things shouldn't "break")
> > > > > > 
> > > > > >
> >
https://codereview.chromium.org/2769823002/diff/280001/third_party/WebKit/Sou...
> > > > > > File third_party/WebKit/Source/core/html/HTMLImageElement.cpp
(right):
> > > > > > 
> > > > > >
> >
https://codereview.chromium.org/2769823002/diff/280001/third_party/WebKit/Sou...
> > > > > > third_party/WebKit/Source/core/html/HTMLImageElement.cpp:140:
> > LocalFrame* frame
> > > > > > = GetDocument().GetFrame();
> > > > > > On 2017/04/17 at 18:48:02, dcheng wrote:
> > > > > > > Hmm. I think we might need to null check here.
> > > > > > > 
> > > > > > > I think JS could remove the iframe it's running in, and then
request
> > decode.
> > > > > > In that case, I think |frame| will be null here.
> > > > > > 
> > > > > > Done.
> > > > > 
> > > > >
> >
https://codereview.chromium.org/2769823002/diff/300001/third_party/WebKit/Sou...
> > > > > File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):
> > > > > 
> > > > >
> >
https://codereview.chromium.org/2769823002/diff/300001/third_party/WebKit/Sou...
> > > > > third_party/WebKit/Source/core/html/HTMLImageElement.cpp:138:
> > !GetImageLoader().GetImage()->GetImage() || !frame) {
> > > > > It would be nice to have coverage for the !frame case as well.
> > > > 
> > > > I've verified that it works locally via:
> > > > 
> > > > <!doctype HTML>
> > > > 
> > > > <iframe id="frame" srcdoc="iframe"></iframe>
> > > > <div id="container"></div>
> > > > <script>
> > > > onload = function() {
> > > >   var frame = document.getElementById("frame");
> > > >   var img = frame.contentDocument.createElement("img");
> > > >   img.src = "cat.jpg";
> > > >   img.onload = function() {
> > > >     frame.parentNode.removeChild(frame);
> > > >     img.decode().then(function() {});
> > > >   }
> > > > };
> > > > </script>
> > > > 
> > > > At the point of HTMLImageElement::RequestDecode the frame is nullptr.
> > However, although we call reject on the resolver, js doesn't see any of
that...
> > I'm not quite sure how to test this automatically. domenic@ do you have any
> > suggestions?
> > > 
> > > I'm a little lost. What js-observable behavior are we trying to test? Is
> > "calling reject on the resolver" the same as calling reject on the promise?
In
> > that case you should be able to use the second argument to then() to catch
> > exceptions. In web platform tests you can use promise_rejects.
> > 
> > Hm, I guess there's nothing that's js observable other than Blink doesn't
crash
> > heh.
> 
> We should still reject the promise, right?

Yeah, I mean the intent is that the promise gets rejected, but with the html I
have, I verified (in C++) that reject gets called, but then js doesn't seem to
receive that since I think the img itself gets deleted? I'll try to write up a
test where I can get consistent behavior. 

domenic@, one thing is that I'm calling decode in an onload handler; I'm not
sure what to return in this case.

[domenic, 2017-04-18 03:41:31]

On 2017/04/17 at 23:45:18, vmpstr wrote:
> On 2017/04/17 at 23:42:19, dcheng wrote:
> > We should still reject the promise, right?
> 
> Yeah, I mean the intent is that the promise gets rejected, but with the html I
have, I verified (in C++) that reject gets called, but then js doesn't seem to
receive that since I think the img itself gets deleted? I'll try to write up a
test where I can get consistent behavior. 
> 
> domenic@, one thing is that I'm calling decode in an onload handler; I'm not
sure what to return in this case.

This does indeed seem important to test.

To test combined event/promise handling you can use EventWatcher, or new
Promise, in combination with promise_test. Examples:

-
https://github.com/w3c/web-platform-tests/blob/f80f77270a45c742944862d51caef7...
-
https://github.com/w3c/web-platform-tests/blob/6d79a36167835f5ca57de5d721d2c4...

[vmpstr, 2017-04-18 17:31:03]

Hey, I added a few more tests and code in ::decode function to throw an
exception if the script state isn't valid.

However, the last test I added times out. Basically, when we are in ::decode
function, the script state is valid and we generate a promise. Then, while we're
working on satisfying the promise, the script state becomes invalid, so I don't
actually get the load event, so the decode doesn't actually start. I'm also not
sure if rejecting the resolver would actually have any effect since the
execution context is gone already (ScriptPromiseResolver just silently doesn't
do anything in these situations).

I'm not sure if there's an event I can hook into to ensure to reject the promise
before the document is shutdown? Or since the load event also seems to not
trigger, do you think it's a reasonable behavior to not do anything with the
promise once the document is destroyed?

https://codereview.chromium.org/2769823002/diff/320001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/external/wpt/image-decodes/image-decode.html
(right):

https://codereview.chromium.org/2769823002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/image-decodes/image-decode.html:220:
promise_test(function(t) {
This test times out.

[vmpstr, 2017-04-18 18:40:35]

On 2017/04/18 at 17:31:03, vmpstr wrote:
> Hey, I added a few more tests and code in ::decode function to throw an
exception if the script state isn't valid.
> 
> However, the last test I added times out. Basically, when we are in ::decode
function, the script state is valid and we generate a promise. Then, while we're
working on satisfying the promise, the script state becomes invalid, so I don't
actually get the load event, so the decode doesn't actually start. I'm also not
sure if rejecting the resolver would actually have any effect since the
execution context is gone already (ScriptPromiseResolver just silently doesn't
do anything in these situations).
> 
> I'm not sure if there's an event I can hook into to ensure to reject the
promise before the document is shutdown? Or since the load event also seems to
not trigger, do you think it's a reasonable behavior to not do anything with the
promise once the document is destroyed?
> 
>
https://codereview.chromium.org/2769823002/diff/320001/third_party/WebKit/Lay...
> File
third_party/WebKit/LayoutTests/external/wpt/image-decodes/image-decode.html
(right):
> 
>
https://codereview.chromium.org/2769823002/diff/320001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/external/wpt/image-decodes/image-decode.html:220:
promise_test(function(t) {
> This test times out.

FWIW, I also tried a similar test with an "audio" tag which has a promise
returning play() function and got the same results: it rejects with some error
(not supported type) if we keep the iframe in the document, but times out if we
remove the iframe after issuing the play() call.

[dcheng, 2017-04-18 19:02:48]

Blink LGTM

It would be kind of nice to be able to test this, but it seems hard.

(I've CCed yhirano@ for any additional insights; I think some code explicitly
rejects promises when the context is destroyed, but here the context is
*already* destroyed. We can investigate any changes here as followups though, I
think)

[domenic, 2017-04-18 20:46:54]

On 2017/04/18 at 18:40:35, vmpstr wrote:
> On 2017/04/18 at 17:31:03, vmpstr wrote:
> > Hey, I added a few more tests and code in ::decode function to throw an
exception if the script state isn't valid.
> > 
> > However, the last test I added times out. Basically, when we are in ::decode
function, the script state is valid and we generate a promise. Then, while we're
working on satisfying the promise, the script state becomes invalid, so I don't
actually get the load event, so the decode doesn't actually start. I'm also not
sure if rejecting the resolver would actually have any effect since the
execution context is gone already (ScriptPromiseResolver just silently doesn't
do anything in these situations).
> > 
> > I'm not sure if there's an event I can hook into to ensure to reject the
promise before the document is shutdown? Or since the load event also seems to
not trigger, do you think it's a reasonable behavior to not do anything with the
promise once the document is destroyed?
> > 
> >
https://codereview.chromium.org/2769823002/diff/320001/third_party/WebKit/Lay...
> > File
third_party/WebKit/LayoutTests/external/wpt/image-decodes/image-decode.html
(right):
> > 
> >
https://codereview.chromium.org/2769823002/diff/320001/third_party/WebKit/Lay...
> >
third_party/WebKit/LayoutTests/external/wpt/image-decodes/image-decode.html:220:
promise_test(function(t) {
> > This test times out.
> 
> FWIW, I also tried a similar test with an "audio" tag which has a promise
returning play() function and got the same results: it rejects with some error
(not supported type) if we keep the iframe in the document, but times out if we
remove the iframe after issuing the play() call.

The spec ecosystem here is pretty bad, but we've also gotten repeated complaints
from Gecko engineers about not speccing and implementing this correctly; they
would much prefer we clean up such promises. I think we should do our best to
reject when the document unloads to be good citizens here. For example, if the
context is already destroyed we can immediately reject.

The fact that you were able to write tests that fail (timeout) is promising. Now
we just need to make them pass :).

The spec has https://html.spec.whatwg.org/#unloading-document-cleanup-steps
which we should maybe be adding to.

We should file a separate bug on play().

[domenic, 2017-04-18 20:48:34]

https://codereview.chromium.org/2769823002/diff/320001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/external/wpt/image-decodes/image-decode.html
(right):

https://codereview.chromium.org/2769823002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/image-decodes/image-decode.html:225:
var promise = img.decode();
Maybe what we need here is a spec line saying "If the img's node document is not
the active document, return a rejected promise".

[vmpstr, 2017-05-11 17:30:03]

I've filed crbug.com/721435 to track the promise destruction issue. Since it's a
pretty uncommon case, I'd like to try and land this patch so that we can start
flushing out any other bugs that might creep up. Does that sound reasonable? If
so, I will remove the test that is currently failing and add it back once we
come up with a promise destruction resolution.

[domenic, 2017-05-11 17:40:50]

On 2017/05/11 at 17:30:03, vmpstr wrote:
> I've filed crbug.com/721435 to track the promise destruction issue. Since it's
a pretty uncommon case, I'd like to try and land this patch so that we can start
flushing out any other bugs that might creep up. Does that sound reasonable? If
so, I will remove the test that is currently failing and add it back once we
come up with a promise destruction resolution.

I think it would be better to keep the test but with a failing test expectation,
since we're trying to create test coverage for the spec in order to land the
spec.

[vmpstr, 2017-05-11 17:56:48]

On 2017/05/11 17:40:50, domenic wrote:
> On 2017/05/11 at 17:30:03, vmpstr wrote:
> > I've filed crbug.com/721435 to track the promise destruction issue. Since
it's
> a pretty uncommon case, I'd like to try and land this patch so that we can
start
> flushing out any other bugs that might creep up. Does that sound reasonable?
If
> so, I will remove the test that is currently failing and add it back once we
> come up with a promise destruction resolution.
> 
> I think it would be better to keep the test but with a failing test
expectation,
> since we're trying to create test coverage for the spec in order to land the
> spec.

Makes sense, I'll do that.

[vmpstr, 2017-05-11 20:33:56]

vmpstr@chromium.org changed reviewers:
+ chrishtr@chromium.org

[vmpstr, 2017-05-11 20:33:57]

I've split the test into several files and marked the iframe ones as skip
pending the bug resolution.

domenic@ could you please take a look?
also +chrishtr@ for review.

[domenic, 2017-05-15 21:43:23]

On 2017/05/11 at 20:33:57, vmpstr wrote:
> I've split the test into several files and marked the iframe ones as skip
pending the bug resolution.
> 
> domenic@ could you please take a look?
> also +chrishtr@ for review.

Tests look good!

[vmpstr, 2017-05-17 18:54:43]

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

[vmpstr, 2017-05-17 18:54:45]

+pdr as well, the more eyes the better. Please take a look.

[pdr., 2017-05-18 03:20:10]

I think this looks great.

https://codereview.chromium.org/2769823002/diff/340001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/2769823002/diff/340001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:1078:
layer_tree_host_->QueueImageDecode(std::move(image), callback);
If javascript calls decode() twice on a loaded image, does that kick off two
independent decodes?

https://codereview.chromium.org/2769823002/diff/340001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/image-decodes/image-decode-path-changes.html
(right):

https://codereview.chromium.org/2769823002/diff/340001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/image-decodes/image-decode-path-changes.html:1:
<!DOCTYPE html>
Do we have any tests that cover the case where the image src changes while a
decode is in-flight?

I'm not sure how possible it is to write such a test as a layouttest because the
compositor is sort of synchronous.

https://codereview.chromium.org/2769823002/diff/340001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/external/wpt/image-decodes/image-decode.html
(right):

https://codereview.chromium.org/2769823002/diff/340001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/image-decodes/image-decode.html:15:
// -------------------
Can you add a test with a png file that is corrupt? I think you might be able to
do this using a data:uri such as:
img.src = "src="data:image/png;base64,iVBO00PDR0BADBEEF00KGg"

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

https://codereview.chromium.org/2769823002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:144: frame,
image->ImageForCurrentFrame(),
Is this the image at its native resolution or does it depend on the current
size?

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

https://codereview.chromium.org/2769823002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.h:194: void
DidDecode(uint32_t sequence_id, bool success);
Can you add some short comments here and above RequestDecode?

Bikeshed: DecodeRequestFinished?

https://codereview.chromium.org/2769823002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/2769823002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:493: if
(isHTMLImageElement(GetElement())) {
Nit: put this in HTMLImageLoader::ImageNotifyFinished.

[vmpstr, 2017-05-18 18:51:13]

Thanks for the review! PTAL

https://codereview.chromium.org/2769823002/diff/340001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/2769823002/diff/340001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:1078:
layer_tree_host_->QueueImageDecode(std::move(image), callback);
On 2017/05/18 03:20:10, pdr. wrote:
> If javascript calls decode() twice on a loaded image, does that kick off two
> independent decodes?

It issues two different promises and makes two different calls here, yes.
However, internally it will only do the decode if necessary. If it was already
locked by the previous decode, it will just succeed without doing work.

https://codereview.chromium.org/2769823002/diff/340001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/image-decodes/image-decode-path-changes.html
(right):

https://codereview.chromium.org/2769823002/diff/340001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/image-decodes/image-decode-path-changes.html:1:
<!DOCTYPE html>
On 2017/05/18 03:20:10, pdr. wrote:
> Do we have any tests that cover the case where the image src changes while a
> decode is in-flight?
> 
> I'm not sure how possible it is to write such a test as a layouttest because
the
> compositor is sort of synchronous.

Yeah the tests here on line 24, 37 kind of test the src changes while things are
happening. Did you have some other type of test in mind?

https://codereview.chromium.org/2769823002/diff/340001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/external/wpt/image-decodes/image-decode.html
(right):

https://codereview.chromium.org/2769823002/diff/340001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/image-decodes/image-decode.html:15:
// -------------------
On 2017/05/18 03:20:10, pdr. wrote:
> Can you add a test with a png file that is corrupt? I think you might be able
to
> do this using a data:uri such as:
> img.src = "src="data:image/png;base64,iVBO00PDR0BADBEEF00KGg"

Good idea! Added that and one with a valid data:image/png as well.

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

https://codereview.chromium.org/2769823002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:144: frame,
image->ImageForCurrentFrame(),
On 2017/05/18 03:20:10, pdr. wrote:
> Is this the image at its native resolution or does it depend on the current
> size?

This should be native resolution

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

https://codereview.chromium.org/2769823002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.h:194: void
DidDecode(uint32_t sequence_id, bool success);
On 2017/05/18 03:20:10, pdr. wrote:
> Can you add some short comments here and above RequestDecode?
> 
> Bikeshed: DecodeRequestFinished?

Done.

https://codereview.chromium.org/2769823002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/2769823002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:493: if
(isHTMLImageElement(GetElement())) {
On 2017/05/18 03:20:10, pdr. wrote:
> Nit: put this in HTMLImageLoader::ImageNotifyFinished.

Done.

[pdr., 2017-05-18 20:35:10]

https://codereview.chromium.org/2769823002/diff/340001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/image-decodes/image-decode-path-changes.html
(right):

https://codereview.chromium.org/2769823002/diff/340001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/image-decodes/image-decode-path-changes.html:1:
<!DOCTYPE html>
On 2017/05/18 at 18:51:12, vmpstr wrote:
> On 2017/05/18 03:20:10, pdr. wrote:
> > Do we have any tests that cover the case where the image src changes while a
> > decode is in-flight?
> > 
> > I'm not sure how possible it is to write such a test as a layouttest because
the
> > compositor is sort of synchronous.
> 
> Yeah the tests here on line 24, 37 kind of test the src changes while things
are happening. Did you have some other type of test in mind?

These tests don't really handle the case when a decode is in-flight because the
images aren't yet loaded when the src changes.

[pdr., 2017-05-18 21:45:27]

On 2017/05/18 at 20:35:10, pdr. wrote:
>
https://codereview.chromium.org/2769823002/diff/340001/third_party/WebKit/Lay...
> File
third_party/WebKit/LayoutTests/external/wpt/image-decodes/image-decode-path-changes.html
(right):
> 
>
https://codereview.chromium.org/2769823002/diff/340001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/external/wpt/image-decodes/image-decode-path-changes.html:1:
<!DOCTYPE html>
> On 2017/05/18 at 18:51:12, vmpstr wrote:
> > On 2017/05/18 03:20:10, pdr. wrote:
> > > Do we have any tests that cover the case where the image src changes while
a
> > > decode is in-flight?
> > > 
> > > I'm not sure how possible it is to write such a test as a layouttest
because the
> > > compositor is sort of synchronous.
> > 
> > Yeah the tests here on line 24, 37 kind of test the src changes while things
are happening. Did you have some other type of test in mind?
> 
> These tests don't really handle the case when a decode is in-flight because
the images aren't yet loaded when the src changes.

We discussed this offline a bit. We don't have a great way to write full-stack
integration tests, nor do we really need one in this case because we can fully
unittest both sides. Please add a unittest of the non-single-thread side of this
if possible.


LGTM overall, this is a great patch.

[vmpstr, 2017-05-23 21:08:39]

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

[commit-bot: I haz the power, 2017-05-23 21:09:32]

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-05-23 22:58:25]

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

[commit-bot: I haz the power, 2017-05-23 22:58:26]

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_...)

[vmpstr, 2017-05-31 17:50:14]

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

[commit-bot: I haz the power, 2017-05-31 17:50:41]

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-05-31 20:44:35]

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

[commit-bot: I haz the power, 2017-05-31 20:44:36]

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_...)

[vmpstr, 2017-06-01 17:27:35]

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

[commit-bot: I haz the power, 2017-06-01 17:27:56]

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-06-01 17:31:11]

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

[commit-bot: I haz the power, 2017-06-01 17:31:12]

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

[vmpstr, 2017-06-01 17:50:22]

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

[commit-bot: I haz the power, 2017-06-01 17:50:41]

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-06-01 19:51:48]

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

[commit-bot: I haz the power, 2017-06-01 19:51:50]

Dry run: This issue passed the CQ dry run.

[vmpstr, 2017-06-01 21:43:06]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2017-06-01 21:43:07]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org,
dcheng@chromium.org, pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2769823002/#ps460001
(title: "rebase")

[commit-bot: I haz the power, 2017-06-01 21:43:56]

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-06-01 21:50:43]

CQ is committing da patch.

Bot data: {"patchset_id": 460001, "attempt_start_ts": 1496353386508340,
"parent_rev": "2011b27fb8555b25ecc46b651b022b54a85d54a9", "commit_rev":
"a840c6a20c62e5ad20ec4e3419370173566b2850"}

[commit-bot: I haz the power, 2017-06-01 21:50:55]

Description was changed from

==========
Add decode() functionality to image elements.

This allows developers to request a decode of an img element.
When the promise resolves, the compositor will keep the image
locked for two frames, ensuring that if appended into the DOM,
jank is minimized.

Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/LMV9dBG_1-I


CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Add decode() functionality to image elements.

This allows developers to request a decode of an img element.
When the promise resolves, the compositor will keep the image
locked for two frames, ensuring that if appended into the DOM,
jank is minimized.

Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/LMV9dBG_1-I


CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2769823002
Cr-Commit-Position: refs/heads/master@{#476445}
Committed:
https://chromium.googlesource.com/chromium/src/+/a840c6a20c62e5ad20ec4e341937...
==========

[commit-bot: I haz the power, 2017-06-01 21:50:57]

Committed patchset #24 (id:460001) as
https://chromium.googlesource.com/chromium/src/+/a840c6a20c62e5ad20ec4e341937...