[webkit-reviews] review granted: [Bug 108751] WebKit shouldn't accept "none, none" in transition shorthand property. : [Attachment 188585] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Feb 15 10:21:21 PST 2013


Dean Jackson <dino at apple.com> has granted Alexis Menard (darktears)
<alexis at webkit.org>'s request for review:
Bug 108751: WebKit shouldn't accept "none, none" in transition shorthand
property.
https://bugs.webkit.org/show_bug.cgi?id=108751

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

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


r+ but I think we need to clarify the names before landing.

> Source/WebCore/ChangeLog:23
> +	   (WebCore::AnimationParseContext::hasFirstAnimationCommitted):

I suggest "hasCommittedFirstAnimation" (getter)

> Source/WebCore/ChangeLog:28
> +	  
(WebCore::AnimationParseContext::markParsingAnimationPropertyFinished):
> +	   In the shorthand as soon as a keyword has been found then the
parsing
> +	   is 'finished', if any other animation/transition declaration part of

> +	   the shorthand are with a keyword then it's invalid.
> +	   (WebCore::AnimationParseContext::canParseMoreAnimationProperty):

This one is a tricky name, because it is so similar to the one below, but is
only used within shorthands :(

How about "canParseAnotherAnimationPropertyKeywordInShorthand" (getter) and
"animationPropertyKeywordParsedInShorthand" (setter).
Although I don't like that this has flipped the logic from your existing code,
and also that the getter returns the opposite of the setter (if that makes
sense).

> Source/WebCore/ChangeLog:30
> +	   (WebCore::AnimationParseContext::hasAnimationPropertyKeyword):
> +	   (WebCore::AnimationParseContext::commitAnimationPropertyKeyword):

How about "hasSeenAnimationPropertyKeyword" (getter) and
"sawAnimationPropertyKeyword" (setter)?


More information about the webkit-reviews mailing list