[webkit-reviews] review granted: [Bug 20368] Implement computed style for animation properties : [Attachment 25322] Patch, testcases, changelog

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 20368: Implement computed style for animation properties
https://bugs.webkit.org/show_bug.cgi?id=20368

Attachment 25322: Patch, testcases, changelog
https://bugs.webkit.org/attachment.cgi?id=25322&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   case CSSPropertyWebkitAnimationDelay: {
> +	       RefPtr<CSSValueList> list =
CSSValueList::createCommaSeparated();
> +	       const AnimationList* t = style->animations();
> +	       if (t) {
> +		   for (size_t i = 0; i < t->size(); ++i)
> +		      
list->append(CSSPrimitiveValue::create(t->animation(i)->delay(),
CSSPrimitiveValue::CSS_S));
> +	       } else
> +		  
list->append(CSSPrimitiveValue::create(RenderStyle::initialAnimationDelay(),
CSSPrimitiveValue::CSS_S));
> +	       return list.release();
> +	   }

These seem like just enough code that it would be better to have them in
separate functions rather than in the giant switch statement.

However, that problem would solve itself if we changed this into a table
instead of a switch, so I guess it can wait for that.

Where does the name "t" come from?

> +		   for (size_t i = 0; i < t->size(); ++i) {
> +		      
list->append(CSSPrimitiveValue::create(t->animation(i)->name(),
CSSPrimitiveValue::CSS_STRING));
> +		   }

Should be no braces here.

> +		       int prop = t->animation(i)->property();

The type of this local variable, and of the property-related functions in
Animation, should be CSSPropertyID, not int.

> +			   propertyValue =
CSSPrimitiveValue::create(getPropertyName(static_cast<CSSPropertyID>(prop)),
CSSPrimitiveValue::CSS_STRING);

Because we don't want typecasts like this one.

r=me