Rework timeline/frame scheduling logic for SVGImage

Rework timeline/frame scheduling logic for SVGImage

This CL provides the SVGImage/SVGImageChromeClient complex with the
capability of suspending and resuming the frame/animation tick.
This gives us the mechanism required to respond to
ImageObserver::shouldPauseAnimation, as well as stopping the animation
timer from running after the animation has been reset (via
Image::resetAnimation.)
In the context of the bug referenced this means an animating SVG image
will no longer cause wakeups because of (unnecessary) timer activity,
saving power (and CPU time.)

Implement willRenderImage() for the CrossfadeSubimageObserverProxy of
CSSCrossfadeValue so that it will not (falsely) claim that it won't
render its images.

While doing this, try to make a decent functional split between SVGImage
and the associated SVGImageChromeClient by putting all timeline/frame tick
related code in the latter, while keeping code related to the actual
animation/document lifecycle update in the former.

BUG=612540
Committed: https://crrev.com/5ad4faa93d2554ffab7f72a992adbeffa8796f94
Cr-Commit-Position: refs/heads/master@{#396009}

[fs, 2016-05-20 14:09:07]

Description was changed from

==========
Rework timeline/frame scheduling logic for SVGImage

BUG=612540
==========

to

==========
Rework timeline/frame scheduling logic for SVGImage

This CL provides the SVGImage/SVGImageChromeClient complex with the
capability of suspending and resuming the frame/animation tick.
This gives us the mechanism required to respond to
ImageObserver::shouldPauseAnimation, as well as stopping the animation
timer from running after the animation has been reset (via
Image::resetAnimation.)
In the context of the bug referenced this means an animating SVG image
will no longer cause wakeups because of (unnecessary) timer activity,
saving power (and CPU time.)

While doing this, try to make a decent functional split between SVGImage
and the associated SVGImageChromeClient by putting all timeline/frame tick
related code in the latter, while keeping code related to the actual
animation/document lifecycle update in the former.

BUG=612540
==========

[fs, 2016-05-20 16:05:51]

Description was changed from

==========
Rework timeline/frame scheduling logic for SVGImage

This CL provides the SVGImage/SVGImageChromeClient complex with the
capability of suspending and resuming the frame/animation tick.
This gives us the mechanism required to respond to
ImageObserver::shouldPauseAnimation, as well as stopping the animation
timer from running after the animation has been reset (via
Image::resetAnimation.)
In the context of the bug referenced this means an animating SVG image
will no longer cause wakeups because of (unnecessary) timer activity,
saving power (and CPU time.)

While doing this, try to make a decent functional split between SVGImage
and the associated SVGImageChromeClient by putting all timeline/frame tick
related code in the latter, while keeping code related to the actual
animation/document lifecycle update in the former.

BUG=612540
==========

to

==========
Rework timeline/frame scheduling logic for SVGImage

This CL provides the SVGImage/SVGImageChromeClient complex with the
capability of suspending and resuming the frame/animation tick.
This gives us the mechanism required to respond to
ImageObserver::shouldPauseAnimation, as well as stopping the animation
timer from running after the animation has been reset (via
Image::resetAnimation.)
In the context of the bug referenced this means an animating SVG image
will no longer cause wakeups because of (unnecessary) timer activity,
saving power (and CPU time.)

Implement willRenderImage() for the CrossfadeSubimageObserverProxy of
CSSCrossfadeValue so that it will not (falsely) claim that it won't
render its images.

While doing this, try to make a decent functional split between SVGImage
and the associated SVGImageChromeClient by putting all timeline/frame tick
related code in the latter, while keeping code related to the actual
animation/document lifecycle update in the former.

BUG=612540
==========

[fs, 2016-05-20 17:31:20]

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

[chrishtr, 2016-05-20 18:26:15]

This code needs testing.

https://codereview.chromium.org/2000483003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp
(right):

https://codereview.chromium.org/2000483003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp:86: if
(m_animationTimer.isActive())
Why not just call scheduleAnimation() directly? The Widget parameter is unused
anyway.

[fs, 2016-05-20 19:23:58]

On 2016/05/20 at 18:26:15, chrishtr wrote:
> This code needs testing.

Trying to concoct a unittest for this. (Or, well, next week I will.)

https://codereview.chromium.org/2000483003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp
(right):

https://codereview.chromium.org/2000483003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp:86: if
(m_animationTimer.isActive())
On 2016/05/20 at 18:26:15, chrishtr wrote:
> Why not just call scheduleAnimation() directly? The Widget parameter is unused
anyway.

That would schedule a timer with a delay of |animationFrameDelay| rather than 0.

[chrishtr, 2016-05-20 19:55:18]

https://codereview.chromium.org/2000483003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp
(right):

https://codereview.chromium.org/2000483003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp:86: if
(m_animationTimer.isActive())
On 2016/05/20 at 19:23:58, fs wrote:
> On 2016/05/20 at 18:26:15, chrishtr wrote:
> > Why not just call scheduleAnimation() directly? The Widget parameter is
unused anyway.
> 
> That would schedule a timer with a delay of |animationFrameDelay| rather than
0.

Does that matter?

[fs, 2016-05-20 20:03:06]

https://codereview.chromium.org/2000483003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp
(right):

https://codereview.chromium.org/2000483003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp:86: if
(m_animationTimer.isActive())
On 2016/05/20 at 19:55:18, chrishtr wrote:
> On 2016/05/20 at 19:23:58, fs wrote:
> > On 2016/05/20 at 18:26:15, chrishtr wrote:
> > > Why not just call scheduleAnimation() directly? The Widget parameter is
unused anyway.
> > 
> > That would schedule a timer with a delay of |animationFrameDelay| rather
than 0.
> 
> Does that matter?

I guess we can try it and see.

[chrishtr, 2016-05-23 20:02:52]

Will wait for the unittest.

https://codereview.chromium.org/2000483003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp
(right):

https://codereview.chromium.org/2000483003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp:86: if
(m_animationTimer.isActive())
On 2016/05/20 at 20:03:06, fs wrote:
> On 2016/05/20 at 19:55:18, chrishtr wrote:
> > On 2016/05/20 at 19:23:58, fs wrote:
> > > On 2016/05/20 at 18:26:15, chrishtr wrote:
> > > > Why not just call scheduleAnimation() directly? The Widget parameter is
unused anyway.
> > > 
> > > That would schedule a timer with a delay of |animationFrameDelay| rather
than 0.
> > 
> > Does that matter?
> 
> I guess we can try it and see.

I suppose it doesn't matter that much. Only a few lines of code.

[fs, 2016-05-23 20:58:34]

On 2016/05/23 at 20:02:52, chrishtr wrote:
> Will wait for the unittest.

Didn't have time for that today unfortunately. Hopefully tomorrow.

https://codereview.chromium.org/2000483003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp
(right):

https://codereview.chromium.org/2000483003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp:86: if
(m_animationTimer.isActive())
On 2016/05/23 at 20:02:51, chrishtr wrote:
> On 2016/05/20 at 20:03:06, fs wrote:
> > On 2016/05/20 at 19:55:18, chrishtr wrote:
> > > On 2016/05/20 at 19:23:58, fs wrote:
> > > > On 2016/05/20 at 18:26:15, chrishtr wrote:
> > > > > Why not just call scheduleAnimation() directly? The Widget parameter
is unused anyway.
> > > > 
> > > > That would schedule a timer with a delay of |animationFrameDelay| rather
than 0.
> > > 
> > > Does that matter?
> > 
> > I guess we can try it and see.
> 
> I suppose it doesn't matter that much. Only a few lines of code.

I've shuffled this around a bit now, and this is now calling scheduleAnimation
directly.

[fs, 2016-05-25 16:01:32]

On 2016/05/23 at 20:58:34, fs wrote:
> On 2016/05/23 at 20:02:52, chrishtr wrote:
> > Will wait for the unittest.
> 
> Didn't have time for that today unfortunately. Hopefully tomorrow.

Finally got that done... PTAL.

[chrishtr, 2016-05-25 20:23:47]

https://codereview.chromium.org/2000483003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/graphics/SVGImageTest.cpp (right):

https://codereview.chromium.org/2000483003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/graphics/SVGImageTest.cpp:160:
EXPECT_FALSE(chromeClient.isSuspended());
Why doesn't it pause again?

[fs, 2016-05-25 20:30:16]

https://codereview.chromium.org/2000483003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/graphics/SVGImageTest.cpp (right):

https://codereview.chromium.org/2000483003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/graphics/SVGImageTest.cpp:160:
EXPECT_FALSE(chromeClient.isSuspended());
On 2016/05/25 at 20:23:47, chrishtr wrote:
> Why doesn't it pause again?

It will/would when the timer fires (i.e shouldPauseAnimation is checked again.)

[chrishtr, 2016-05-25 22:18:13]

https://codereview.chromium.org/2000483003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/graphics/SVGImageTest.cpp (right):

https://codereview.chromium.org/2000483003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/graphics/SVGImageTest.cpp:160:
EXPECT_FALSE(chromeClient.isSuspended());
On 2016/05/25 at 20:30:16, fs wrote:
> On 2016/05/25 at 20:23:47, chrishtr wrote:
> > Why doesn't it pause again?
> 
> It will/would when the timer fires (i.e shouldPauseAnimation is checked
again.)

oh right, you're calling pumpFrame() not timer->fire()

[chrishtr, 2016-05-25 22:20:32]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-05-25 22:20:33]

