Bug 95223 – Use arrays in shaders

• Component: CSS
• Status: RESOLVED FIXED

[Michelangelo De Simone]

As per summary the array passed via array() shall be used within shaders.

[Michelangelo De Simone]

Created attachment 163445 [details]
Patch

[Michelangelo De Simone]

check-webkit-style reports the following false positive:

Source/WebCore/css/StyleResolver.cpp:5253:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]

Bug #95266 already addresses this. The patch will be landed manually.

[WebKit Review Bot]

Attachment 163445 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
Source/WebCore/css/StyleResolver.cpp:5253:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexandru Chiculita]

Comment on attachment 163445 [details]
Patch

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

Looks good. I have some minor comments.

> Source/WebCore/ChangeLog:11
> +        Values within array() are comma-separated even though the actual revision

Is there an issue added on the spec? If not we should have one and link it here.

> Source/WebCore/css/StyleResolver.cpp:5308
> +    if (parameterValue->isWebKitCSSArrayFunctionValue())

The fact that parameterValue is a value list which is also happens to be a CSSArrayFunctionValue is very subtle. 

I would at the very least add a comment before the check for the value list, saying that all the parameter values have the value list as the base class. Also move the check for the array function before the transforms.

> Source/WebCore/platform/graphics/filters/CustomFilterArrayParameter.h:64
> +    {
> +        if (!from || !isSameType(*from))
> +            return this;
> +
> +        const CustomFilterArrayParameter* fromArray = static_cast<const CustomFilterArrayParameter*>(from);
> +        if (size() != fromArray->size())
> +            return this;
> +
> +        RefPtr<CustomFilterArrayParameter> result = CustomFilterArrayParameter::create(name());
> +        for (size_t i = 0; i < size(); ++i)
> +            result->addValue(WebCore::blend(fromArray->valueAt(i), valueAt(i), progress));
> +
> +        return result.release();

You didn't add any test for this method. I have another patch that fixes the filter animation test framework to handle functions: 
https://bugs.webkit.org/show_bug.cgi?id=94980

I think you will need to wait for that one first. So extract it from this patch and fill a different bug for it (add a fixme here and "return this").

> Source/WebCore/platform/graphics/filters/CustomFilterNumberParameter.h:-45
> -    

Can you revert these changes? The whole file is untouched.

> Source/WebCore/platform/graphics/filters/CustomFilterNumberParameter.h:-50
> -    

ditto

> Source/WebCore/platform/graphics/filters/CustomFilterNumberParameter.h:-63
> -    

ditto

> Source/WebCore/platform/graphics/filters/CustomFilterNumberParameter.h:-71
> -    

ditto

> Source/WebCore/platform/graphics/filters/CustomFilterNumberParameter.h:-77
> -    

ditto

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:267
> +    unsigned parameterSize = arrayParameter->size();

nit: you could put this on the same line with the for.

for (unsigned i = 0, size = arrayParameter->size(); i < size; ++i)

> LayoutTests/css3/filters/custom/custom-filter-array-expected.html:15
> +        <script>
> +            if (window.testRunner)
> +                window.testRunner.waitUntilDone();
> +
> +            function runTest()
> +            {
> +                // We need to run the tests after the downloading succeeded.
> +                if (window.testRunner)
> +                    window.testRunner.notifyDone();
> +            }
> +        </script>

You don't need this in the ref html.

> LayoutTests/css3/filters/custom/custom-filter-array.html:28
> +    <body onload="runTest()">

Add a comment explaining what happens and what I should expect.

[Michelangelo De Simone]

Created attachment 163465 [details]
Patch

[WebKit Review Bot]

Attachment 163465 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
Source/WebCore/css/StyleResolver.cpp:5253:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexandru Chiculita]

Comment on attachment 163465 [details]
Patch

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

> Source/WebCore/css/StyleResolver.cpp:5298
> +    // All functions (eg.: array(), mix(), ...) are ValueLists.

Not all the possible values of the paramterValue are functions. That's why I wanted a comment here. The idea was that even simple number parameters are encapsulated in CSSValueLists. 

What about using the following comment instead?

// Number parameters are wrapped inside a CSSValueList and all the other functions values inherit from CSSValueList.

> Source/WebCore/css/StyleResolver.cpp:5310
> +    // At this point we assume that we're dealing with a transform and,
> +    // therefore, we check whether its first value is a valid transform.

