Bug 94226 – Parse the array() function for custom filters

• Component: CSS
• Status: RESOLVED FIXED

[Michelangelo De Simone]

Code shall be added to parse the array() function.

[Michelangelo De Simone]

Created attachment 160814 [details]
(Not for review) Patch

[WebKit Review Bot]

Comment on attachment 160814 [details]
(Not for review) Patch

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

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

[WebKit Review Bot]

Created attachment 160840 [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 161096 [details]
Patch not for review

[WebKit Review Bot]

Comment on attachment 161096 [details]
Patch not for review

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

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

[Michelangelo De Simone]

Created attachment 161313 [details]
Patch

[WebKit Review Bot]

Comment on attachment 161313 [details]
Patch

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

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

[Michelangelo De Simone]

Created attachment 161623 [details]
Patch

[Alexandru Chiculita]

Comment on attachment 161623 [details]
Patch

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

Looks good! Some minor comments.

> Source/WebCore/css/CSSParser.cpp:7460
> +        if (!validUnit(arrayArgParserValue, FNumber, CSSStrictMode) && !isComma(arrayArgParserValue))

I think this can be simplified.

> Source/WebCore/css/WebKitCSSArrayFunctionValue.cpp:40
> +: CSSValueList(WebKitCSSArrayFunctionValueClass, CommaSeparator)

nit: indent here.

> Source/WebCore/css/WebKitCSSArrayFunctionValue.cpp:45
> +: CSSValueList(cloneFrom)

nit: indent here.

> LayoutTests/css3/filters/script-tests/custom-filter-property-parsing-invalid.js:95
> +testInvalidFilterRule("One comma in array()", "custom(none url(shader), id array(,))");

Should we add a test for array(none) or any other invalid unit. Maybe another one with array(1px).

> LayoutTests/css3/filters/script-tests/custom-filter-property-parsing.js:168
> +            "custom(none url(fragment.shader), firstTransform rotate(0deg), secondTransform rotate(0deg))",

Did you add a comment in the changelog about this?

> LayoutTests/css3/filters/script-tests/custom-filter-property-parsing.js:184
> +            "custom(none url(fragment.shader), t3 array(1, -2.2, 3.14, 0.4, 5))",

Can you do the same thing as you did for transforms? Have more arrays in the same filter.

[Michelangelo De Simone]

Created attachment 162119 [details]
Patch

[Benjamin Poulain]

Comment on attachment 162119 [details]
Patch

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

Quick comments:

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:25507
>  				FBD6AF8C15EF2604008B7110 /* BasicShapes.h in Headers */,
> +				150B923A15F08DC400E10986 /* WebKitCSSArrayFunctionValue.h in Headers */,
>  			);

Please sort the build section, it is very helpful for rebaseline.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:28598
>  				377A3A9015EFCE9B0059F5C7 /* BitmapImageCG.cpp in Sources */,
> +				150B923915F08DC400E10986 /* WebKitCSSArrayFunctionValue.cpp in Sources */,
>  			);

Ditto.

> Source/WebCore/css/CSSParser.cpp:7457
> +    // array() values are comma separated

Comments are sentences, the period is missing.

> Source/WebCore/css/CSSParser.cpp:7461
> +        // We parse pairs <Value, Comma> at each step

Ditto.

> Source/WebCore/css/CSSParser.cpp:7469
> +        // A valid value is supposed to be followed by a comma, or null, if the
> +        // ParserValueList is over

Ditto.

I am not sure this comment is useful, the code bellow is pretty straightforward.

> Source/WebCore/css/CSSParser.cpp:7479
> +        arrayArgParserValue = arrayArgsParserValueList->next();
> +        if (!arrayArgParserValue)
> +            return 0;

This while loop has a strange construct.
The condition arrayArgParserValue it tested here and at the beginning.

I think you should use a more direct flow. For example:
while (true) {
    if (!arrayArgParserValue)
        return 0;

    ...
}

> Source/WebCore/css/WebKitCSSArrayFunctionValue.cpp:51
> +    return "array(" + CSSValueList::customCssText() + ")";

")" -> ')'

[Michelangelo De Simone]

Created attachment 162151 [details]
Patch

[Dirk Schulze]

Comment on attachment 162151 [details]
Patch

Thanks Benjamin for the great review! Patch looks good for me. r=me.

[WebKit Review Bot]

Comment on attachment 162151 [details]
Patch

Rejecting attachment 162151 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
aining/Source/WebCore/css/WebKitCSSFilterValue.o
Source/WebCore/css/WebKitCSSArrayFunctionValue.cpp: In member function 'void WebCore::WebKitCSSArrayFunctionValue::reportDescendantMemoryUsage(WebCore::MemoryObjectInfo*) const':
Source/WebCore/css/WebKitCSSArrayFunctionValue.cpp:61: error: 'CSS' is not a member of 'WebCore::MemoryInstrumentation'
make: *** [out/Release/obj.target/webcore_remaining/Source/WebCore/css/WebKitCSSArrayFunctionValue.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/13743945

[Michelangelo De Simone]

Created attachment 162286 [details]
Patch for landing

[Benjamin Poulain]

(In reply to comment #15)
> Created an attachment (id=162286) [details]
> Patch for landing

Nice of you. There are so many cowboys with webkit-patch land...

[Dirk Schulze]

(In reply to comment #16)
> (In reply to comment #15)
> > Created an attachment (id=162286) [details] [details]
> > Patch for landing
> 
> Nice of you. There are so many cowboys with webkit-patch land...

Actually we agreed to land patches our selfs a long time ago. So don't call them cow boys, they were asked to use webkit-patch land. And I didn't hear another statement since that time.

[Benjamin Poulain]

> Actually we agreed to land patches our selfs a long time ago. So don't call them cow boys, they were asked to use webkit-patch land. And I didn't hear another statement since that time.

Oh, I did not intend to point fingers! I am not sure who you are thinking about here.

You know what I mean...those non-trivial patches that lands before all bots were green. :)

[WebKit Review Bot]

Comment on attachment 162286 [details]
Patch for landing

Clearing flags on attachment: 162286

Committed r127682: <http://trac.webkit.org/changeset/127682>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.