lgtm

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

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

[commit-bot: I haz the power, 2016-05-25 22:28:53]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-05-25 22:31:09]

Description was changed from

==========
Rework timeline/frame scheduling logic for SVGImage

This CL provides the SVGImage/SVGImageChromeClient complex with the
capability of suspending and resuming the frame/animation tick.
This gives us the mechanism required to respond to
ImageObserver::shouldPauseAnimation, as well as stopping the animation
timer from running after the animation has been reset (via
Image::resetAnimation.)
In the context of the bug referenced this means an animating SVG image
will no longer cause wakeups because of (unnecessary) timer activity,
saving power (and CPU time.)

Implement willRenderImage() for the CrossfadeSubimageObserverProxy of
CSSCrossfadeValue so that it will not (falsely) claim that it won't
render its images.

While doing this, try to make a decent functional split between SVGImage
and the associated SVGImageChromeClient by putting all timeline/frame tick
related code in the latter, while keeping code related to the actual
animation/document lifecycle update in the former.

BUG=612540
==========

to

==========
Rework timeline/frame scheduling logic for SVGImage

This CL provides the SVGImage/SVGImageChromeClient complex with the
capability of suspending and resuming the frame/animation tick.
This gives us the mechanism required to respond to
ImageObserver::shouldPauseAnimation, as well as stopping the animation
timer from running after the animation has been reset (via
Image::resetAnimation.)
In the context of the bug referenced this means an animating SVG image
will no longer cause wakeups because of (unnecessary) timer activity,
saving power (and CPU time.)

Implement willRenderImage() for the CrossfadeSubimageObserverProxy of
CSSCrossfadeValue so that it will not (falsely) claim that it won't
render its images.

While doing this, try to make a decent functional split between SVGImage
and the associated SVGImageChromeClient by putting all timeline/frame tick
related code in the latter, while keeping code related to the actual
animation/document lifecycle update in the former.

BUG=612540

Committed: https://crrev.com/5ad4faa93d2554ffab7f72a992adbeffa8796f94
Cr-Commit-Position: refs/heads/master@{#396009}
==========

[commit-bot: I haz the power, 2016-05-25 22:31:10]

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