Bug 93295 – CSSParser::parseTransform() refactor to accept valueList as argument

• Status: RESOLVED
• Resolution: INVALID
• Component: CSS

[Michelangelo De Simone]

CSSParser::parseTransform() should be refactored to accept a valueList as argument.
This is needed for parsing the 3d-transform (see bug 71443) within the custom() function.

[Michelangelo De Simone]

Created attachment 156813 [details]
Patch

[Michelangelo De Simone]

Created attachment 156987 [details]
Patch

[Darin Adler]

Comment on attachment 156987 [details]
Patch

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

Seems OK.

> Source/WebCore/ChangeLog:3
> +        CSSParser::parseTransform() refactor to accept valueList as argument

This says “what”, but should say “why”.

> Source/WebCore/ChangeLog:8
> +        parseTransform now accepts a CSSParserValueList as argument instead of relying on m_valueList.

This repeats the bug title without really adding anything.

> Source/WebCore/css/CSSParser.cpp:2336
> -            PassRefPtr<CSSValue> val = parseTransform();
> +            RefPtr<CSSValue> val = parseTransform(m_valueList.get());

If touching this code would be better to replace “val” with a word, either “value” or “transform”.

> Source/WebCore/css/CSSParser.cpp:2338
>                  addProperty(propId, val, important);

Should say val.release() instead of just val since this is now a RefPtr rather than a PassRefPtr.

[Alexis Menard (darktears)]

Comment on attachment 156987 [details]
Patch

I have a question. What is the benefit of that change? Why patching the transform only and not all other functions that uses the same pattern (parseFillShorthand...), i.e. using m_valueList? Is it for a more long term plan? If so then the ChangeLog is not explicit enough.

[Michelangelo De Simone]

(In reply to comment #4)
> (From update of attachment 156987 [details])
> I have a question. What is the benefit of that change? Why patching the transform only and not all other functions that uses the same pattern (parseFillShorthand...), i.e. using m_valueList? Is it for a more long term plan? If so then the ChangeLog is not explicit enough.

Yes, the patch for the bug #71443 relies on this and I thought to tear this change from it.
I'll update the changelog accordingly.

[Michelangelo De Simone]

Created attachment 157038 [details]
Patch for landing

[Rafael Brandao]

Comment on attachment 157038 [details]
Patch for landing

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

> Source/WebCore/css/CSSParser.cpp:7281
> +PassRefPtr<CSSValueList> CSSParser::parseTransform(CSSParserValueList* valueList)

Shouldn't this function be static now?

[WebKit Review Bot]

Comment on attachment 157038 [details]
Patch for landing

Clearing flags on attachment: 157038

Committed r124970: <http://trac.webkit.org/changeset/124970>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Rafael Brandao]

Comment on attachment 157038 [details]
Patch for landing

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

>> Source/WebCore/css/CSSParser.cpp:7281
>> +PassRefPtr<CSSValueList> CSSParser::parseTransform(CSSParserValueList* valueList)
> 
> Shouldn't this function be static now?

The static change shouldn't be straight forward though, but I think that if we are going to change the pattern to pass a parameter to deal with, we no longer need it to be a member function.

[Alexis Menard (darktears)]

(In reply to comment #10)
> (From update of attachment 157038 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157038&action=review
> 
> >> Source/WebCore/css/CSSParser.cpp:7281
> >> +PassRefPtr<CSSValueList> CSSParser::parseTransform(CSSParserValueList* valueList)
> > 
> > Shouldn't this function be static now?
> 
> The static change shouldn't be straight forward though, but I think that if we are going to change the pattern to pass a parameter to deal with, we no longer need it to be a member function.

I'm very unhappy with this change. If you need to go that way then rafael is right it needs to be static at the very least. CSSParser operates on member variables (it's the pattern in the entire file). Landing this patch out of context on *why* exactly you need to go that way doesn't make any sense to me. I think you need to upload the patch on https://bugs.webkit.org/show_bug.cgi?id=71443 for us to see the big picture.

On a side note : PassRefPtr<CSSValueList> CSSParser::parseFilter() also do something similar, handling custom() functions for filters. You could possibly use the same pattern or even better share code (if possible).

[Michelangelo De Simone]

I've talked with Alexis and we both agreed to roll this out. I'll merge the change in the patch for 71443.

[WebKit Review Bot]

Re-opened since this is blocked by 93495

[Michelangelo De Simone]

This patch will be part of 71443.