Bug 93210 – CSSParser: Move enumeration to a common place (StylePropertyShorthand)

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Adenilson Cavalcanti Silva]

Current code in CSSParser::parseAnimationShorthand() has an enumeration declared with the same elements as another enumeration declared in StylePropertyShorthand.

The issue is that this code depends on a different ordering of enum values (i.e. animationName) to avoid matching with other keywords like fill-mode/timing function, etc.

I considered copying the original values and reordering in a static variable, but it may cause performance regressions. For while, it seems to make sense to at least have both enums in the same place.

[Adenilson Cavalcanti Silva]

Created attachment 156575 [details]
Moving the enum to a common place

[Alexis Menard (darktears)]

Comment on attachment 156575 [details]
Moving the enum to a common place

I think this move is useless but just clutter the history. Secondly webkitAnimationShorthandWorkaround is a bad name as we don't workaround anything here, we just parse differently as the spec is unclear on how we should parse. I'd rather move this code/refactor/whatever when the w3c spec bug is resolved and the parsing clarified.

[Adenilson Cavalcanti Silva]

Alexis

Thanks for the comments and I agree that the patch is not good.

My question is:

a) is this duplication of enumerations a problem that should be fixed?

b) if so, what would be the best approach?

[Alexis Menard (darktears)]

(In reply to comment #3)
> Alexis
> 
> Thanks for the comments and I agree that the patch is not good.
> 
> My question is:
> 
> a) is this duplication of enumerations a problem that should be fixed?
>

In a way we should
 
> b) if so, what would be the best approach?

Wait to see the spec clarified to analyse the best way to fix it.

[Darin Adler]

Comment on attachment 156575 [details]
Moving the enum to a common place

View in context: https://bugs.webkit.org/attachment.cgi?id=156575&action=review

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

Idea is fine, but adding the word “workaround” does not express what’s different about this list. webkitAnimationShorthandWithAnimationNameLast might be OK.

[Adenilson Cavalcanti Silva]

Created attachment 159704 [details]
Following the suggestion about naming the function

Updated the patch to follow the suggestion about function name.

[Adenilson Cavalcanti Silva]

Darin

Thanks for the feedback, I agree that 'workaround' is not a good and proper name. The new patch has it fixed, would you mind to have a look on it again?


(In reply to comment #5)
> (From update of attachment 156575 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156575&action=review
> 
> > Source/WebCore/css/StylePropertyShorthand.h:83
> >  const StylePropertyShorthand& webkitAnimationShorthand();
> > +const StylePropertyShorthand& webkitAnimationShorthandWorkaround();
> 
> Idea is fine, but adding the word “workaround” does not express what’s different about this list. webkitAnimationShorthandWithAnimationNameLast might be OK.

[Dean Jackson]

Comment on attachment 159704 [details]
Following the suggestion about naming the function

View in context: https://bugs.webkit.org/attachment.cgi?id=159704&action=review

I wonder if webkitAnimationShorthandForParsing() is a better name? It makes it clear why we have this (annoying) duplication. I don't have a strong opinion either way.

Otherwise, just some small fixes needed before r+.

> Source/WebCore/css/CSSParser.cpp:3064
> +    const WebCore::StylePropertyShorthand& animationProperties = WebCore::webkitAnimationShorthandNameLast();

Why do you need the WebCore:: here?

> Source/WebCore/css/CSSParser.cpp:3071
> +    ASSERT(numProperties == webkitAnimationShorthandNameLast().length());
> +    ASSERT(webkitAnimationShorthand().length() == webkitAnimationShorthandNameLast().length());

You could compare to the animationProperties local variable.

> Source/WebCore/css/StylePropertyShorthand.cpp:275
> +    static const CSSPropertyID animationPropertiesLast[] = {

Why not animationPropertiesNameLast to be consistent?

[Adenilson Cavalcanti Silva]

Created attachment 160147 [details]
Implementing reviewers suggestion: naming + comparing enum array length

Implementing suggestions: naming + comparing array length

[Adenilson Cavalcanti Silva]

Dean

Thanks for the comments. I've just uploaded a new version of the patch, addressing your suggestions.

[Adenilson Cavalcanti Silva]

Created attachment 160151 [details]
Forgot a last unneeded namespace reference

[Dean Jackson]

Comment on attachment 160151 [details]
Forgot a last unneeded namespace reference

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.

[Adenilson Cavalcanti Silva]

Created attachment 160214 [details]
Changed method name, fixes in CL, etc.

[Dean Jackson]

Comment on attachment 160214 [details]
Changed method name, fixes in CL, etc.

Thanks for all the revisions.

[WebKit Review Bot]

Comment on attachment 160214 [details]
Changed method name, fixes in CL, etc.

Clearing flags on attachment: 160214

Committed r126491: <http://trac.webkit.org/changeset/126491>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.