[webkit-reviews] review denied: [Bug 105035] Implement CSS computed style value for transition shorthand : [Attachment 181734] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has denied Alexis Menard (darktears)
<alexis at webkit.org>'s request for review:
Bug 105035: Implement CSS computed style value for transition shorthand
https://bugs.webkit.org/show_bug.cgi?id=105035

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

------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=181734&action=review


All good. Just small changes and I'll r+ cq+

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1051
> +{

I wonder if this should be called createTransitionPropertyValue since it will
never be used for animations (yeah, our naming is a bit confusing).

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1061
> +static PassRefPtr<CSSValue> getAnimationPropertyValue(const AnimationList*
animList)

Ditto.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2490
> +	   case CSSPropertyWebkitTransitionProperty:
> +	       return getAnimationPropertyValue(style->transitions());

This is where I noticed the confusing name. It's even more strange because
style->transitions() returns an AnimationList* :)

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2503
> +		       RefPtr<CSSValueList> spaceList =
CSSValueList::createSpaceSeparated();
> +		      
spaceList->append(createAnimationPropertyValue(animList->animation(i)));
> +		      
spaceList->append(cssValuePool().createValue(animList->animation(i)->duration()
, CSSPrimitiveValue::CSS_S));
> +		      
spaceList->append(createTimingFunctionValue(animList->animation(i)->timingFunct
ion().get()));
> +		      
spaceList->append(cssValuePool().createValue(animList->animation(i)->delay(),
CSSPrimitiveValue::CSS_S));
> +		       list->append(spaceList);

I suggest you replace "spaceList" with "list" (like you do below). Also, get a
local variable for animList->animation(i) and reuse it through here.

> LayoutTests/transitions/transitions-parsing.html:27
> +
> +

Nit, extra blank line.