Hm, I think this would read better as: "If the first value of the list is a transform function, then we could safely assume that all the remaining items are transforms. parseCustomFilterTransformParameter will return 0 if that assumption is incorrect."

[Michelangelo De Simone]

Created attachment 163491 [details]
Patch

[WebKit Review Bot]

Attachment 163491 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
Source/WebCore/css/StyleResolver.cpp:5253:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 163491 [details]
Patch

Attachment 163491 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13821578

[Michelangelo De Simone]

I guess the(In reply to comment #10)

> Attachment 163491 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/13821578

I guess there was something not working on the Mac bot.:-/

[Michelangelo De Simone]

Created attachment 163655 [details]
Patch

[WebKit Review Bot]

Attachment 163655 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
Source/WebCore/css/StyleResolver.cpp:5255:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Comment on attachment 163655 [details]
Patch

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

Looks good - only a few small changes.

But I think we need more tests:

- testing other valid and invalid arrays e.g. array(), array(1,), array(NaN), array(1, 2, foo), array(1, 2, translate(10)), array(with many many values) etc
- testing the transform fallback code

> Source/WebCore/ChangeLog:12
> +        Values within array() are comma-separated; specs will be updated
> +        accordingly: https://www.w3.org/Bugs/Public/show_bug.cgi?id=18839

"specs" -> "The specification"

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:779
> +    for (unsigned i = 0; i < arrayParameter->size(); ++i)

Might as well do length = arrayParameter->size(); i < length

> Source/WebCore/css/StyleResolver.cpp:5258
> +    for (unsigned i = 0; i < values->length(); ++i) {

Ditto here.

> LayoutTests/css3/filters/resources/fragment-color.fs:6
> +uniform float coolArray[4];
> +
> +void main()
> +{
> +    gl_FragColor = vec4(coolArray[0], coolArray[1], coolArray[2], coolArray[3]);

Should probably call this colorArray[4]. My first instinct was that this shader was "cooling" the saturation.

[Alexandru Chiculita]

(In reply to comment #14)
> (From update of attachment 163655 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163655&action=review
> 
> Looks good - only a few small changes.
> 
> But I think we need more tests:
> 
> - testing other valid and invalid arrays e.g. array(), array(1,), array(NaN), array(1, 2, foo), array(1, 2, translate(10)), array(with many many values) etc
> - testing the transform fallback code

I think we already have a couple of those covered in the invalid parsing test file.

http://svn.webkit.org/repository/webkit/trunk/LayoutTests/css3/filters/script-tests/custom-filter-property-parsing-invalid.js

Some of the things not covered there are:
- array(NaN)
- arrays with many values

Also I didn't see any test to check for floating point numbers.

[Michelangelo De Simone]

Created attachment 163947 [details]
Patch

[WebKit Review Bot]

Attachment 163947 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
Source/WebCore/css/StyleResolver.cpp:5256:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michelangelo De Simone]

(In reply to comment #14)

> Looks good - only a few small changes.

All the review comments have been address; let me know if there is anything else you wish me to change.

Thanks.

[Dean Jackson]

(In reply to comment #15)

> I think we already have a couple of those covered in the invalid parsing test file.
> 
> http://svn.webkit.org/repository/webkit/trunk/LayoutTests/css3/filters/script-tests/custom-filter-property-parsing-invalid.js
> 
> Some of the things not covered there are:
> - array(NaN)
> - arrays with many values

It was this one that worried me most, because parsing might succeed but binding to the uniforms could go wrong.

[Alexandru Chiculita]

(In reply to comment #19)
> (In reply to comment #15)
> 
> > I think we already have a couple of those covered in the invalid parsing test file.
> > 
> > http://svn.webkit.org/repository/webkit/trunk/LayoutTests/css3/filters/script-tests/custom-filter-property-parsing-invalid.js
> > 
> > Some of the things not covered there are:
> > - array(NaN)
> > - arrays with many values
> 
> It was this one that worried me most, because parsing might succeed but binding to the uniforms could go wrong.

Do you think we should enforce a maximum size for the array?

[WebKit Review Bot]

Comment on attachment 163947 [details]
Patch

Clearing flags on attachment: 163947

Committed r128626: <http://trac.webkit.org/changeset/128626>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.