[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