Implement Image::maybeAnimated for SVGImage

Implement Image::maybeAnimated for SVGImage

This CL renames SVGImage::hasAnimations to maybeAnimated, overriding
the implementation from the base class (Image.) The old method matches
the new one in certainty of the reply, and allows SVGImages to be paused
when being scrolled out of view etc.

BUG=706152
Review-Url: https://codereview.chromium.org/2783133002
Cr-Commit-Position: refs/heads/master@{#461094}
Committed: https://chromium.googlesource.com/chromium/src/+/bb03bc3127de97898e62323b12cd2997267cfd5a

[fs, 2017-03-30 09:04:56]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 09:05:14]

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-30 09:23:12]

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

[commit-bot: I haz the power, 2017-03-30 09:23:12]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fs, 2017-03-30 09:31:50]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 09:32:09]

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

[fs, 2017-03-30 10:42:05]

fs@opera.com changed reviewers:
+ chrishtr@chromium.org

[commit-bot: I haz the power, 2017-03-30 10:51:09]

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

[commit-bot: I haz the power, 2017-03-30 10:51:10]

Dry run: This issue passed the CQ dry run.

[fs, 2017-03-30 12:16:13]

(Seemed to roughly halve the CPU usage on www.yahoo.com/news/ in a quick and
unscientific test.)

[kouhei (in TOK), 2017-03-30 13:28:26]

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

[kouhei (in TOK), 2017-03-30 13:28:27]

drive-by lgtm, but please wait for chrishtr

> (Seemed to roughly halve the CPU usage on www.yahoo.com/news/ in a quick and
unscientific test.)
O_o. Amazed by the result.

https://codereview.chromium.org/2783133002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/paint/invalidation/svg/animated-svg-as-image-transformed-offscreen.html
(right):

https://codereview.chromium.org/2783133002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/invalidation/svg/animated-svg-as-image-transformed-offscreen.html:7:
// offscreen gif animation is intentional.
SVG

[fs, 2017-03-30 14:34:37]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 14:34:58]

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

[fs, 2017-03-30 14:37:51]

> > (Seemed to roughly halve the CPU usage on www.yahoo.com/news/ in a quick and
unscientific test.)
> O_o. Amazed by the result.
> 

I'm not sure we can trust the numbers too much (the CPU usage fluctuates quite a
bit on this site so I could've sampled a bad time interval. The tests should be
more reliable indicators.)

https://codereview.chromium.org/2783133002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/paint/invalidation/svg/animated-svg-as-image-transformed-offscreen.html
(right):

https://codereview.chromium.org/2783133002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/invalidation/svg/animated-svg-as-image-transformed-offscreen.html:7:
// offscreen gif animation is intentional.
On 2017/03/30 at 13:28:27, kouhei wrote:
> SVG

Doh! C'n'P spot missed, thanks for catching. Fixed.

[chrishtr, 2017-03-30 15:42:35]

lgtm

Nice!

https://codereview.chromium.org/2783133002/diff/30011/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/paint/invalidation/svg/animated-svg-as-image-background-offscreen.html
(right):

https://codereview.chromium.org/2783133002/diff/30011/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/invalidation/svg/animated-svg-as-image-background-offscreen.html:26:
targetImage.style.background =
"url(../../../svg/as-image/resources/animated-rect-fixed-size-2.svg)";
Does this sufficiently test the case of an animated LayoutBox background?
I guess image backgrounds go through the LayoutBox code for invalidation?

https://codereview.chromium.org/2783133002/diff/30011/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/invalidation/svg/animated-svg-as-image-background-offscreen.html:29:
// we'll get a frame.
onload events don't work for SVG Images? Weird. Why is that? Is it something
that should be fixed in the platform?

I googled around and found this hack:

http://stackoverflow.com/questions/11390830/is-it-possible-to-listen-image-lo...

Does it work for this test?

[Stephen Chennney, 2017-03-30 15:55:12]

On 2017/03/30 15:42:35, chrishtr wrote:
> lgtm
> 
> Nice!
> 
>
https://codereview.chromium.org/2783133002/diff/30011/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/paint/invalidation/svg/animated-svg-as-image-background-offscreen.html
> (right):
> 
>
https://codereview.chromium.org/2783133002/diff/30011/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/paint/invalidation/svg/animated-svg-as-image-background-offscreen.html:26:
> targetImage.style.background =
> "url(../../../svg/as-image/resources/animated-rect-fixed-size-2.svg)";
> Does this sufficiently test the case of an animated LayoutBox background?
> I guess image backgrounds go through the LayoutBox code for invalidation?
> 
>
https://codereview.chromium.org/2783133002/diff/30011/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/paint/invalidation/svg/animated-svg-as-image-background-offscreen.html:29:
> // we'll get a frame.
> onload events don't work for SVG Images? Weird. Why is that? Is it something
> that should be fixed in the platform?
> 
> I googled around and found this hack:
> 
>
http://stackoverflow.com/questions/11390830/is-it-possible-to-listen-image-lo...
> 
> Does it work for this test?

There's an open bug about onload and SVG image. The issue is that it does not
check if the sub-resources have loaded, so you can get onload before the SVG is
ready to manipulate.
https://bugs.chromium.org/p/chromium/issues/detail?id=382170

