Bug 44682 - [CHROMIUM] Canvas2D shaders should have their own classes
Summary: [CHROMIUM] Canvas2D shaders should have their own classes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Stephen White
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-26 06:18 PDT by Stephen White
Modified: 2010-08-26 14:12 PDT (History)
5 users (show)

See Also:


Attachments
Patch (24.29 KB, patch)
2010-08-26 06:52 PDT, Stephen White
kbr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephen White 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.
Comment 1 Stephen White 2010-08-26 06:52:52 PDT
Created attachment 65555 [details]
Patch
Comment 2 Kenneth Russell 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.
Comment 3 Stephen White 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.
Comment 4 Kenneth Russell 2010-08-26 13:06:27 PDT
Comment on attachment 65555 [details]
Patch

r+'ing after offline discussion.
Comment 5 Stephen White 2010-08-26 13:33:38 PDT
Committed r66130: <http://trac.webkit.org/changeset/66130>
Comment 6 WebKit Review Bot 2010-08-26 14:12:28 PDT
http://trac.webkit.org/changeset/66130 might have broken Chromium Mac Release