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.
Created attachment 156813 [details] Patch
Created attachment 156987 [details] Patch
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.
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.
(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.
Created attachment 157038 [details] Patch for landing
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?
Comment on attachment 157038 [details] Patch for landing Clearing flags on attachment: 157038 Committed r124970: <http://trac.webkit.org/changeset/124970>
All reviewed patches have been landed. Closing bug.
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.
(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).
I've talked with Alexis and we both agreed to roll this out. I'll merge the change in the patch for 71443.
Re-opened since this is blocked by 93495
This patch will be part of 71443.