Bug 71441 – [Part 1] Parse the custom() function in -webkit-filter

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

[Alexandru Chiculita]

This will just add the parsing of the property (no CSSStyleSelector code yet)

[Alexandru Chiculita]

Created attachment 113493 [details]
Patch V1

[Ojan Vafai]

Where is the spec for this? Would help to have it to compare against when reviewing the code.

[Dean Jackson]

(In reply to comment #2)
> Where is the spec for this? Would help to have it to compare against when reviewing the code.

The shaders parsing is described here:
https://dvcs.w3.org/hg/FXTF/raw-file/tip/custom/index.html

It falls into the CSS Filters code:
https://dvcs.w3.org/hg/FXTF/raw-file/tip/filters/publish/Filters.html

[Dean Jackson]

Comment on attachment 113493 [details]
Patch V1

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

Cool. I think there are just minor fixes. The main thing is making sure the file is included in the platform build systems.

> Source/WebCore/css/CSSParser.cpp:6475
> +static bool skipCommaValue(CSSParserValueList* argsList)
> +{

This is not an issue on the patch, but it makes me realise that we're being inconsistent with filters expecting space-separated values and shaders expecting comma-separated.

> Source/WebCore/css/CSSParser.cpp:6511
> +    // vertexMesh:  +<integer>{1,2}[wsp<box>][wspâdetached']
> +    //
> +    // param-value: true|false[wsp+true|false]{0-3} |
> +    //              <number>[wsp+<number>]{0-3} |
> +    //              <array> |
> +    //              <transform> |
> +    //              <texture(<uri>)>
> +    // array: âarray(â<number>[wsp<number>]*â)â
> +    // css-3d-transform: <transform-function>;[<transform-function>]*
> +    // transform:   <css-3d-transform> | <mat>
> +    // mat:         âmat2(â<number>(,<number>){3}â)â | 
> +    //              âmat3(â<number>(,<number>){8}â)â | 
> +    //              âmat4(â<number>(,<number>){15}â)â )

Unicode cruft here.

> Source/WebCore/css/CSSParser.cpp:6537
> +    if (!shadersList->length() || !hadAtLeastOneCustomShader || shadersList->length() > 2 || !skipCommaValue(argsList))

I wonder if we can come up with a better name than skipCommaValue, since it's returning a boolean? This isn't a big deal.

> Source/WebCore/css/CSSParser.cpp:6550
> +        int intValue = static_cast<int>(arg->fValue);
> +        meshSizeList->append(primitiveValueCache()->createValue(intValue, CSSPrimitiveValue::CSS_NUMBER));

No need for the variable - just use it in createValue.

> Source/WebCore/css/CSSParser.cpp:6588
> +        // TODO: Implement other parameters types parsing.

Could we add a bugzilla link here?

> Source/WebCore/css/CSSParser.cpp:6591
> +            // If we hit a comma it means we finished this parameters values.

Super-nit "parameter's" :)

> Source/WebCore/css/CSSParser.h:191
> +#if ENABLE(CSS_SHADERS)
> +    PassRefPtr<WebKitCSSFilterValue> parseCustomFilter(CSSParserValue*);
> +#endif

Could we move this to below parseFilter()?

> Source/WebCore/css/CSSValueKeywords.in:856
> +#if defined(ENABLE_CSS_SHADERS) && ENABLE_CSS_SHADERS
> +// -webkit-filter
> +// values for the custom() function
> +//border-box
> +//padding-box
> +//content-box

Guard this with CSS_FILTERS as well

> Source/WebCore/css/WebKitCSSShaderValue.h:1
> +/*

I don't see this file being added to any of the build systems. You'll need to edit the Xcode project, the VS file, the gyp stuff, etc.

> Source/WebCore/css/WebKitCSSShaderValue.h:32
> +

We typically put #if ENABLE(CSS_SHADERS) in these files too.

[Alexandru Chiculita]

Created attachment 113650 [details]
Patch V2

Thanks for the comments, I've updated the patch.

[Dean Jackson]

Comment on attachment 113650 [details]
Patch V2

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

All good, as long as you fix the tests. Maybe have JS extract the url into just the last component?

> LayoutTests/css3/filters/custom-filter-property-parsing-expected.txt:10
> +PASS declaration.getPropertyValue('-webkit-filter') is 'custom(url(file:///Volumes/SSD/Users/achicu/code/webkit/git/webkit/LayoutTests/css3/filters/vertex.shader))'

This is going to break for everyone but you :)

> LayoutTests/css3/filters/custom-filter-property-parsing-expected.txt:16
> +PASS subRule.cssText is 'custom(url(file:///Volumes/SSD/Users/achicu/code/webkit/git/webkit/LayoutTests/css3/filters/vertex.shader))'

ditto

> LayoutTests/css3/filters/custom-filter-property-parsing-expected.txt:27
> +PASS subRule.cssText is 'custom(none url(file:///Volumes/SSD/Users/achicu/code/webkit/git/webkit/LayoutTests/css3/filters/fragment.shader))'

ditto (and a couple more)

> Source/WebCore/css/CSSParser.cpp:6488
> +    // the implementation of the syntax:

Typically we don't put instructions like this, because it is obvious to anyone editing the code.

[Dean Jackson]

Comment on attachment 113650 [details]
Patch V2

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

> Source/WebCore/css/CSSParser.cpp:6487
> +    // Listing the format for the custom filter here. Please update this when changing

Typically we don't put instructions like this, because it is obvious to anyone editing the code.

[Alexandru Chiculita]

Created attachment 114068 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 114068 [details]
Patch for landing

Rejecting attachment 114068 [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:
3/filters/custom-filter-property-parsing-invalid-expected.txt
patching file LayoutTests/css3/filters/custom-filter-property-parsing-invalid.html
patching file LayoutTests/css3/filters/custom-filter-property-parsing.html
patching file LayoutTests/css3/filters/script-tests/custom-filter-property-parsing-invalid.js
patching file LayoutTests/css3/filters/script-tests/custom-filter-property-parsing.js

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1

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

[Dean Jackson]

I tried to land this patch manually but noticed a couple of build errors (they were on mac, that's why EWS didn't catch them - I guess you're on Windows).

Source/WebCore/css/CSSValue.cpp:90:13: error: enumeration value 'WebKitCSSShaderClass' not handled in switch [-Werror,-Wswitch-enum,3]
Source/WebCore/css/CSSValue.cpp:165:13: error: enumeration value 'WebKitCSSShaderClass' not handled in switch [-Werror,-Wswitch-enum,3]

Source/WebCore/css/WebKitCSSShaderValue.h:39:7: error: 'WebCore::WebKitCSSShaderValue' has virtual functions but non-virtual destructor [-Werror,-Wnon-virtual-dtor,3]

All simple fixes thankfully.

[Alexandru Chiculita]

(In reply to comment #10)
> I tried to land this patch manually but noticed a couple of build errors (they were on mac, that's why EWS didn't catch them - I guess you're on Windows).
> 
> Source/WebCore/css/CSSValue.cpp:90:13: error: enumeration value 'WebKitCSSShaderClass' not handled in switch [-Werror,-Wswitch-enum,3]
> Source/WebCore/css/CSSValue.cpp:165:13: error: enumeration value 'WebKitCSSShaderClass' not handled in switch [-Werror,-Wswitch-enum,3]
> 
> Source/WebCore/css/WebKitCSSShaderValue.h:39:7: error: 'WebCore::WebKitCSSShaderValue' has virtual functions but non-virtual destructor [-Werror,-Wnon-virtual-dtor,3]
> 
> All simple fixes thankfully.

Yes, should be simple. I'm actually using Mac, just that the CSSValue.cpp file was heavily changed after I've posted the patch: http://trac.webkit.org/log/trunk/Source/WebCore/css/CSSValue.cpp

I will update the patch and post for commit again.

[Alexandru Chiculita]

Created attachment 114248 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 114248 [details]
Patch for landing

Clearing flags on attachment: 114248

Committed r99695: <http://trac.webkit.org/changeset/99695>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/10420439>