[webkit-reviews] review denied: [Bug 95223] Use arrays in shaders : [Attachment 163655] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has denied Michelangelo De Simone
<michelangelo at webkit.org>'s request for review:
Bug 95223: Use arrays in shaders
https://bugs.webkit.org/show_bug.cgi?id=95223

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

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