Bug 44541 – Implement steps() timing function for animations

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Dean Jackson]

Implement step-start, step-end, steps() as defined in http://dev.w3.org/csswg/css3-transitions/#transition-timing-function_tag

Since CSSTimingFunction assumes it is a bezier, I suspect we'll have to make a base class and a couple of isBezierFunction() / isStepFunction() queries, similar to Events.

[Dean Jackson]

Created attachment 66880 [details]
Patch

[Dean Jackson]

Note that the patch also includes the small change in WebCore/rendering/RenderLayerBacking.cpp that was causing hardware animations to not run. This will be removed before submission.

[Simon Fraser (smfr)]

Comment on attachment 66880 [details]
Patch

> diff --git a/WebCore/WebCore.xcodeproj/project.pbxproj b/WebCore/WebCore.xcodeproj/project.pbxproj

>  			compatibilityVersion = "Xcode 2.4";
> +			developmentRegion = English;

Please don't commit this.


> @@ -6079,7 +6085,13 @@ void CSSStyleSelector::mapAnimationTimingFunction(Animation* animation, CSSValue
>      
>      if (value->isTimingFunctionValue()) {
>          CSSTimingFunctionValue* timingFunction = static_cast<CSSTimingFunctionValue*>(value);
> -        animation->setTimingFunction(TimingFunction(CubicBezierTimingFunction, timingFunction->x1(), timingFunction->y1(), timingFunction->x2(), timingFunction->y2()));
> +        if (timingFunction->isCubicBezierTimingFunctionValue()) {
> +            CSSCubicBezierTimingFunctionValue* cubicTimingFunction = static_cast<CSSCubicBezierTimingFunctionValue*>(value);
> +            animation->setTimingFunction(CubicBezierTimingFunction::create(cubicTimingFunction->x1(), cubicTimingFunction->y1(), cubicTimingFunction->x2(), cubicTimingFunction->y2()));
> +        } else if (timingFunction->isStepsTimingFunctionValue()) {
> +            CSSStepsTimingFunctionValue* stepsTimingFunction = static_cast<CSSStepsTimingFunctionValue*>(value);
> +            animation->setTimingFunction(StepsTimingFunction::create(stepsTimingFunction->numberOfSteps(), stepsTimingFunction->stepAtStart()));
> +        }

Why aren't linear timing functions handled here?

  
> diff --git a/WebCore/css/CSSTimingFunctionValue.cpp b/WebCore/css/CSSTimingFunctionValue.cpp

> +String CSSLinearTimingFunctionValue::cssText() const
> +{
> +    String text("linear");
> +    return text;

return "linear" would work.

> diff --git a/WebCore/css/CSSTimingFunctionValue.h b/WebCore/css/CSSTimingFunctionValue.h

> +class CSSLinearTimingFunctionValue : public CSSTimingFunctionValue {
> +public:

> +    virtual String cssText() const;
> +    
> +    virtual bool isLinearTimingFunctionValue() const { return true; }

Darin would argue that these two methods should be private.

> +class CSSCubicBezierTimingFunctionValue : public CSSTimingFunctionValue {

> +    virtual String cssText() const;

Ditto for this.

> +    
> +    virtual bool isCubicBezierTimingFunctionValue() const { return true; }

Ditto for this.

>  private:
> -    CSSTimingFunctionValue(double x1, double y1, double x2, double y2)
> -        : m_x1(x1)
> -        , m_y1(y1)
> -        , m_x2(x2)
> -        , m_y2(y2)
> +    CSSCubicBezierTimingFunctionValue(double x1, double y1, double x2, double y2)
> +    : m_x1(x1)
> +    , m_y1(y1)
> +    , m_x2(x2)
> +    , m_y2(y2)

The old indentation was correct.

> +class CSSStepsTimingFunctionValue : public CSSTimingFunctionValue {
> +    virtual String cssText() const;

Private?

> +    virtual bool isStepsTimingFunctionValue() const { return true; }

Private.

> +private:
> +    CSSStepsTimingFunctionValue(int steps, bool stepAtStart)
> +    : m_steps(steps)
> +    , m_stepAtStart(stepAtStart)

Indent these.

> diff --git a/WebCore/platform/animation/Animation.h b/WebCore/platform/animation/Animation.h

> -    const TimingFunction& timingFunction() const { return m_timingFunction; }
> +    const PassRefPtr<TimingFunction> timingFunction() const { return m_timingFunction; }

This should return a raw pointer. You're not transferring ownership.

I'm not convinced that TimingFunctions need to be refcounted.They can use OwnPtrs etc.

> diff --git a/WebCore/platform/animation/TimingFunction.h b/WebCore/platform/animation/TimingFunction.h

> +class TimingFunction : public RefCounted<TimingFunction> {

Again, no need to refcount here either.

> +    enum TimingFunctionType {
> +        LINEAR_FUNCTION, CUBIC_BEZIER_FUNCTION, STEPS_FUNCTION
> +    };

We don't normally use uppercase for enums.

> +    CubicBezierTimingFunction()
> +        : TimingFunction(CUBIC_BEZIER_FUNCTION)
> +        , m_x1(0.25)
> +        , m_y1(0.1)
> +        , m_x2(0.25)
> +        , m_y2(1.0)
> +    {
> +    }
> +
> +    CubicBezierTimingFunction(double x1, double y1, double x2, double y2)
> +        : TimingFunction(CUBIC_BEZIER_FUNCTION)
> +        , m_x1(x1)
> +        , m_y1(y1)
> +        , m_x2(x2)
> +        , m_y2(y2)
> +    {
> +    }

You could just provide default arguments and have a single ctor. It bugs me that the magic numbers are here and in the CSS code. I wonder if the CSS/style timing fucntions sbould just wrap these?

> +    static const StepsTimingFunction* cast(const TimingFunction* timingFunction)
> +    {
> +        return timingFunction->isStepsTimingFunction()
> +            ? static_cast<const StepsTimingFunction*>(timingFunction)
> +            : 0;
> +    }

This is a bit weird.We do stuff like this with toRenderBox etc, so maybe toStepsTimingFunction()?

> diff --git a/WebCore/platform/graphics/GraphicsLayer.h b/WebCore/platform/graphics/GraphicsLayer.h

>  class AnimationValue : public Noncopyable {
>  public:
> -    AnimationValue(float keyTime, const TimingFunction* timingFunction = 0)
> +    AnimationValue(float keyTime, PassRefPtr<TimingFunction> timingFunction = 0)
>          : m_keyTime(keyTime)
>      {
>          if (timingFunction)
> -            m_timingFunction = adoptPtr(new TimingFunction(*timingFunction));
> +            m_timingFunction = timingFunction;

If timing functions are now RefCounted, then you could use OwnPtr/PassOwnPtr here.

> -static CAMediaTimingFunction* getCAMediaTimingFunction(const TimingFunction& timingFunction)
> +static CAMediaTimingFunction* getCAMediaTimingFunction(const TimingFunction* timingFunction)

> +    // By this point, timing functions can only be linear or cubic, not steps.

Add assertion?

> +    if (timingFunction->isCubicBezierTimingFunction()) {
> +        const CubicBezierTimingFunction* ctf = static_cast<const CubicBezierTimingFunction*>(timingFunction);
> +        return [CAMediaTimingFunction functionWithControlPoints:static_cast<float>(ctf->x1()) :static_cast<float>(ctf->y1())
> +                                                               :static_cast<float>(ctf->x2()) :static_cast<float>(ctf->y2())];
> +    } else
> +        return [CAMediaTimingFunction functionWithName:@"linear"];

> diff --git a/WebCore/rendering/RenderLayerBacking.cpp b/WebCore/rendering/RenderLayerBacking.cpp

>          // get timing function
> -        const TimingFunction* tf = keyframeStyle->hasAnimations() ? &((*keyframeStyle->animations()).animation(0)->timingFunction()) : 0;
> +        RefPtr<TimingFunction> tf = keyframeStyle->hasAnimations() ? (*keyframeStyle->animations()).animation(0)->timingFunction() : 0;

Do you really need a RefPtr here? You can use a bare pointer I think.

> -        if (currentKeyframe.containsProperty(AnimatedPropertyWebkitTransform))
> +        if (currentKeyframe.containsProperty(CSSPropertyWebkitTransform))
>              transformVector.insert(new TransformAnimationValue(key, &(keyframeStyle->transform()), tf));
>          
> -        if (currentKeyframe.containsProperty(AnimatedPropertyOpacity))
> +        if (currentKeyframe.containsProperty(CSSPropertyOpacity))
>              opacityVector.insert(new FloatAnimationValue(key, keyframeStyle->opacity(), tf));

These changes are in TOT already, as you mention.

r- for bad use of PassRefPtr in style code, possibly missing linear block, and consideration of making TimingFunction not refcounted.

[Dean Jackson]

(In reply to comment #3)
> 
> > @@ -6079,7 +6085,13 @@ void CSSStyleSelector::mapAnimationTimingFunction(Animation* animation, CSSValue
> >      
> >      if (value->isTimingFunctionValue()) {
> >          CSSTimingFunctionValue* timingFunction = static_cast<CSSTimingFunctionValue*>(value);
> > -        animation->setTimingFunction(TimingFunction(CubicBezierTimingFunction, timingFunction->x1(), timingFunction->y1(), timingFunction->x2(), timingFunction->y2()));
> > +        if (timingFunction->isCubicBezierTimingFunctionValue()) {
> > +            CSSCubicBezierTimingFunctionValue* cubicTimingFunction = static_cast<CSSCubicBezierTimingFunctionValue*>(value);
> > +            animation->setTimingFunction(CubicBezierTimingFunction::create(cubicTimingFunction->x1(), cubicTimingFunction->y1(), cubicTimingFunction->x2(), cubicTimingFunction->y2()));
> > +        } else if (timingFunction->isStepsTimingFunctionValue()) {
> > +            CSSStepsTimingFunctionValue* stepsTimingFunction = static_cast<CSSStepsTimingFunctionValue*>(value);
> > +            animation->setTimingFunction(StepsTimingFunction::create(stepsTimingFunction->numberOfSteps(), stepsTimingFunction->stepAtStart()));
> > +        }
> 
> Why aren't linear timing functions handled here?

Because the only way to get a linear function is via the keyword "linear" which is handled right above.

> 
> 
> > diff --git a/WebCore/css/CSSTimingFunctionValue.cpp b/WebCore/css/CSSTimingFunctionValue.cpp
> 
> > +String CSSLinearTimingFunctionValue::cssText() const
> > +{
> > +    String text("linear");
> > +    return text;
> 
> return "linear" would work.

done.

> 
> > diff --git a/WebCore/css/CSSTimingFunctionValue.h b/WebCore/css/CSSTimingFunctionValue.h
> 
> > +class CSSLinearTimingFunctionValue : public CSSTimingFunctionValue {
> > +public:
> 
> > +    virtual String cssText() const;
> > +    
> > +    virtual bool isLinearTimingFunctionValue() const { return true; }
> 
> Darin would argue that these two methods should be private.

done... and for the other cases you mentioned.

> >  private:
> > -    CSSTimingFunctionValue(double x1, double y1, double x2, double y2)
> > -        : m_x1(x1)
> > -        , m_y1(y1)
> > -        , m_x2(x2)
> > -        , m_y2(y2)
> > +    CSSCubicBezierTimingFunctionValue(double x1, double y1, double x2, double y2)
> > +    : m_x1(x1)
> > +    , m_y1(y1)
> > +    , m_x2(x2)
> > +    , m_y2(y2)
> 
> The old indentation was correct.

Fixed.

> > +private:
> > +    CSSStepsTimingFunctionValue(int steps, bool stepAtStart)
> > +    : m_steps(steps)
> > +    , m_stepAtStart(stepAtStart)
> 
> Indent these.

Fixed.

> 
> > diff --git a/WebCore/platform/animation/Animation.h b/WebCore/platform/animation/Animation.h
> 
> > -    const TimingFunction& timingFunction() const { return m_timingFunction; }
> > +    const PassRefPtr<TimingFunction> timingFunction() const { return m_timingFunction; }
> 
> This should return a raw pointer. You're not transferring ownership.

We do make a copy in RenderLayerBacking::startAnimation when creating an AnimationValue.

> I'm not convinced that TimingFunctions need to be refcounted.They can use OwnPtrs etc.

After discussion on IRC, we came back to refcounted. 

> > +    enum TimingFunctionType {
> > +        LINEAR_FUNCTION, CUBIC_BEZIER_FUNCTION, STEPS_FUNCTION
> > +    };
> 
> We don't normally use uppercase for enums.

Fixed.

> 
> > +    CubicBezierTimingFunction()
> > +        : TimingFunction(CUBIC_BEZIER_FUNCTION)
> > +        , m_x1(0.25)
> > +        , m_y1(0.1)
> > +        , m_x2(0.25)
> > +        , m_y2(1.0)
> > +    {
> > +    }
> > +
> > +    CubicBezierTimingFunction(double x1, double y1, double x2, double y2)
> > +        : TimingFunction(CUBIC_BEZIER_FUNCTION)
> > +        , m_x1(x1)
> > +        , m_y1(y1)
> > +        , m_x2(x2)
> > +        , m_y2(y2)
> > +    {
> > +    }
> 
> You could just provide default arguments and have a single ctor. It bugs me that the magic numbers are here and in the CSS code. I wonder if the CSS/style timing fucntions sbould just wrap these?

There is only one copy of the numbers, but you're right in that some of them are in the style code. The problem is that initialTimingFunction also needs to create a default version, so it would have to be in two places again. Maybe we should make createEaseIn() type methods, so the code is always here?

Anyway, for this I removed the empty constructor and added default values to the create(x1, y1, x2, y2) call.

> 
> > +    static const StepsTimingFunction* cast(const TimingFunction* timingFunction)
> > +    {
> > +        return timingFunction->isStepsTimingFunction()
> > +            ? static_cast<const StepsTimingFunction*>(timingFunction)
> > +            : 0;
> > +    }
> 
> This is a bit weird.We do stuff like this with toRenderBox etc, so maybe toStepsTimingFunction()?

I copied this approach from somewhere else, but it turns out it isn't really needed. The only client for cast() was the operator== so I simply moved the code in there.

> 
> > -static CAMediaTimingFunction* getCAMediaTimingFunction(const TimingFunction& timingFunction)
> > +static CAMediaTimingFunction* getCAMediaTimingFunction(const TimingFunction* timingFunction)
> 
> > +    // By this point, timing functions can only be linear or cubic, not steps.
> 
> Add assertion?

Done

> 
> > diff --git a/WebCore/rendering/RenderLayerBacking.cpp b/WebCore/rendering/RenderLayerBacking.cpp
> 
> >          // get timing function
> > -        const TimingFunction* tf = keyframeStyle->hasAnimations() ? &((*keyframeStyle->animations()).animation(0)->timingFunction()) : 0;
> > +        RefPtr<TimingFunction> tf = keyframeStyle->hasAnimations() ? (*keyframeStyle->animations()).animation(0)->timingFunction() : 0;
> 
> Do you really need a RefPtr here? You can use a bare pointer I think.

The problem was that AnimationValue makes a copy of this timing function. Now that TimingFunction is a base class, it can't easily clone, so making another reference is ok.

[Dean Jackson]

Created attachment 66941 [details]
Patch

[WebKit Review Bot]

Attachment 66941 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/animation/Animation.cpp:114:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Created attachment 66943 [details]
Patch

[WebKit Review Bot]

Attachment 66943 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/animation/Animation.cpp:114:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 66941 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3923288

[Eric Seidel (no email)]

Attachment 66941 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3937320

[Dean Jackson]

Created attachment 66950 [details]
Patch

Testing build

[Dean Jackson]

Committed. Fingers crossed I didn't break QT.

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/animations/animation-shorthand.html
	M	LayoutTests/animations/computed-style-expected.txt
	M	LayoutTests/animations/computed-style.html
	M	LayoutTests/animations/fill-unset-properties.html
	A	LayoutTests/animations/timing-functions-expected.txt
	A	LayoutTests/animations/timing-functions.html
	M	LayoutTests/transitions/inherit-other-props-expected.txt
	M	LayoutTests/transitions/inherit-other-props.html
	A	LayoutTests/transitions/steps-timing-function-expected.txt
	A	LayoutTests/transitions/steps-timing-function.html
	M	WebCore/ChangeLog
	M	WebCore/css/CSSComputedStyleDeclaration.cpp
	M	WebCore/css/CSSParser.cpp
	M	WebCore/css/CSSParser.h
	M	WebCore/css/CSSStyleSelector.cpp
	M	WebCore/css/CSSTimingFunctionValue.cpp
	M	WebCore/css/CSSTimingFunctionValue.h
	M	WebCore/css/CSSValueKeywords.in
	M	WebCore/page/animation/AnimationBase.cpp
	M	WebCore/page/animation/KeyframeAnimation.cpp
	M	WebCore/platform/animation/Animation.cpp
	M	WebCore/platform/animation/Animation.h
	M	WebCore/platform/animation/TimingFunction.h
	M	WebCore/platform/graphics/GraphicsLayer.h
	M	WebCore/platform/graphics/mac/GraphicsLayerCA.mm
	M	WebCore/platform/graphics/qt/GraphicsLayerQt.cpp
	M	WebCore/rendering/RenderLayerBacking.cpp
	M	WebCore/rendering/style/RenderStyleConstants.h
Committed r67032

[Dean Jackson]

Comment on attachment 66950 [details]
Patch

r=smfr

[Early Warning System Bot]

Attachment 66950 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3955288

[Eric Seidel (no email)]

Attachment 66950 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3973271

[WebKit Review Bot]

http://trac.webkit.org/changeset/67032 might have broken Qt Linux Release minimal

[Dean Jackson]

two subsequent build fix commits