Bug 71443 – [Part 3] Parse the custom() function in -webkit-filter: parse the 3d-transforms parameters

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

[Alexandru Chiculita]

Add parsing code for the the 3d-transforms parameters.

[Michelangelo De Simone]

Created attachment 158647 [details]
Patch

[Dirk Schulze]

Comment on attachment 158647 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:2325
> +            RefPtr<CSSValue> transformValue = parseTransform(m_valueList.get());
> +            if (transformValue) {
> +                addProperty(propId, transformValue.release(), important);

Can we avoid passing a boolean? It is also not obvious from the name why we care about the comma. Anyway, I would rather avoid this.

> Source/WebCore/css/CSSParser.cpp:7567
> +        if (arg->unit == CSSParserValue::Function && arg->function)
> +            parameterValue = parseTransform(argsList, true);

It looks like you are assuming that every function is a transform and then you parse it as a transform, that doesn't look quite right. What about the mat2,3,4() functions? Or texture() or array()?

> Source/WebCore/css/CSSParser.cpp:7574
> +            // TODO: Implement other parameters types parsing.
> +            // textures: https://bugs.webkit.org/show_bug.cgi?id=71442
> +            // mat2, mat3, mat4: https://bugs.webkit.org/show_bug.cgi?id=71444
> +            RefPtr<CSSValueList> paramValueList = CSSValueList::createSpaceSeparated();
> +            while ((arg = argsList->current())) {
> +                // If we hit a comma it means we finished this parameter's values.

The others are functions as well, how do you differ?

[WebKit Review Bot]

Comment on attachment 158647 [details]
Patch

Attachment 158647 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13501825

New failing tests:
css3/filters/custom/custom-filter-property-parsing.html

[WebKit Review Bot]

Created attachment 158653 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Michelangelo De Simone]

Created attachment 158672 [details]
Patch

[WebKit Review Bot]

Comment on attachment 158672 [details]
Patch

Attachment 158672 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13517166

New failing tests:
css3/filters/custom/custom-filter-property-parsing.html

[WebKit Review Bot]

Created attachment 158680 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Michelangelo De Simone]

Created attachment 158909 [details]
Patch

[Michelangelo De Simone]

(In reply to comment #2)

> > Source/WebCore/css/CSSParser.cpp:2325
> > +            RefPtr<CSSValue> transformValue = parseTransform(m_valueList.get());
> > +            if (transformValue) {
> > +                addProperty(propId, transformValue.release(), important);
> 
> Can we avoid passing a boolean? It is also not obvious from the name why we care about the comma. Anyway, I would rather avoid this.

Done. Present code has been slightly refactored, the parseTransform() method has been split in two to improve code reuse.

> > Source/WebCore/css/CSSParser.cpp:7567
> > +        if (arg->unit == CSSParserValue::Function && arg->function)
> > +            parameterValue = parseTransform(argsList, true);
> 
> It looks like you are assuming that every function is a transform and then you parse it as a transform, that doesn't look quite right. What about the mat2,3,4() functions? Or texture() or array()?

I've added a comment to reflect this: once we will implement the other functions (matN, array, texture), we shall branch for transform at the end of the future if-else-if block. In shall be something like this:

 if (arg->unit == CSSParserValue::Function && arg->function) {
  if (isMatN())...
  else if (isArray())...
  else if (isTexture())....
  else [it might be a transform]..

[Dirk Schulze]

Comment on attachment 158909 [details]
Patch

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

Patch looks good to me. Just some snippets.

> Source/WebCore/css/CSSParser.cpp:7579
>          // TODO: Implement other parameters types parsing.
>          // textures: https://bugs.webkit.org/show_bug.cgi?id=71442
> -        // 3d-transforms: https://bugs.webkit.org/show_bug.cgi?id=71443
>          // mat2, mat3, mat4: https://bugs.webkit.org/show_bug.cgi?id=71444
> -        RefPtr<CSSValueList> paramValueList = CSSValueList::createSpaceSeparated();
> -        while ((arg = argsList->current())) {
> -            // If we hit a comma it means we finished this parameter's values.
> -            if (isComma(arg))
> -                break;
> -            if (!validUnit(arg, FNumber, CSSStrictMode))
> +        // array: https://bugs.webkit.org/show_bug.cgi?id=94226
> +        // 3d-transform shall be the last to be checked
> +        if (arg->unit == CSSParserValue::Function && arg->function)
> +            parameterValue = parseCustomFilterTransform(argsList);

Please move the comment in to this if statement, I think you want to check all functions in the same if statement.

[Michelangelo De Simone]

Created attachment 158924 [details]
Patch

[WebKit Review Bot]

Comment on attachment 158924 [details]
Patch

Clearing flags on attachment: 158924

Committed r125845: <http://trac.webkit.org/changeset/125845>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Taiju Tsuiki]

Reopening to attach new patch.

[Taiju Tsuiki]

Created attachment 159022 [details]
Patch

[Taiju Tsuiki]

Comment on attachment 159022 [details]
Patch

Sorry, I might upload a patch to wrong bug.