Animations: Allow document animation timelines to have a playback rate

Animations: Allow document animation timelines to have a playback rate

This change allows for animation timelines to have a playback rate
specified which can then be used to pause or slow down all animations
on a timeline. An inspector protocol method is also added to allow the
animation timeline playback rate to be set on all document animation
timelines associated with a page.

BUG=432368
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185262

[samli, 2014-11-12 02:04:23]

samli@chromium.org changed reviewers:
+ alancutter@chromium.org, vsevik@chromium.org

[samli, 2014-11-12 02:04:23]

PTAL. This patch doesn't include any UI changes and thus has no effective
change. It also does't include changes to keep the compositor in sync with the
main thread.

[alancutter (OOO until 2018), 2014-11-12 03:19:17]

alancutter@chromium.org changed reviewers:
+ dstockwell@chromium.org

[alancutter (OOO until 2018), 2014-11-12 03:19:18]

+dstockwell

https://codereview.chromium.org/717003002/diff/20001/Source/core/animation/An...
File Source/core/animation/AnimationTimeline.h (right):

https://codereview.chromium.org/717003002/diff/20001/Source/core/animation/An...
Source/core/animation/AnimationTimeline.h:118: double m_lastRealTime;
These names are quite unclear. It's hard to think of a good name so perhaps a
comment here explaining what these member variables are would suffice.

https://codereview.chromium.org/717003002/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InspectorPageAgent.cpp (right):

https://codereview.chromium.org/717003002/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorPageAgent.cpp:1487:
document->timeline().setPlaybackRateForInspector(playbackRate);
No need for vector, just perform this operation inside the first for loop.

[dstockwell, 2014-11-12 03:39:42]

https://codereview.chromium.org/717003002/diff/20001/Source/core/animation/An...
File Source/core/animation/AnimationTest.cpp (right):

https://codereview.chromium.org/717003002/diff/20001/Source/core/animation/An...
Source/core/animation/AnimationTest.cpp:31: 
Remove stray blank line.

https://codereview.chromium.org/717003002/diff/20001/Source/core/animation/An...
File Source/core/animation/AnimationTimeline.cpp (left):

