Bug 44541

Summary: Implement steps() timing function for animations
Product: WebKit Reporter: Dean Jackson <dino>
Component: CSSAssignee: Dean Jackson <dino>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cmarrin, divya, eric, gianni, ml, peter, simon.fraser, webkit-ews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: OS X 10.5   
URL: http://dev.w3.org/csswg/css3-transitions/#transition-timing-function_tag
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch dino: review+

Dean Jackson
Reported 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.
Attachments
Patch (59.64 KB, patch)
2010-09-08 04:44 PDT, Dean Jackson
no flags
Patch (57.93 KB, patch)
2010-09-08 14:17 PDT, Dean Jackson
no flags
Patch (57.96 KB, patch)
2010-09-08 14:42 PDT, Dean Jackson
no flags
Patch (63.19 KB, patch)
2010-09-08 15:35 PDT, Dean Jackson
dino: review+
Dean Jackson
Comment 1 2010-09-08 04:44:18 PDT
Dean Jackson
Comment 2 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.
Simon Fraser (smfr)
Comment 3 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.
Dean Jackson
Comment 4 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.
Dean Jackson
Comment 5 2010-09-08 14:17:37 PDT
WebKit Review Bot
Comment 6 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.
Dean Jackson
Comment 7 2010-09-08 14:42:16 PDT
WebKit Review Bot
Comment 8 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.
Early Warning System Bot
Comment 9 2010-09-08 14:50:58 PDT
Eric Seidel (no email)
Comment 10 2010-09-08 14:53:17 PDT
Dean Jackson
Comment 11 2010-09-08 15:35:22 PDT
Created attachment 66950 [details] Patch Testing build
Dean Jackson
Comment 12 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
Dean Jackson
Comment 13 2010-09-08 16:07:53 PDT
Comment on attachment 66950 [details] Patch r=smfr
Early Warning System Bot
Comment 14 2010-09-08 16:08:50 PDT
Eric Seidel (no email)
Comment 15 2010-09-08 16:12:57 PDT
WebKit Review Bot
Comment 16 2010-09-08 16:16:48 PDT
http://trac.webkit.org/changeset/67032 might have broken Qt Linux Release minimal
Dean Jackson
Comment 17 2010-09-08 17:10:02 PDT
two subsequent build fix commits
Note You need to log in before you can comment on or make changes to this bug.