[webkit-reviews] review denied: [Bug 44541] Implement steps() timing function for animations : [Attachment 66880] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Dean Jackson
<dino at apple.com>'s request for review:
Bug 44541: Implement steps() timing function for animations
https://bugs.webkit.org/show_bug.cgi?id=44541

Attachment 66880: Patch
https://bugs.webkit.org/attachment.cgi?id=66880&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> 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(cubicTimingFunct
ion->x1(), cubicTimingFunction->y1(), cubicTimingFunction->x2(),
cubicTimingFunction->y2()));
> +	   } else if (timingFunction->isStepsTimingFunctionValue()) {
> +	       CSSStepsTimingFunctionValue* stepsTimingFunction =
static_cast<CSSStepsTimingFunctionValue*>(value);
> +	      
animation->setTimingFunction(StepsTimingFunction::create(stepsTimingFunction->n
umberOfSteps(), 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.