Bug 96285

Summary: [CSS Shaders] The mesh should be specified using <column, row> order
Product: WebKit Reporter: Alexandru Chiculita <achicu>
Component: CSSAssignee: Max Vujovic <mvujovic>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, cmarcelo, dino, macpherson, menard, mvujovic, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 71392    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Alexandru Chiculita
Reported 2012-09-10 10:07:57 PDT
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
Attachments
Patch (9.09 KB, patch)
2012-10-22 16:50 PDT, Max Vujovic 		no flags
DetailsFormatted DiffDiff
Patch (9.20 KB, patch)
2012-10-23 11:55 PDT, Max Vujovic 		no flags
DetailsFormatted DiffDiff
Patch (9.21 KB, patch)
2012-10-23 13:44 PDT, Max Vujovic 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Max Vujovic
Comment 1 2012-10-22 16:50:27 PDT
Created attachment 170027 [details] Patch
Alexandru Chiculita
Comment 2 2012-10-23 09:40:19 PDT
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
Comment 3 2012-10-23 11:55:48 PDT
Created attachment 170203 [details] Patch
Max Vujovic
Comment 4 2012-10-23 13:44:59 PDT
Created attachment 170226 [details] Patch Removed tab character from test.
Max Vujovic
Comment 5 2012-10-23 13:47:19 PDT
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
Comment 6 2012-10-24 15:43:10 PDT
Thanks for the review, Dean!
WebKit Review Bot
Comment 7 2012-10-24 16:01:46 PDT
Comment on attachment 170226 [details] Patch Clearing flags on attachment: 170226 Committed r132416: <http://trac.webkit.org/changeset/132416>
WebKit Review Bot
Comment 8 2012-10-24 16:01:51 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.