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

Source/core/animation/CompositorAnimations.cpp

Index: Source/core/animation/CompositorAnimations.cpp
diff --git a/Source/core/animation/CompositorAnimations.cpp b/Source/core/animation/CompositorAnimations.cpp
index ef29fc8a6335de77863837900410dc4b587ebed7..0fe99df503e323520548dfdd3d60529615359daf 100644
--- a/Source/core/animation/CompositorAnimations.cpp
+++ b/Source/core/animation/CompositorAnimations.cpp
@@ -173,15 +173,18 @@ bool CompositorAnimations::isCandidateForAnimationOnCompositor(const Timing& tim
     if (!CompositorAnimationsImpl::convertTimingForCompositor(timing, 0, out, playerPlaybackRate))
         return false;
 
-    if (timing.timingFunction->type() != TimingFunction::LinearFunction) {
-        // Checks the of size of KeyframeVector instead of PropertySpecificKeyframeVector.
+    if (timing.timingFunction->type() == TimingFunction::CubicBezierFunction) {
+        // FIXME: Fix compositor timing functions to accept inputs outside of

[dstockwell, 2015/01/05 23:13:24]

Does this fixme apply to line 187? Should be moved closer.

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

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

+        // [0,1].
+        const CubicBezierTimingFunction& cubic = toCubicBezierTimingFunction(*timing.timingFunction);
         const KeyframeVector& keyframes = keyframeEffect.getFrames();
-        if (keyframes.size() == 2 && keyframes.first()->easing().type() == TimingFunction::LinearFunction)
-            return true;
+        double startRange = 0;

[dstockwell, 2015/01/05 23:13:24]

start/end should be min/max

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

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

+        double endRange = 1;
+        cubic.range(&startRange, &endRange);
 
-        // FIXME: Support non-linear timing functions in the compositor for
-        // more than two keyframes.
-        return false;
+        ASSERT(keyframes.size() >= 2);
+        if ((startRange < 0 || endRange > 1) && (keyframes.first()->easing().type() != TimingFunction::LinearFunction || keyframes[keyframes.size() - 2]->easing().type() != TimingFunction::LinearFunction))

[dstockwell, 2015/01/05 23:13:24]

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?

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

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)

[dstockwell, 2015/01/06 03:33:49]

Ahh, yes makes sense.
OK, no need then.

+            return false;
     }
 
     return true;
@@ -277,6 +280,58 @@ bool CompositorAnimationsImpl::convertTimingForCompositor(const Timing& timing,
 
 namespace {
 
+void getCubicBezierTimingFunctionParameters(const TimingFunction& timingFunction, bool& outCustom,
+    WebCompositorAnimationCurve::TimingFunctionType& outEaseSubType,
+    double& outX1, double& outY1, double& outX2, double& outY2)
+{
+    const CubicBezierTimingFunction& cubic = toCubicBezierTimingFunction(timingFunction);
+    outCustom = false;
+
+    switch (cubic.subType()) {
+    case CubicBezierTimingFunction::Ease:
+        outEaseSubType = WebCompositorAnimationCurve::TimingFunctionTypeEase;
+        break;
+    case CubicBezierTimingFunction::EaseIn:
+        outEaseSubType = WebCompositorAnimationCurve::TimingFunctionTypeEaseIn;
+        break;
+    case CubicBezierTimingFunction::EaseOut:
+        outEaseSubType = WebCompositorAnimationCurve::TimingFunctionTypeEaseOut;
+        break;
+    case CubicBezierTimingFunction::EaseInOut:
+        outEaseSubType = WebCompositorAnimationCurve::TimingFunctionTypeEaseInOut;
+        break;
+    case CubicBezierTimingFunction::Custom:
+        outCustom = true;
+        outX1 = cubic.x1();
+        outY1 = cubic.y1();
+        outX2 = cubic.x2();
+        outY2 = cubic.y2();
+        break;
+    default:
+        ASSERT_NOT_REACHED();
+    }
+}
+
+void getStepsTimingFunctionParameters(const TimingFunction& timingFunction, int& outSteps, float& outStepsStartOffset)
+{
+    const StepsTimingFunction& steps = toStepsTimingFunction(timingFunction);
+
+    outSteps = steps.numberOfSteps();
+    switch (steps.stepAtPosition()) {
+    case StepsTimingFunction::Start:
+        outStepsStartOffset = 1;
+        break;
+    case StepsTimingFunction::Middle:
+        outStepsStartOffset = 0.5;
+        break;
+    case StepsTimingFunction::End:
+        outStepsStartOffset = 0;
+        break;
+    default:
+        ASSERT_NOT_REACHED();
+    }
+}
+
 template<typename PlatformAnimationCurveType, typename PlatformAnimationKeyframeType>
 void addKeyframeWithTimingFunction(PlatformAnimationCurveType& curve, const PlatformAnimationKeyframeType& keyframe, const TimingFunction* timingFunction)
 {
@@ -288,62 +343,74 @@ void addKeyframeWithTimingFunction(PlatformAnimationCurveType& curve, const Plat
     switch (timingFunction->type()) {
     case TimingFunction::LinearFunction:
         curve.add(keyframe, WebCompositorAnimationCurve::TimingFunctionTypeLinear);
-        return;
+        break;
 
     case TimingFunction::CubicBezierFunction: {
-        const CubicBezierTimingFunction* cubic = toCubicBezierTimingFunction(timingFunction);
+        bool custom;
+        WebCompositorAnimationCurve::TimingFunctionType easeSubType;
+        double x1, y1;
+        double x2, y2;
+        getCubicBezierTimingFunctionParameters(*timingFunction, custom, easeSubType, x1, y1, x2, y2);
+
+        if (custom)
+            curve.add(keyframe, x1, y1, x2, y2);
+        else
+            curve.add(keyframe, easeSubType);

[dstockwell, 2015/01/05 23:13:24]

Why not always specify the control points?

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

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.

+        break;
+    }
 
-        if (cubic->subType() == CubicBezierTimingFunction::Custom) {
-            curve.add(keyframe, cubic->x1(), cubic->y1(), cubic->x2(), cubic->y2());
-        } else {
+    case TimingFunction::StepsFunction: {
+        int steps;
+        float stepsStartOffset;
+        getStepsTimingFunctionParameters(*timingFunction, steps, stepsStartOffset);
 
-            WebCompositorAnimationCurve::TimingFunctionType easeType;
-            switch (cubic->subType()) {
-            case CubicBezierTimingFunction::Ease:
-                easeType = WebCompositorAnimationCurve::TimingFunctionTypeEase;
-                break;
-            case CubicBezierTimingFunction::EaseIn:
-                easeType = WebCompositorAnimationCurve::TimingFunctionTypeEaseIn;
-                break;
-            case CubicBezierTimingFunction::EaseOut:
-                easeType = WebCompositorAnimationCurve::TimingFunctionTypeEaseOut;
-                break;
-            case CubicBezierTimingFunction::EaseInOut:
-                easeType = WebCompositorAnimationCurve::TimingFunctionTypeEaseInOut;
-                break;
+        curve.add(keyframe, steps, stepsStartOffset);
+        break;
+    }
 
-            // Custom Bezier are handled seperately.
-            case CubicBezierTimingFunction::Custom:
-            default:
-                ASSERT_NOT_REACHED();
-                return;
-            }
+    default:
+        ASSERT_NOT_REACHED();
+    }
+}
 
-            curve.add(keyframe, easeType);
-        }
+template <typename PlatformAnimationCurveType>
+void setTimingFunctionOnCurve(PlatformAnimationCurveType& curve, TimingFunction* timingFunction)
+{
+    if (!timingFunction) {
+        curve.setLinearTimingFunction();
         return;
     }
 
-    case TimingFunction::StepsFunction:
-        const StepsTimingFunction* steps = toStepsTimingFunction(timingFunction);
+    switch (timingFunction->type()) {
+    case TimingFunction::LinearFunction:
+        curve.setLinearTimingFunction();
+        break;
+
+    case TimingFunction::CubicBezierFunction: {
+        bool custom;
+        WebCompositorAnimationCurve::TimingFunctionType easeSubType;
+        double x1, y1;
+        double x2, y2;
+        getCubicBezierTimingFunctionParameters(*timingFunction, custom, easeSubType, x1, y1, x2, y2);
+
+        if (custom)
+            curve.setCubicBezierTimingFunction(x1, y1, x2, y2);
+        else
+            curve.setCubicBezierTimingFunction(easeSubType);
+        break;
+    }
 
+    case TimingFunction::StepsFunction: {
+        int steps;
         float stepsStartOffset;
-        switch (steps->stepAtPosition()) {
-        case StepsTimingFunction::Start:
-            stepsStartOffset = 1;
-            break;
-        case StepsTimingFunction::Middle:
-            stepsStartOffset = 0.5;
-            break;
-        case StepsTimingFunction::End:
-            stepsStartOffset = 0;
-            break;
-        default:
-            ASSERT_NOT_REACHED();
-            return;
-        }
-        curve.add(keyframe, steps->numberOfSteps(), stepsStartOffset);
-        return;
+        getStepsTimingFunctionParameters(*timingFunction, steps, stepsStartOffset);
+
+        curve.setStepsTimingFunction(steps, stepsStartOffset);
+        break;
+    }
+
+    default:
+        ASSERT_NOT_REACHED();
     }
 }
 
@@ -355,10 +422,7 @@ void CompositorAnimationsImpl::addKeyframesToCurve(WebCompositorAnimationCurve&
     for (const auto& keyframe : keyframes) {
         const TimingFunction* keyframeTimingFunction = 0;
         if (keyframe != lastKeyframe) { // Ignore timing function of last frame.
-            if (keyframes.size() == 2 && keyframes.first()->easing().type() == TimingFunction::LinearFunction)
-                keyframeTimingFunction = timing.timingFunction.get();
-            else
-                keyframeTimingFunction = &keyframe->easing();
+            keyframeTimingFunction = &keyframe->easing();
         }
 
         // FIXME: This relies on StringKeyframes being eagerly evaluated, which will
@@ -418,6 +482,7 @@ void CompositorAnimationsImpl::getAnimationOnCompositor(const Timing& timing, in
 
             WebFloatAnimationCurve* floatCurve = Platform::current()->compositorSupport()->createFloatAnimationCurve();
             addKeyframesToCurve(*floatCurve, values, timing);
+            setTimingFunctionOnCurve(*floatCurve, timing.timingFunction.get());
             curve = adoptPtr(floatCurve);
             break;
         }
@@ -425,6 +490,7 @@ void CompositorAnimationsImpl::getAnimationOnCompositor(const Timing& timing, in
             targetProperty = WebCompositorAnimation::TargetPropertyFilter;
             WebFilterAnimationCurve* filterCurve = Platform::current()->compositorSupport()->createFilterAnimationCurve();
             addKeyframesToCurve(*filterCurve, values, timing);
+            setTimingFunctionOnCurve(*filterCurve, timing.timingFunction.get());
             curve = adoptPtr(filterCurve);
             break;
         }
@@ -432,6 +498,7 @@ void CompositorAnimationsImpl::getAnimationOnCompositor(const Timing& timing, in
             targetProperty = WebCompositorAnimation::TargetPropertyTransform;
             WebTransformAnimationCurve* transformCurve = Platform::current()->compositorSupport()->createTransformAnimationCurve();
             addKeyframesToCurve(*transformCurve, values, timing);
+            setTimingFunctionOnCurve(*transformCurve, timing.timingFunction.get());
             curve = adoptPtr(transformCurve);
             break;
         }