[chrishtr, 2017-03-30 16:04:35]

On 2017/03/30 at 15:55:12, schenney wrote:
> On 2017/03/30 15:42:35, chrishtr wrote:
> > lgtm
> > 
> > Nice!
> > 
> >
https://codereview.chromium.org/2783133002/diff/30011/third_party/WebKit/Layo...
> > File
> >
third_party/WebKit/LayoutTests/paint/invalidation/svg/animated-svg-as-image-background-offscreen.html
> > (right):
> > 
> >
https://codereview.chromium.org/2783133002/diff/30011/third_party/WebKit/Layo...
> >
third_party/WebKit/LayoutTests/paint/invalidation/svg/animated-svg-as-image-background-offscreen.html:26:
> > targetImage.style.background =
> > "url(../../../svg/as-image/resources/animated-rect-fixed-size-2.svg)";
> > Does this sufficiently test the case of an animated LayoutBox background?
> > I guess image backgrounds go through the LayoutBox code for invalidation?
> > 
> >
https://codereview.chromium.org/2783133002/diff/30011/third_party/WebKit/Layo...
> >
third_party/WebKit/LayoutTests/paint/invalidation/svg/animated-svg-as-image-background-offscreen.html:29:
> > // we'll get a frame.
> > onload events don't work for SVG Images? Weird. Why is that? Is it something
> > that should be fixed in the platform?
> > 
> > I googled around and found this hack:
> > 
> >
http://stackoverflow.com/questions/11390830/is-it-possible-to-listen-image-lo...
> > 
> > Does it work for this test?
> 
> There's an open bug about onload and SVG image. The issue is that it does not
check if the sub-resources have loaded, so you can get onload before the SVG is
ready to manipulate.
https://bugs.chromium.org/p/chromium/issues/detail?id=382170

Oh interesting! Thanks for the pointer.

[commit-bot: I haz the power, 2017-03-30 17:24:51]

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

[commit-bot: I haz the power, 2017-03-30 17:24:52]

Dry run: This issue passed the CQ dry run.

[pdr., 2017-03-30 19:09:54]

LGTM thank you!

[fs, 2017-03-31 10:32:45]

fs@opera.com changed reviewers:
+ pdr@chromium.org

[fs, 2017-03-31 10:32:45]

The CQ bit was checked by fs@opera.com

[fs, 2017-03-31 10:32:46]

https://codereview.chromium.org/2783133002/diff/30011/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/paint/invalidation/svg/animated-svg-as-image-background-offscreen.html
(right):

https://codereview.chromium.org/2783133002/diff/30011/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/invalidation/svg/animated-svg-as-image-background-offscreen.html:26:
targetImage.style.background =
"url(../../../svg/as-image/resources/animated-rect-fixed-size-2.svg)";
On 2017/03/30 at 15:42:35, chrishtr wrote:
> Does this sufficiently test the case of an animated LayoutBox background?
> I guess image backgrounds go through the LayoutBox code for invalidation?

Yes, this should trigger the code in LayoutBox::imageChanged.

https://codereview.chromium.org/2783133002/diff/30011/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/invalidation/svg/animated-svg-as-image-background-offscreen.html:29:
// we'll get a frame.
On 2017/03/30 at 15:42:35, chrishtr wrote:
> onload events don't work for SVG Images? Weird. Why is that? Is it something
> that should be fixed in the platform?

They do, but for background images you won't get a notification. (I guess
latching onto document 'load' could work...)
 
> I googled around and found this hack:
> 
>
http://stackoverflow.com/questions/11390830/is-it-possible-to-listen-image-lo...
> 
> Does it work for this test?

This is unrelated. (That's for <svg:image> which is like <img>.)

[fs, 2017-03-31 10:32:47]

The patchset sent to the CQ was uploaded after l-g-t-m from kouhei@chromium.org
Link to the patchset: https://codereview.chromium.org/2783133002/#ps30011
(title: "Fix typo")

[commit-bot: I haz the power, 2017-03-31 10:33:02]

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 10:38:20]

CQ is committing da patch.

Bot data: {"patchset_id": 30011, "attempt_start_ts": 1490956365980340,
"parent_rev": "9770fad0b3ebe389c34672dc8797bf8fbffeda9b", "commit_rev":
"bb03bc3127de97898e62323b12cd2997267cfd5a"}

[commit-bot: I haz the power, 2017-03-31 10:39:10]

Description was changed from

==========
Implement Image::maybeAnimated for SVGImage

This CL renames SVGImage::hasAnimations to maybeAnimated, overriding
the implementation from the base class (Image.) The old method matches
the new one in certainty of the reply, and allows SVGImages to be paused
when being scrolled out of view etc.

BUG=706152
==========

to

==========
Implement Image::maybeAnimated for SVGImage

This CL renames SVGImage::hasAnimations to maybeAnimated, overriding
the implementation from the base class (Image.) The old method matches
the new one in certainty of the reply, and allows SVGImages to be paused
when being scrolled out of view etc.

BUG=706152

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

[commit-bot: I haz the power, 2017-03-31 10:39:11]

Committed patchset #3 (id:30011) as
https://chromium.googlesource.com/chromium/src/+/bb03bc3127de97898e62323b12cd...