Plumb timeToNextEffect through players and animation

Web Animations: Plumb timeToNextEffect through players and animation

timeToNextEffect is used to return how long before an animation needs another sample (or example, an animation that has just started with a delay of 2s will have a timeToNextEffect of 2s).

We intend to union these values together in Timelines so that we can set a timer when we have nothing to do for a long period of time (currently we just use requestAnimationFrame).

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

[Timothy Loh, 2013-10-18 06:01:27]

You might want to upload and link to the next part of this for more context,
although I think it would be fine to just have this as part of a larger patch
instead of adding it before it's used.

https://codereview.chromium.org/28263002/diff/1/Source/core/animation/Documen...
File Source/core/animation/DocumentTimeline.cpp (right):

https://codereview.chromium.org/28263002/diff/1/Source/core/animation/Documen...
Source/core/animation/DocumentTimeline.cpp:79: if
(!m_players[i]->update(timeToNextEffect))
Won't this only keep the last value?

https://codereview.chromium.org/28263002/diff/1/Source/core/animation/Player.h
File Source/core/animation/Player.h (right):

https://codereview.chromium.org/28263002/diff/1/Source/core/animation/Player....
Source/core/animation/Player.h:49: //  -1  - if this player is no longer in
effect
Maybe change -1 to std::numeric_limits<double>::infinity() so you can use min to
reduce multiple values? It's also more semantically meaningful.

https://codereview.chromium.org/28263002/diff/1/Source/core/animation/Player....
Source/core/animation/Player.h:54: inline bool update()
Do we need to keep this version around?

https://codereview.chromium.org/28263002/diff/1/Source/core/animation/Player....
Source/core/animation/Player.h:56: double tmp;
We don't use these sorts of abbreviated variable names. Maybe 'placeholder', but
someone else can probably come up with a better name here.

https://codereview.chromium.org/28263002/diff/1/Source/core/animation/TimedIt...
File Source/core/animation/TimedItem.h (right):

https://codereview.chromium.org/28263002/diff/1/Source/core/animation/TimedIt...
Source/core/animation/TimedItem.h:98: virtual double
calculateTimeToEffectChange(double inheritedTime, double activeTime, Phase)
const = 0;
Can we make this private? I think protected-virtual is for virtual functions
where you want to be able to explicitly call your ancestors' functions, while
private-virtual disallows this.

[shans, 2013-10-21 00:37:57]

https://codereview.chromium.org/28263002/diff/1/Source/core/animation/Documen...
File Source/core/animation/DocumentTimeline.cpp (right):

https://codereview.chromium.org/28263002/diff/1/Source/core/animation/Documen...
Source/core/animation/DocumentTimeline.cpp:79: if
(!m_players[i]->update(timeToNextEffect))
On 2013/10/18 06:01:27, Timothy Loh wrote:
> Won't this only keep the last value?

Yeah. This point is where the next patch would kick in.

https://codereview.chromium.org/28263002/diff/1/Source/core/animation/Player.h
File Source/core/animation/Player.h (right):

https://codereview.chromium.org/28263002/diff/1/Source/core/animation/Player....
Source/core/animation/Player.h:49: //  -1  - if this player is no longer in
effect
On 2013/10/18 06:01:27, Timothy Loh wrote:
> Maybe change -1 to std::numeric_limits<double>::infinity() so you can use min
to
> reduce multiple values? It's also more semantically meaningful.

Done.

https://codereview.chromium.org/28263002/diff/1/Source/core/animation/Player....
Source/core/animation/Player.h:54: inline bool update()
On 2013/10/18 06:01:27, Timothy Loh wrote:
> Do we need to keep this version around?

Yeah. There are a few places where we simply don't care about the result of
timeToEffectChange. It's not cool to put the onus of creating a throwaway
variable on the caller in these cases.

https://codereview.chromium.org/28263002/diff/1/Source/core/animation/Player....
Source/core/animation/Player.h:56: double tmp;
On 2013/10/18 06:01:27, Timothy Loh wrote:
> We don't use these sorts of abbreviated variable names. Maybe 'placeholder',
but
> someone else can probably come up with a better name here.

Changed to 'unused'

https://codereview.chromium.org/28263002/diff/1/Source/core/animation/TimedIt...
File Source/core/animation/TimedItem.h (right):

https://codereview.chromium.org/28263002/diff/1/Source/core/animation/TimedIt...
Source/core/animation/TimedItem.h:98: virtual double
calculateTimeToEffectChange(double inheritedTime, double activeTime, Phase)
const = 0;
On 2013/10/18 06:01:27, Timothy Loh wrote:
> Can we make this private? I think protected-virtual is for virtual functions
> where you want to be able to explicitly call your ancestors' functions, while
> private-virtual disallows this.

private-virtual disallows *any* access to the ancestor version of the function,
so it's impossible to e.g. define a single version at the top of an inheritance
subtree - every class needs to have its own implementation (or be abstract).

calculateTimeToEffectChange can probably be made private for now
(updateChildrenAndEffects and willDetach too), but we'll likely run into this
issue at some point in the future. In constract, we already can't make
intrinsicIterationDuration private because we have a default implementation in
TimedItem.

I think I'd rather keep all of these functions at the same level of access
(they're all part of the same process and will evolve together as we support the
rest of the model), but I don't feel particularly strongly about it.

[dstockwell, 2013-10-21 01:51:31]

lgtm

https://codereview.chromium.org/28263002/diff/240001/Source/core/animation/Do...
File Source/core/animation/DocumentTimeline.cpp (right):

https://codereview.chromium.org/28263002/diff/240001/Source/core/animation/Do...
Source/core/animation/DocumentTimeline.cpp:79: if
(!m_players[i]->update(&timeToNextEffect))
We could just not pass a param here. But I guess this will be fixed in the next
change.

[Timothy Loh, 2013-10-21 01:53:43]

lgtm

[commit-bot: I haz the power, 2013-10-21 03:06:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shans@chromium.org/28263002/240001

[commit-bot: I haz the power, 2013-10-21 04:42:25]

Change committed as 160054