[webkit-reviews] review denied: [Bug 105442] Querying transition-timing-function value on the computed style does not return keywords when it should. : [Attachment 181027] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jan 2 20:57:54 PST 2013


Simon Fraser (smfr) <simon.fraser at apple.com> has denied Alexis Menard
(darktears) <alexis at webkit.org>'s request for review:
Bug 105442: Querying transition-timing-function value on the computed style
does not return keywords when it should.
https://bugs.webkit.org/show_bug.cgi?id=105442

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=181027&action=review


> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1079
> +static PassRefPtr<CSSValue> createAnimationValue(const TimingFunction* tf)
> +{
> +    if (tf->isCubicBezierTimingFunction()) {

You should expand 'tf' to timingFunction.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1080
> +	   const CubicBezierTimingFunction* ctf = static_cast<const
CubicBezierTimingFunction*>(tf);

Avoid abbreviations like ctf.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1082
> +	       CSSValueID id = CSSValueInvalid;

Don't use 'id' (it's a reserved word in Objective-C).

> Source/WebCore/css/CSSToStyleMap.cpp:460
> -	      
animation->setTimingFunction(CubicBezierTimingFunction::create(0.42, 0.0, 1.0,
1.0));
> +	      
animation->setTimingFunction(CubicBezierTimingFunction::create(CubicBezierTimin
gFunction::EaseIn, 0.42, 0.0, 1.0, 1.0));

Seems redundant to pass in both the enum, and the values that it corresponds
to. Would be better to have an overloaded CubicBezierTimingFunction::create()
and ctor that takes the enum only.

> Source/WebCore/platform/animation/TimingFunction.h:105
> +	       return m_x1 == ctf->m_x1 && m_y1 == ctf->m_y1 && m_x2 ==
ctf->m_x2 && m_y2 == ctf->m_y2 && m_cubicBezierTimingFunctionDescription ==
ctf->m_cubicBezierTimingFunctionDescription;

Can't you test the type first; you only need to test the values for custom
functions.


More information about the webkit-reviews mailing list