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


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-08-24 11:41:11 PST
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 From 2010-09-08 04:44:18 PST -------
Created an attachment (id=66880) [details]
Patch
------- Comment #2 From 2010-09-08 04:46:00 PST -------
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 From 2010-09-08 09:49:15 PST -------
(From update of attachment 66880 [details])
> 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 From 2010-09-08 14:11:37 PST -------
(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 From 2010-09-08 14:17:37 PST -------
Created an attachment (id=66941) [details]
Patch
------- Comment #6 From 2010-09-08 14:20:25 PST -------
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 From 2010-09-08 14:42:16 PST -------
Created an attachment (id=66943) [details]
Patch
------- Comment #8 From 2010-09-08 14:47:56 PST -------
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 From 2010-09-08 14:50:58 PST -------
Attachment 66941 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3923288
------- Comment #10 From 2010-09-08 14:53:17 PST -------
Attachment 66941 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3937320
------- Comment #11 From 2010-09-08 15:35:22 PST -------
Created an attachment (id=66950) [details]
Patch

Testing build
------- Comment #12 From 2010-09-08 16:07:02 PST -------
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 From 2010-09-08 16:07:53 PST -------
(From update of attachment 66950 [details])
r=smfr
------- Comment #14 From 2010-09-08 16:08:50 PST -------
Attachment 66950 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3955288
------- Comment #15 From 2010-09-08 16:12:57 PST -------
Attachment 66950 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3973271
------- Comment #16 From 2010-09-08 16:16:48 PST -------
http://trac.webkit.org/changeset/67032 might have broken Qt Linux Release minimal
------- Comment #17 From 2010-09-08 17:10:02 PST -------
two subsequent build fix commits