Bug 74652

Summary: [CSS Shaders] Move CustomFilterOperation to the platform layer
Product: WebKit Reporter: Alexandru Chiculita <achicu>
Component: Layout and RenderingAssignee: Alexandru Chiculita <achicu>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarrin, dino, macpherson, rakuco, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 71392    
Attachments:
Description Flags
Patch V1
cmarrin: review-
Patch V2
none
Patch V3 cmarrin: review+

Alexandru Chiculita
Reported 2011-12-15 15:13:13 PST
Move CustomFilterOperation to the platform layer and use a PlatformShader instead of a CachedShader. Quoted from previous discussions with Chris Marrin: 1) Make an abstract base class called Shader (or maybe PlatformShader or ShaderClient or something). This has a pure virtual method to get the shader string. 2) Subclass StyleShader from Shader and implement the get shader method. 3) Replace the methods that take/return StyleShader with those that take/return Shader. 4) Move CustomFilterOperation down into platform along with the others.
Attachments
Patch V1 (56.82 KB, patch)
2012-01-05 05:51 PST, Alexandru Chiculita 		cmarrin: review-
DetailsFormatted DiffDiff
Patch V2 (65.52 KB, patch)
2012-01-09 07:25 PST, Alexandru Chiculita 		no flags
DetailsFormatted DiffDiff
Patch V3 (81.08 KB, patch)
2012-01-09 07:29 PST, Alexandru Chiculita 		cmarrin: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Alexandru Chiculita
Comment 1 2012-01-05 05:51:16 PST
Created attachment 121267 [details] Patch V1
Chris Marrin
Comment 2 2012-01-05 07:37:57 PST
Comment on attachment 121267 [details] Patch V1 Great patch. But I'm really concerned that we have only 3 custom filter tests in LayoutTests/css3/filters. And two of those are just testing style, so they don't exercise any of this new code. Please add at least a couple of tests which exercise the tolerance of missing shaders and the use of the cache (write two shaders which use the same vertex or fragment shader, etc.).
Alexandru Chiculita
Comment 3 2012-01-09 07:25:07 PST
Created attachment 121669 [details] Patch V2 Added two new tests.
Alexandru Chiculita
Comment 4 2012-01-09 07:29:46 PST
Created attachment 121671 [details] Patch V3 This time including binary files.
Chris Marrin
Comment 5 2012-01-09 10:02:55 PST
Comment on attachment 121671 [details] Patch V3 View in context: https://bugs.webkit.org/attachment.cgi?id=121671&action=review r=me, but please add text to the test cases so a successful test is obvious without looking at the pixel results > LayoutTests/css3/filters/custom-filter-shader-cache-expected.txt:1 > + You should really have something output here. At least say something like "Test of cached custom filter shaders. You should see 12 blocks of color bars, with increasing lightness from left to right" > LayoutTests/css3/filters/missing-custom-filter-shader-expected.txt:1 > + Same here. At least say something like "Test of missing custom filter shaders. You should see 5 blocks of color bars, all the same"
Simon Fraser (smfr)
Comment 6 2012-01-09 10:32:37 PST
Comment on attachment 121671 [details] Patch V3 Custom filter tests always fail (bug 75047) so I don't see how you can be adding more.
Alexandru Chiculita
Comment 7 2012-01-09 11:16:11 PST
(In reply to comment #6) > (From update of attachment 121671 [details]) > Custom filter tests always fail (bug 75047) so I don't see how you can be adding more. New tests have window.layoutTestController.overridePreference("WebKitWebGLEnabled", "1"); which seems to do the trick about enabling WebGL in DRT (at least on SnowLeopard it works, not sure about Lion). For the old test case I've did the same in https://bugs.webkit.org/show_bug.cgi?id=75842 .
Alexandru Chiculita
Comment 8 2012-01-11 05:28:49 PST
Landed in http://trac.webkit.org/changeset/104702. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.