[webkit-reviews] review denied: [Bug 71441] [Part 1] Parse the custom() function in -webkit-filter : [Attachment 113493] Patch V1

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has denied Chiculita Alexandru
<achicu at adobe.com>'s request for review:
Bug 71441: [Part 1] Parse the custom() function in -webkit-filter
https://bugs.webkit.org/show_bug.cgi?id=71441

Attachment 113493: Patch V1
https://bugs.webkit.org/attachment.cgi?id=113493&action=review

------- Additional Comments from Dean Jackson <dino at apple.com>
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.