[Webkit-unassigned] [Bug 93210] CSSParser: Move enumeration to a common place (StylePropertyShorthand)

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=93210


Dean Jackson <dino at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #160151|review?                     |review-
               Flag|                            |




--- Comment #12 from Dean Jackson <dino at apple.com>  2012-08-23 11:48:25 PST ---
(From update of attachment 160151)
View in context: https://bugs.webkit.org/attachment.cgi?id=160151&action=review

Sorry, it's so close! Just some minor nits.

> Source/WebCore/ChangeLog:10
> +        CSSParser::parseAnimationShorthand() uses an enumeration with the same elements as another enumeration
> +        present in StylePropertyShorthand, but with different ordering of values. It seems to make sense to
> +        have both enums in the same place.

"It seems to make sense to have both enums in the same place." -> "Put both enums in the same place".

> Source/WebCore/css/StylePropertyShorthand.cpp:271
> +    // When we parse the animation shorthand we need to look for animation-name
> +    // last because otherwise it might match against the keywords for fill mode,
> +    // timing functions and infinite iteration. This means that animation names
> +    // that are the same as keywords (e.g. 'forwards') won't always match in the
> +    // shorthand. In that case they should be using longhands (or reconsidering
> +    // their approach). This is covered by the animations spec bug:

This was my fault in the original patch, but could you change "In that case they..." to "In that case authors..." to make it more clear?

> Source/WebCore/css/StylePropertyShorthand.cpp:286
> +        CSSPropertyWebkitAnimationName
> +    };
> +
> +
> +    DEFINE_STATIC_LOCAL(StylePropertyShorthand, webkitAnimationLonghandsNameLast, (animationPropertiesNameLast, WTF_ARRAY_LENGTH(animationPropertiesNameLast)));

Extra blank line above.

> Source/WebCore/css/StylePropertyShorthand.h:83
>  const StylePropertyShorthand& webkitAnimationShorthand();
> +const StylePropertyShorthand& webkitAnimationShorthandNameLast();

I thought about this a bit more and I think the name should be webkitAnimationShorthandForParsing(). It makes it clear that there is only one reason why we have this duplication.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.