Animation: Allow CC per-curve timing functions to be steps timing functions

Animation: Allow CC per-curve timing functions to be steps timing functions

Implement per-curve timing functions.
A blink part. Chromium part: https://codereview.chromium.org/814853003

In general: Allow Web Animations with Animation timing functions to run on compositor.

BUG=417909
R=dstockwell@chromium.org
R=ajuma@chromium.org
R=japhet@chromium.org
TEST=ManualTests/animation/compositor-animation-steps.html

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

[loyso (OOO), 2015-01-05 03:58:42]

loyso@chromium.org changed reviewers:
+ pfeldman@chromium.org

[ajuma, 2015-01-05 15:48:01]

Non-owner lgtm for public/platform.

[dstockwell, 2015-01-05 23:13:25]

https://codereview.chromium.org/800133004/diff/1/Source/core/animation/Compos...
File Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/800133004/diff/1/Source/core/animation/Compos...
Source/core/animation/CompositorAnimations.cpp:177: // FIXME: Fix compositor
timing functions to accept inputs outside of
Does this fixme apply to line 187? Should be moved closer.

https://codereview.chromium.org/800133004/diff/1/Source/core/animation/Compos...
Source/core/animation/CompositorAnimations.cpp:181: double startRange = 0;
start/end should be min/max

https://codereview.chromium.org/800133004/diff/1/Source/core/animation/Compos...
Source/core/animation/CompositorAnimations.cpp:186: if ((startRange < 0 ||
endRange > 1) && (keyframes.first()->easing().type() !=
TimingFunction::LinearFunction || keyframes[keyframes.size() -
2]->easing().type() != TimingFunction::LinearFunction))
Can you add a comment to explain this? If it's related to the fixme from above,
why don't we care about the timing functions on the keyframes not tested here?

https://codereview.chromium.org/800133004/diff/1/Source/core/animation/Compos...
Source/core/animation/CompositorAnimations.cpp:358: curve.add(keyframe,
easeSubType);
Why not always specify the control points?

[loyso (OOO), 2015-01-06 00:45:29]

https://codereview.chromium.org/800133004/diff/1/Source/core/animation/Compos...
File Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/800133004/diff/1/Source/core/animation/Compos...
Source/core/animation/CompositorAnimations.cpp:177: // FIXME: Fix compositor
timing functions to accept inputs outside of
On 2015/01/05 23:13:24, dstockwell wrote:
> Does this fixme apply to line 187? Should be moved closer.

It's related to the whole block. See my comment below.

https://codereview.chromium.org/800133004/diff/1/Source/core/animation/Compos...
Source/core/animation/CompositorAnimations.cpp:181: double startRange = 0;
On 2015/01/05 23:13:24, dstockwell wrote:
> start/end should be min/max

Agreed. In fact, this code is deleted in the upcoming CL (see the link below).
Should I fix this nit? :)

https://codereview.chromium.org/800133004/diff/1/Source/core/animation/Compos...
Source/core/animation/CompositorAnimations.cpp:186: if ((startRange < 0 ||
endRange > 1) && (keyframes.first()->easing().type() !=
TimingFunction::LinearFunction || keyframes[keyframes.size() -
2]->easing().type() != TimingFunction::LinearFunction))
On 2015/01/05 23:13:24, dstockwell wrote:
> Can you add a comment to explain this? If it's related to the fixme from
above,
> why don't we care about the timing functions on the keyframes not tested here?

Yes, this whole block is related to FIXME from above. If per-curve timing
function produces time out of range [0,1] then it query the first key for
extrapolated values for t<0 and the last key for t>1.

I could put a comment, but we already have this FIXME fixed and under your
review :) It's here https://codereview.chromium.org/787353003/patch/1/10002
(removes this whole block)

https://codereview.chromium.org/800133004/diff/1/Source/core/animation/Compos...
Source/core/animation/CompositorAnimations.cpp:358: curve.add(keyframe,
easeSubType);
On 2015/01/05 23:13:24, dstockwell wrote:
> Why not always specify the control points?

