Bug 94226 - Parse the array() function for custom filters
Summary: Parse the array() function for custom filters
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michelangelo De Simone
URL:
Keywords:
Depends on:
Blocks: 71395 95223 95224
  Show dependency treegraph
 
Reported: 2012-08-16 10:41 PDT by Michelangelo De Simone
Modified: 2012-09-05 19:54 PDT (History)
9 users (show)

See Also:


Attachments
(Not for review) Patch (52.27 KB, patch)
2012-08-27 14:51 PDT, Michelangelo De Simone
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-04 (640.24 KB, application/zip)
2012-08-27 16:07 PDT, WebKit Review Bot
no flags Details
Patch not for review (53.29 KB, patch)
2012-08-28 17:10 PDT, Michelangelo De Simone
no flags Details | Formatted Diff | Diff
Patch (54.09 KB, patch)
2012-08-29 14:11 PDT, Michelangelo De Simone
no flags Details | Formatted Diff | Diff
Patch (39.54 KB, patch)
2012-08-30 23:35 PDT, Michelangelo De Simone
no flags Details | Formatted Diff | Diff
Patch (43.12 KB, patch)
2012-09-04 16:13 PDT, Michelangelo De Simone
no flags Details | Formatted Diff | Diff
Patch (43.27 KB, patch)
2012-09-04 20:20 PDT, Michelangelo De Simone
no flags Details | Formatted Diff | Diff
Patch for landing (43.34 KB, patch)
2012-09-05 10:51 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-16 10:41:46 PDT
Code shall be added to parse the array() function.
Comment 1 Michelangelo De Simone 2012-08-27 14:51:59 PDT
Created attachment 160814 [details]
(Not for review) Patch
Comment 2 WebKit Review Bot 2012-08-27 16:07:27 PDT
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
Comment 3 WebKit Review Bot 2012-08-27 16:07:30 PDT
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
Comment 4 Michelangelo De Simone 2012-08-28 17:10:40 PDT
Created attachment 161096 [details]
Patch not for review
Comment 5 WebKit Review Bot 2012-08-29 12:25:03 PDT
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
Comment 6 Michelangelo De Simone 2012-08-29 14:11:26 PDT
Created attachment 161313 [details]
Patch
Comment 7 WebKit Review Bot 2012-08-29 20:52:51 PDT
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
Comment 8 Michelangelo De Simone 2012-08-30 23:35:07 PDT
Created attachment 161623 [details]
Patch
Comment 9 Alexandru Chiculita 2012-09-04 11:01:15 PDT
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.
Comment 10 Michelangelo De Simone 2012-09-04 16:13:20 PDT
Created attachment 162119 [details]
Patch
Comment 11 Benjamin Poulain 2012-09-04 18:37:47 PDT
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() + ")";

")" -> ')'
Comment 12 Michelangelo De Simone 2012-09-04 20:20:55 PDT
Created attachment 162151 [details]
Patch
Comment 13 Dirk Schulze 2012-09-05 10:02:54 PDT
Comment on attachment 162151 [details]
Patch

Thanks Benjamin for the great review! Patch looks good for me. r=me.
Comment 14 WebKit Review Bot 2012-09-05 10:26:09 PDT
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
Comment 15 Michelangelo De Simone 2012-09-05 10:51:07 PDT
Created attachment 162286 [details]
Patch for landing
Comment 16 Benjamin Poulain 2012-09-05 11:03:18 PDT
(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...
Comment 17 Dirk Schulze 2012-09-05 11:12:05 PDT
(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.
Comment 18 Benjamin Poulain 2012-09-05 11:34:35 PDT
> 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. :)
Comment 19 WebKit Review Bot 2012-09-05 19:54:31 PDT
Comment on attachment 162286 [details]
Patch for landing

Clearing flags on attachment: 162286

Committed r127682: <http://trac.webkit.org/changeset/127682>
Comment 20 WebKit Review Bot 2012-09-05 19:54:35 PDT
All reviewed patches have been landed.  Closing bug.