Bug 44682

Summary: [CHROMIUM] Canvas2D shaders should have their own classes
Product: WebKit Reporter: Stephen White <senorblanco>
Component: Layout and RenderingAssignee: Stephen White <senorblanco>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, jamesr, kbr, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows 7   
Attachments:
Description Flags
Patch kbr: review+

Stephen White
Reported 2010-08-26 06:18:40 PDT
GLES2Canvas currently contains all the shaders for 2D <canvas>. They should be refactored out into their own class hierarchy.
Attachments
Patch (24.29 KB, patch)
2010-08-26 06:52 PDT, Stephen White 		kbr: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Stephen White
Comment 1 2010-08-26 06:52:52 PDT
Created attachment 65555 [details] Patch
Kenneth Russell
Comment 2 2010-08-26 11:37:55 PDT
Comment on attachment 65555 [details] Patch This refactoring looks good overall but I'm concerned about putting a name as generic as Shader into the WebCore namespace. I will start sending out patches today which introduce a gpu2d sub-namespace into which I'm also planning to put some shader-related classes that will need to be merged with your work. Once that is in place I think it would be best to scope Shader (and the other accelerated 2D canvas classes) within that namespace. I wonder whether you can hold off this commit for a day or two. If not, I'll r+ it and we can refactor things later. Up to you.
Stephen White
Comment 3 2010-08-26 12:14:54 PDT
(In reply to comment #2) > (From update of attachment 65555 [details]) > This refactoring looks good overall but I'm concerned about putting a name as generic as Shader into the WebCore namespace. I will start sending out patches today which introduce a gpu2d sub-namespace into which I'm also planning to put some shader-related classes that will need to be merged with your work. Once that is in place I think it would be best to scope Shader (and the other accelerated 2D canvas classes) within that namespace. I wonder whether you can hold off this commit for a day or two. If not, I'll r+ it and we can refactor things later. Up to you. According to the EWS bots, it doesn't seem to conflict with anything yet. I don't mind refactoring after your stuff goes in, as long as this change doesn't slow you down too much.
Kenneth Russell
Comment 4 2010-08-26 13:06:27 PDT
Comment on attachment 65555 [details] Patch r+'ing after offline discussion.
Stephen White
Comment 5 2010-08-26 13:33:38 PDT
Committed r66130: <http://trac.webkit.org/changeset/66130>
WebKit Review Bot
Comment 6 2010-08-26 14:12:28 PDT
http://trac.webkit.org/changeset/66130 might have broken Chromium Mac Release
Note You need to log in before you can comment on or make changes to this bug.