Bug 71441 - [Part 1] Parse the custom() function in -webkit-filter
Summary: [Part 1] Parse the custom() function in -webkit-filter
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexandru Chiculita
URL:
Keywords: InRadar
Depends on:
Blocks: 71395
  Show dependency treegraph
 
Reported: 2011-11-03 01:14 PDT by Alexandru Chiculita
Modified: 2011-12-31 17:52 PST (History)
7 users (show)

See Also:


Attachments
Patch V1 (38.36 KB, patch)
2011-11-03 07:57 PDT, Alexandru Chiculita
dino: review-
Details | Formatted Diff | Diff
Patch V2 (44.05 KB, patch)
2011-11-04 05:26 PDT, Alexandru Chiculita
no flags Details | Formatted Diff | Diff
Patch for landing (45.11 KB, patch)
2011-11-08 07:54 PST, Alexandru Chiculita
no flags Details | Formatted Diff | Diff
Patch for landing (45.82 KB, patch)
2011-11-09 04:33 PST, Alexandru Chiculita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandru Chiculita 2011-11-03 01:14:11 PDT
This will just add the parsing of the property (no CSSStyleSelector code yet)
Comment 1 Alexandru Chiculita 2011-11-03 07:57:26 PDT
Created attachment 113493 [details]
Patch V1
Comment 2 Ojan Vafai 2011-11-03 09:54:11 PDT
Where is the spec for this? Would help to have it to compare against when reviewing the code.
Comment 3 Dean Jackson 2011-11-03 10:06:57 PDT
(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
Comment 4 Dean Jackson 2011-11-03 11:16:28 PDT
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.
Comment 5 Alexandru Chiculita 2011-11-04 05:26:30 PDT
Created attachment 113650 [details]
Patch V2

Thanks for the comments, I've updated the patch.
Comment 6 Dean Jackson 2011-11-07 15:48:48 PST
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.
Comment 7 Dean Jackson 2011-11-07 15:51:05 PST
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.
Comment 8 Alexandru Chiculita 2011-11-08 07:54:47 PST
Created attachment 114068 [details]
Patch for landing
Comment 9 WebKit Review Bot 2011-11-08 07:58:38 PST
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
Comment 10 Dean Jackson 2011-11-08 21:21:45 PST
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.
Comment 11 Alexandru Chiculita 2011-11-09 00:21:04 PST
(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.
Comment 12 Alexandru Chiculita 2011-11-09 04:33:13 PST
Created attachment 114248 [details]
Patch for landing
Comment 13 WebKit Review Bot 2011-11-09 05:55:31 PST
Comment on attachment 114248 [details]
Patch for landing

Clearing flags on attachment: 114248

Committed r99695: <http://trac.webkit.org/changeset/99695>
Comment 14 WebKit Review Bot 2011-11-09 05:55:36 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2011-11-09 10:54:45 PST
<rdar://problem/10420439>