https://codereview.chromium.org/717003002/diff/20001/Source/core/animation/An...
Source/core/animation/AnimationTimeline.cpp:170: if (!m_zeroTime && m_document
&& m_document->loader()) {
Don't reformat code.

https://codereview.chromium.org/717003002/diff/20001/Source/core/animation/An...
File Source/core/animation/AnimationTimeline.cpp (right):

https://codereview.chromium.org/717003002/diff/20001/Source/core/animation/An...
Source/core/animation/AnimationTimeline.cpp:235: void
AnimationTimeline::setPlaybackRateForInspector(double playbackRate)
Add a fixme, needs to invalidate compositor animations.

https://codereview.chromium.org/717003002/diff/20001/Source/core/animation/An...
Source/core/animation/AnimationTimeline.cpp:237: m_lastCurrentTime =
currentTimeInternal();
I'm not sure you need both lastCurrentTime and lastRealTime. Seems you just need
to shift the animationClock's currentTime.

eg. in the currentTime() calcuation above, something like:

return (m_document->animationClock().currentTime() + m_delta - zeroTime()) *
playbackRate();

Should be able to recover a delta value here.

https://codereview.chromium.org/717003002/diff/20001/Source/core/animation/An...
File Source/core/animation/AnimationTimeline.h (right):

https://codereview.chromium.org/717003002/diff/20001/Source/core/animation/An...
Source/core/animation/AnimationTimeline.h:94: void
setPlaybackRateForInspector(double);
remove "ForInspector" this should be a general feature

https://codereview.chromium.org/717003002/diff/20001/Source/core/animation/An...
Source/core/animation/AnimationTimeline.h:116: // Inspector variables
Don't know what these are for.

[vsevik, 2014-11-12 15:54:27]

inspector changes lgtm provided alancutter's comment is addressed.

[samli, 2014-11-12 20:01:13]

PTAL at animation changes

https://codereview.chromium.org/717003002/diff/20001/Source/core/animation/An...
File Source/core/animation/AnimationTest.cpp (right):

https://codereview.chromium.org/717003002/diff/20001/Source/core/animation/An...
Source/core/animation/AnimationTest.cpp:31: 
On 2014/11/12 03:39:42, dstockwell wrote:
> Remove stray blank line.

Done.

https://codereview.chromium.org/717003002/diff/20001/Source/core/animation/An...
File Source/core/animation/AnimationTimeline.cpp (left):

https://codereview.chromium.org/717003002/diff/20001/Source/core/animation/An...
Source/core/animation/AnimationTimeline.cpp:170: if (!m_zeroTime && m_document
&& m_document->loader()) {
On 2014/11/12 03:39:42, dstockwell wrote:
> Don't reformat code.

Done.

https://codereview.chromium.org/717003002/diff/20001/Source/core/animation/An...
File Source/core/animation/AnimationTimeline.cpp (right):

https://codereview.chromium.org/717003002/diff/20001/Source/core/animation/An...
Source/core/animation/AnimationTimeline.cpp:235: void
AnimationTimeline::setPlaybackRateForInspector(double playbackRate)
On 2014/11/12 03:39:42, dstockwell wrote:
> Add a fixme, needs to invalidate compositor animations.

Done.

https://codereview.chromium.org/717003002/diff/20001/Source/core/animation/An...
Source/core/animation/AnimationTimeline.cpp:237: m_lastCurrentTime =
currentTimeInternal();
On 2014/11/12 03:39:42, dstockwell wrote:
> I'm not sure you need both lastCurrentTime and lastRealTime. Seems you just
need
> to shift the animationClock's currentTime.
> 
> eg. in the currentTime() calcuation above, something like:
> 
> return (m_document->animationClock().currentTime() + m_delta - zeroTime()) *
> playbackRate();
> 
> Should be able to recover a delta value here.

(m_document->animationClock().currentTime() + m_delta - zeroTime()) *
playbackRate()) doesn't work. Simple case: play for a bit and set playback rate
to 0, this would return current time is now 0. You need to store 2 pieces of
information at minimum: a snapshotted value of what the animationClock's
currentTime from the last change in playback rate and a delta/offset of how that
maps across to the currentTime (when I change the playback rate multiple times).

This method here just snapshots the raw currentTime with no changes and one with
an offset instead of having to calculate what that offset value would be.
Something like finding delta * playbackRate. Current time calculation in this
case is: last snapshotted current time value + time thats elapsed since then
(need to use animationClock time here) * playbackRate. eg if playback rate is 0,
the delta evaluates to 0 and we return the base value of when it was paused.

The variable names probably make this a bit unclear so I'll try think of
something better.

https://codereview.chromium.org/717003002/diff/20001/Source/core/animation/An...
File Source/core/animation/AnimationTimeline.h (right):

https://codereview.chromium.org/717003002/diff/20001/Source/core/animation/An...
Source/core/animation/AnimationTimeline.h:94: void
setPlaybackRateForInspector(double);
On 2014/11/12 03:39:42, dstockwell wrote:
> remove "ForInspector" this should be a general feature

Done.

https://codereview.chromium.org/717003002/diff/20001/Source/core/animation/An...
Source/core/animation/AnimationTimeline.h:116: // Inspector variables
On 2014/11/12 03:39:42, dstockwell wrote:
> Don't know what these are for.

Done.

https://codereview.chromium.org/717003002/diff/20001/Source/core/animation/An...
Source/core/animation/AnimationTimeline.h:118: double m_lastRealTime;
On 2014/11/12 03:19:18, alancutter wrote:
> These names are quite unclear. It's hard to think of a good name so perhaps a
> comment here explaining what these member variables are would suffice.

Done.

https://codereview.chromium.org/717003002/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InspectorPageAgent.cpp (right):

https://codereview.chromium.org/717003002/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorPageAgent.cpp:1487:
document->timeline().setPlaybackRateForInspector(playbackRate);
On 2014/11/12 03:19:18, alancutter wrote:
> No need for vector, just perform this operation inside the first for loop.

Done.

[dstockwell, 2014-11-12 22:49:44]

https://codereview.chromium.org/717003002/diff/20001/Source/core/animation/An...
File Source/core/animation/AnimationTimeline.cpp (right):

https://codereview.chromium.org/717003002/diff/20001/Source/core/animation/An...
Source/core/animation/AnimationTimeline.cpp:237: m_lastCurrentTime =
currentTimeInternal();
On 2014/11/12 at 20:01:13, samli wrote:
> On 2014/11/12 03:39:42, dstockwell wrote:
> > I'm not sure you need both lastCurrentTime and lastRealTime. Seems you just
need
> > to shift the animationClock's currentTime.
> > 
> > eg. in the currentTime() calcuation above, something like:
> > 
> > return (m_document->animationClock().currentTime() + m_delta - zeroTime()) *
> > playbackRate();
> > 
> > Should be able to recover a delta value here.
> 
> (m_document->animationClock().currentTime() + m_delta - zeroTime()) *
> playbackRate()) doesn't work. Simple case: play for a bit and set playback
rate to 0, this would return current time is now 0. You need to store 2 pieces
of information at minimum: a snapshotted value of what the animationClock's
currentTime from the last change in playback rate and a delta/offset of how that
maps across to the currentTime (when I change the playback rate multiple times).
> 
> This method here just snapshots the raw currentTime with no changes and one
with an offset instead of having to calculate what that offset value would be.
Something like finding delta * playbackRate. Current time calculation in this
case is: last snapshotted current time value + time thats elapsed since then
(need to use animationClock time here) * playbackRate. eg if playback rate is 0,
the delta evaluates to 0 and we return the base value of when it was paused.
> 
> The variable names probably make this a bit unclear so I'll try think of
something better.

Can we just special case playback rate 0, like in the Web Animations spec and as
we do for Animation Player?

rate 0 would store or clear m_holdTime, currentTime() would return m_holdTime if
it's not NaN. For other rates we would use the delta.

[dstockwell, 2014-11-13 00:43:18]

lgtm

[samli, 2014-11-13 01:16:28]

The CQ bit was checked by samli@chromium.org

[commit-bot: I haz the power, 2014-11-13 01:17:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/717003002/40001

[commit-bot: I haz the power, 2014-11-13 03:11:53]

Committed patchset #3 (id:40001) as 185262