Bug 79819 – [chromium] Add impl-thread support for animation-timing-function

• Component: WebKit Misc.
• Status: RESOLVED FIXED

Add support for animation-timing-function as per http://www.w3.org/TR/css3-transitions/ on the impl-thread

Created attachment 129305 [details]
Patch

[Nat Duca]

Comment on attachment 129305 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129305&action=review

Dumb question --- if we had N-step cubic bezier curves, could we convert this bizarreness with the per-frame 

Can you pull out the const-correctness changes to UnitBezier to a separate patch?

Can we call CCTimingFunction -> CCTimingCurve?

> Source/WebCore/platform/graphics/chromium/cc/CCKeyframedAnimationCurve.cpp:-47
> -    if (!keyframes.size())

can we at least put an early-check for the keyframe being at the end to make the common case fast?

> Source/WebCore/platform/graphics/chromium/cc/CCKeyframedAnimationCurve.cpp:47
> +    OwnPtr<Keyframe> popKeyframe = keyframe;

also, do we have unit tests that insert keyframes out of order and verify that the keyframes are sorted?

> Source/WebCore/platform/graphics/chromium/cc/CCKeyframedAnimationCurve.cpp:59
> +PassOwnPtr<CCTimingFunction> cloneTimingFunction(const CCTimingFunction* timingFunction)

This smells funny that you're allowed to clone a null timing function. I'd rather ASSERT(timingFunction) and guard against null in the callers.

> Source/WebCore/platform/graphics/chromium/cc/CCKeyframedAnimationCurve.cpp:64
> +    OwnPtr<CCAnimationCurve> curve = timingFunction->clone();

Virtuals can override return type and save you this mess, or so people far smarter than I tell me.

> Source/WebCore/platform/graphics/chromium/cc/CCKeyframedAnimationCurve.cpp:112
> +    return CCFloatKeyframe::create(time(), value(), cloneTimingFunction(timingFunction()));

avoid passing null to the clone, i think

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:77
> +                // FIXME: add support for steps timing function.

is this noted on the "modes we need to eventually support"?

Can this be implemented as a specific set of cubic bezier segments?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:82
> +            case TimingFunction::CubicBezierFunction: {

Is this double-bracing in keeping with wk style? Smells funny.

> Source/WebKit/chromium/tests/CCKeyframedAnimationCurveTest.cpp:218
>  }

do we need tests for the cubic bezier curve itself?

Created attachment 129374 [details]
Patch