That would be a lose of information. At this stage I want to keep the CC part in
structure as close as possible to blink implementation (to get rid of one of
them finally). blink::CubicBezierTimingFunction stores it's subtype. So I want
to store it the same way.

[dstockwell, 2015-01-06 03:33:50]

lgtm

https://codereview.chromium.org/800133004/diff/1/Source/core/animation/Compos...
File Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/800133004/diff/1/Source/core/animation/Compos...
Source/core/animation/CompositorAnimations.cpp:186: if ((startRange < 0 ||
endRange > 1) && (keyframes.first()->easing().type() !=
TimingFunction::LinearFunction || keyframes[keyframes.size() -
2]->easing().type() != TimingFunction::LinearFunction))
On 2015/01/06 at 00:45:29, loyso wrote:
> On 2015/01/05 23:13:24, dstockwell wrote:
> > Can you add a comment to explain this? If it's related to the fixme from
above,
> > why don't we care about the timing functions on the keyframes not tested
here?
> 
> Yes, this whole block is related to FIXME from above. If per-curve timing
function produces time out of range [0,1] then it query the first key for
extrapolated values for t<0 and the last key for t>1.
> 
Ahh, yes makes sense.

> I could put a comment, but we already have this FIXME fixed and under your
review :) It's here https://codereview.chromium.org/787353003/patch/1/10002
> (removes this whole block)

OK, no need then.

[loyso (OOO), 2015-01-07 01:00:50]

loyso@chromium.org changed reviewers:
+ dglazkov@chromium.org

[loyso (OOO), 2015-01-07 01:09:33]

Just one approval needed for public/platform/OWNERS. Please!

[loyso (OOO), 2015-01-07 05:56:35]

The CQ bit was checked by loyso@chromium.org

[commit-bot: I haz the power, 2015-01-07 05:56:49]

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

[loyso (OOO), 2015-01-07 05:56:57]

The CQ bit was unchecked by loyso@chromium.org

[Nate Chapin, 2015-01-07 19:00:30]

lgtm

https://codereview.chromium.org/800133004/diff/20001/public/platform/WebFilte...
File public/platform/WebFilterAnimationCurve.h (right):

https://codereview.chromium.org/800133004/diff/20001/public/platform/WebFilte...
public/platform/WebFilterAnimationCurve.h:56: virtual void
setStepsTimingFunction(int numberOfSteps, float stepsStartOffset) = 0;
It's kind of unfortunate to have to define these identical functions on 3
different interfaces. I take it that there is a subclass of
WebCompositorAnimationCurve for which these would be nonsensical?

[loyso (OOO), 2015-01-07 23:27:01]

On 2015/01/07 19:00:30, Nate Chapin wrote:
> lgtm
> 
>
https://codereview.chromium.org/800133004/diff/20001/public/platform/WebFilte...
> File public/platform/WebFilterAnimationCurve.h (right):
> 
>
https://codereview.chromium.org/800133004/diff/20001/public/platform/WebFilte...
> public/platform/WebFilterAnimationCurve.h:56: virtual void
> setStepsTimingFunction(int numberOfSteps, float stepsStartOffset) = 0;
> It's kind of unfortunate to have to define these identical functions on 3
> different interfaces. I take it that there is a subclass of
> WebCompositorAnimationCurve for which these would be nonsensical?

Exactly. That's WebScrollOffsetAnimationCurve.
Moreover, we have timing functions/curves/animation system implemented in CC and
blink, which is redundant. Whould be nice to revisit architectural diagram
(layers) and, say, use blink timing functions in cc/animation directly.

[loyso (OOO), 2015-01-07 23:28:07]

The CQ bit was checked by loyso@chromium.org

[commit-bot: I haz the power, 2015-01-07 23:29:10]

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

[commit-bot: I haz the power, 2015-01-07 23:36:02]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188006