[webkit-reviews] review denied: [Bug 93210] CSSParser: Move enumeration to a common place (StylePropertyShorthand) : [Attachment 160151] Forgot a last unneeded namespace reference
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Aug 23 11:48:26 PDT 2012
Dean Jackson <dino at apple.com> has denied Adenilson Cavalcanti Silva
<savagobr at yahoo.com>'s request for review:
Bug 93210: CSSParser: Move enumeration to a common place
(StylePropertyShorthand)
https://bugs.webkit.org/show_bug.cgi?id=93210
Attachment 160151: Forgot a last unneeded namespace reference
https://bugs.webkit.org/attachment.cgi?id=160151&action=review
------- Additional Comments from Dean Jackson <dino at apple.com>
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.
More information about the webkit-reviews
mailing list