(In reply to comment #2)
> (From update of attachment 129305 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129305&action=review
> 
> Dumb question --- if we had N-step cubic bezier curves, could we convert this bizarreness with the per-frame 
My goal was to closely match http://www.w3.org/TR/css3-animations/. In this document, each keyframe save the last one can be associated with a timing function, and I've tried to do the same thing. This approach also simplifies CCKeyframed*AnimationCurve -- I just have to resample using the timing function before interpolating two keyframes.
> 
> Can you pull out the const-correctness changes to UnitBezier to a separate patch?
Done.
> 
> Can we call CCTimingFunction -> CCTimingCurve?
> 
It does clash a bit with the other *Curve class names, but I was trying to match http://www.w3.org/TR/css3-animations/ which uses this jargon.
> > Source/WebCore/platform/graphics/chromium/cc/CCKeyframedAnimationCurve.cpp:-47
> > -    if (!keyframes.size())
> 
> can we at least put an early-check for the keyframe being at the end to make the common case fast?
> 
Done.
> > Source/WebCore/platform/graphics/chromium/cc/CCKeyframedAnimationCurve.cpp:47
> > +    OwnPtr<Keyframe> popKeyframe = keyframe;
> 
> also, do we have unit tests that insert keyframes out of order and verify that the keyframes are sorted?
> 
Added.
> > Source/WebCore/platform/graphics/chromium/cc/CCKeyframedAnimationCurve.cpp:59
> > +PassOwnPtr<CCTimingFunction> cloneTimingFunction(const CCTimingFunction* timingFunction)
> 
> This smells funny that you're allowed to clone a null timing function. I'd rather ASSERT(timingFunction) and guard against null in the callers.
> 
Done.
> > Source/WebCore/platform/graphics/chromium/cc/CCKeyframedAnimationCurve.cpp:64
> > +    OwnPtr<CCAnimationCurve> curve = timingFunction->clone();
> 
> Virtuals can override return type and save you this mess, or so people far smarter than I tell me.
> 
I would love to do this, but it turns out that covariant return type tricks won't work with smart pointers :( http://stackoverflow.com/questions/5969996/covariant-return-type-with-template-container
> > Source/WebCore/platform/graphics/chromium/cc/CCKeyframedAnimationCurve.cpp:112
> > +    return CCFloatKeyframe::create(time(), value(), cloneTimingFunction(timingFunction()));
> 
> avoid passing null to the clone, i think
Done.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:77
> > +                // FIXME: add support for steps timing function.
> 
> is this noted on the "modes we need to eventually support"?
> 
Wasn't mentioned here http://www.w3.org/TR/css3-animations/#animation-timing-function_tag, so I didn't include in the list.

> Can this be implemented as a specific set of cubic bezier segments?
Yes, that's a possibility.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:82
> > +            case TimingFunction::CubicBezierFunction: {
> 
> Is this double-bracing in keeping with wk style? Smells funny.
> 
Removed.
> > Source/WebKit/chromium/tests/CCKeyframedAnimationCurveTest.cpp:218
> >  }
> 
> do we need tests for the cubic bezier curve itself?
I didn't think so since it's implemented in terms of UnitBezier. I have a very basic test in CCKeyframedAnimationCurveTest that checks that the curve is nonlinear when it should be and that it takes on sane values.

[James Robinson]

Comment on attachment 129374 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129374&action=review

R=me. Several naming nits cos that's how we roll :)

> Source/WebCore/platform/graphics/chromium/cc/CCKeyframedAnimationCurve.cpp:47
> +void insertKeyframe(PassOwnPtr<Keyframe> keyframe, Vector<OwnPtr<Keyframe> >& keyframes)
>  {
> -    if (!keyframes.size())
> -        return true;
> -
> -    for (size_t i = 0; i < keyframes.size() - 1; ++i) {
> -        if (keyframes[i].time > keyframes[i+1].time)
> -            return false;
> +    OwnPtr<Keyframe> popKeyframe = keyframe;

nit: the naming here is reversed from what we normally do, although I think the logic is correct. the normal idiom is to name the parameter of type PassOwnPtr<> "popFoo" and then create a local called "foo" and never use the PassOwnPtr<> value again. http://www.webkit.org/coding/RefPtr.html has an example using PassRefPtr. this looks like the code is manipulating the PassOwnPtr<> value, which doesn't work since it's nulled out after assignment into a local OwnPtr<>

would you mind swapping the names?

> Source/WebCore/platform/graphics/chromium/cc/CCTimingFunction.cpp:32
> +const double kEpsilon = 1e-6;

nit: in WebKit, constants are named the same way as other variables - there's no "k" prefix

> Source/WebCore/platform/graphics/chromium/cc/CCTimingFunction.cpp:75
> +PassOwnPtr<CCTimingFunction> CCEaseTimingFunction::create()

nit: it probably wouldn't hurt to put a link to the section of the CSS spec that defines these seemingly magical numbers (I see you have one in the header)

> Source/WebCore/platform/graphics/chromium/cc/CCTimingFunction.h:53
> +    virtual float getValue(double t) const;

nit: in headers please expand the variable name out to 'time' just to be extra clear

Created attachment 129492 [details]
Patch

[WebKit Review Bot]

Comment on attachment 129492 [details]
Patch

Clearing flags on attachment: 129492

Committed r109297: <http://trac.webkit.org/changeset/109297>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.