Bug 44541 - Implement steps() timing function for animations
: Implement steps() timing function for animations
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: All Mac OS X 10.5
: P2 Normal
Assigned To: Dean Jackson
http://dev.w3.org/csswg/css3-transiti...
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-24 11:41 PDT by Dean Jackson
Modified: 2010-09-08 17:10 PDT (History)
10 users (show)

See Also:


Attachments
Patch (59.64 KB, patch)
2010-09-08 04:44 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (57.93 KB, patch)
2010-09-08 14:17 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (57.96 KB, patch)
2010-09-08 14:42 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (63.19 KB, patch)
2010-09-08 15:35 PDT, Dean Jackson
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2010-08-24 11:41:11 PDT
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.
Comment 1 Dean Jackson 2010-09-08 04:44:18 PDT
Created attachment 66880 [details]
Patch
Comment 2 Dean Jackson 2010-09-08 04:46:00 PDT
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.
Comment 3 Simon Fraser (smfr) 2010-09-08 09:49:15 PDT
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.
Comment 4 Dean Jackson 2010-09-08 14:11:37 PDT
(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.
Comment 5 Dean Jackson 2010-09-08 14:17:37 PDT
Created attachment 66941 [details]
Patch
Comment 6 WebKit Review Bot 2010-09-08 14:20:25 PDT
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.
Comment 7 Dean Jackson 2010-09-08 14:42:16 PDT
Created attachment 66943 [details]
Patch
Comment 8 WebKit Review Bot 2010-09-08 14:47:56 PDT
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.
Comment 9 Early Warning System Bot 2010-09-08 14:50:58 PDT
Attachment 66941 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3923288
Comment 10 Eric Seidel 2010-09-08 14:53:17 PDT
Attachment 66941 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3937320
Comment 11 Dean Jackson 2010-09-08 15:35:22 PDT
Created attachment 66950 [details]
Patch

Testing build
Comment 12 Dean Jackson 2010-09-08 16:07:02 PDT
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
Comment 13 Dean Jackson 2010-09-08 16:07:53 PDT
Comment on attachment 66950 [details]
Patch

r=smfr
Comment 14 Early Warning System Bot 2010-09-08 16:08:50 PDT
Attachment 66950 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3955288
Comment 15 Eric Seidel 2010-09-08 16:12:57 PDT
Attachment 66950 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3973271
Comment 16 WebKit Review Bot 2010-09-08 16:16:48 PDT
http://trac.webkit.org/changeset/67032 might have broken Qt Linux Release minimal
Comment 17 Dean Jackson 2010-09-08 17:10:02 PDT
two subsequent build fix commits