Bug 96285 – [CSS Shaders] The mesh should be specified using <column, row> order

• Component: CSS
• Status: RESOLVED FIXED

[Alexandru Chiculita]

The spec says the mesh is defined using <column, row> and not <row, column>. Update the implementation to match that.

https://dvcs.w3.org/hg/FXTF/raw-file/tip/filters/index.html#vertexMesh-attribute

[Max Vujovic]

Created attachment 170027 [details]
Patch

[Alexandru Chiculita]

Comment on attachment 170027 [details]
Patch

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

Looks good.

> LayoutTests/css3/filters/resources/discard-bottom-vertices.vs:13
> +	if (position.y > 0.0)

Why not use the a_triangleCoord.y instead?

> LayoutTests/css3/filters/resources/discard-bottom-vertices.vs:14
> +		position = vec4(-0.5);

nit: use 4 spaces for indentation.
nit: You are also setting the 3rd and 4th values to -0.5.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:939
> -            meshParameters->append(cssValuePool().createValue(customOperation->meshRows(), CSSPrimitiveValue::CSS_NUMBER));
>              meshParameters->append(cssValuePool().createValue(customOperation->meshColumns(), CSSPrimitiveValue::CSS_NUMBER));
> +            meshParameters->append(cssValuePool().createValue(customOperation->meshRows(), CSSPrimitiveValue::CSS_NUMBER));

Add a test for the computed style.

[Max Vujovic]

Created attachment 170203 [details]
Patch

[Max Vujovic]

Created attachment 170226 [details]
Patch

Removed tab character from test.

[Max Vujovic]

Comment on attachment 170027 [details]
Patch

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

>> LayoutTests/css3/filters/resources/discard-bottom-vertices.vs:13
>> +	if (position.y > 0.0)
> 
> Why not use the a_triangleCoord.y instead?

After our in-person discussion, I've changed the test to create a checkboard pattern by coloring the mesh tiles using a_triangleCoord. If the checkboard tiles are squares, the test passes. If not, the test fails.

>> LayoutTests/css3/filters/resources/discard-bottom-vertices.vs:14
>> +		position = vec4(-0.5);
> 
> nit: use 4 spaces for indentation.
> nit: You are also setting the 3rd and 4th values to -0.5.

nit 1: Thanks for catching that. My editor was misbehaving.
nit 2: The new test doesn't need to do that anymore.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:939
>> +            meshParameters->append(cssValuePool().createValue(customOperation->meshRows(), CSSPrimitiveValue::CSS_NUMBER));
> 
> Add a test for the computed style.

This is already covered by the computed style tests. In computed style, we can only check that the order we input the mesh attributes is the order we output them. For example, we can only check that the mesh coords of "10 20" come back as "10 20". They're not labeled, so I can't test which one represents numColumns vs. numRows. However, the test I've added verifies the numColumns vs. numRows difference by rendering it.

[Max Vujovic]

Thanks for the review, Dean!

[WebKit Review Bot]

Comment on attachment 170226 [details]
Patch

Clearing flags on attachment: 170226

Committed r132416: <http://trac.webkit.org/changeset/132416>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.