Bug 93295 - CSSParser::parseTransform() refactor to accept valueList as argument
Summary: CSSParser::parseTransform() refactor to accept valueList as argument
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Minor
Assignee: Michelangelo De Simone
URL:
Keywords:
Depends on: 93495
Blocks: 71443
  Show dependency treegraph
 
Reported: 2012-08-06 14:19 PDT by Michelangelo De Simone
Modified: 2012-08-08 10:12 PDT (History)
5 users (show)

See Also:


Attachments
Patch (14.28 KB, patch)
2012-08-06 17:47 PDT, Michelangelo De Simone
no flags Details | Formatted Diff | Diff
Patch (3.03 KB, patch)
2012-08-07 13:22 PDT, Michelangelo De Simone
no flags Details | Formatted Diff | Diff
Patch for landing (3.30 KB, patch)
2012-08-07 16:26 PDT, Michelangelo De Simone
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michelangelo De Simone 2012-08-06 14:19:08 PDT
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.
Comment 1 Michelangelo De Simone 2012-08-06 17:47:54 PDT
Created attachment 156813 [details]
Patch
Comment 2 Michelangelo De Simone 2012-08-07 13:22:08 PDT
Created attachment 156987 [details]
Patch
Comment 3 Darin Adler 2012-08-07 14:35:49 PDT
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 4 Alexis Menard (darktears) 2012-08-07 14:47:20 PDT
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.
Comment 5 Michelangelo De Simone 2012-08-07 15:58:06 PDT
(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.
Comment 6 Michelangelo De Simone 2012-08-07 16:26:52 PDT
Created attachment 157038 [details]
Patch for landing
Comment 7 Rafael Brandao 2012-08-07 19:09:34 PDT
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 8 WebKit Review Bot 2012-08-07 19:26:08 PDT
Comment on attachment 157038 [details]
Patch for landing

Clearing flags on attachment: 157038

Committed r124970: <http://trac.webkit.org/changeset/124970>
Comment 9 WebKit Review Bot 2012-08-07 19:26:13 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Rafael Brandao 2012-08-08 06:01:11 PDT
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.
Comment 11 Alexis Menard (darktears) 2012-08-08 06:17:35 PDT
(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).
Comment 12 Michelangelo De Simone 2012-08-08 10:07:38 PDT
I've talked with Alexis and we both agreed to roll this out. I'll merge the change in the patch for 71443.
Comment 13 WebKit Review Bot 2012-08-08 10:10:52 PDT
Re-opened since this is blocked by 93495
Comment 14 Michelangelo De Simone 2012-08-08 10:12:41 PDT
This patch will be part of 71443.