Summary: | [chromium] Composited layers should handle their own drawing | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Vangelis Kokkevis <vangelis> | ||||||||
Component: | WebCore Misc. | Assignee: | Vangelis Kokkevis <vangelis> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | amarinichev, hclam, jamesr, kbr, senorblanco, vrk, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
Vangelis Kokkevis
2010-08-17 19:28:29 PDT
Created attachment 64665 [details]
Proposed patch
Attachment 64665 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/chromium/LayerRendererChromium.h:58: Should have a space between // and comment [whitespace/comments] [4]
Total errors found: 1 in 15 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 64734 [details]
Proposed patch - fixed style issue
Comment on attachment 64734 [details]
Proposed patch - fixed style issue
The code reorganization looks good in general. Trusting that your testing covers the changes since it's impossible for a review to catch all issues in a change this large.
A few mostly stylistic issues.
The naming of CanvasLayerChromium::CanvasLayerSharedValues, ContentLayerChromium::ContentLayerSharedValues and LayerChromium::SharedValues is inconsistent. Given that you already have scoping within the class, I recommend renaming them all to SharedValues. These should all be structs, not classes, and though this isn't mentioned in the WebKit coding guidelines, the members shouldn't have the "m_" prefix.
WebCore/platform/graphics/chromium/LayerRendererChromium.h:153
+ CanvasLayerChromium::CanvasLayerSharedValues m_canvasLayerSharedValues;
I recommend changing these to OwnPtr<T>, and instantiating them in LayerRendererChromium::initializeSharedObjects. (For example: m_layerSharedValues = adoptPtr(new LayerChromium::SharedValues()).) This will let you make the OpenGL context current before initializing them. You can then clear out the OwnPtrs in cleanupSharedObjects, again giving you the chance to make the OpenGL context current. Then you can move the logic in initializeSharedObjects and cleanupSharedObjects in the LayerChromium subclasses into the constructors and destructors of the SharedValues structs. This will encapsulate this logic more cleanly.
WebCore/platform/graphics/chromium/LayerRendererChromium.h:159
+ #define GLC(x) { x, LayerRendererChromium::debugGLCall(#x, __FILE__, __LINE__); }
Does this macro definition style handle the trailing semicolon at the invocations on all platforms? Could you use it with assignments (like "GLC(val = glGetUniformLocation(...))")? Do you need to parenthesize (x) as I believe is common in some macros?
WebCore/platform/graphics/chromium/LayerChromium.h:202
+ static const unsigned m_texCoordAttribLocation;
These should be prefixed with s_.
Created attachment 64810 [details]
Proposed patch addressing review comments
Thanks for the review and sorry about the size of this beast. (In reply to comment #4) > (From update of attachment 64734 [details]) > The code reorganization looks good in general. Trusting that your testing covers the changes since it's impossible for a review to catch all issues in a change this large. > > A few mostly stylistic issues. > > The naming of CanvasLayerChromium::CanvasLayerSharedValues, ContentLayerChromium::ContentLayerSharedValues and LayerChromium::SharedValues is inconsistent. Given that you already have scoping within the class, I recommend renaming them all to SharedValues. These should all be structs, not classes, and though this isn't mentioned in the WebKit coding guidelines, the members shouldn't have the "m_" prefix. Done. Classes renamed to SharedValues. I did however keep them as classes, made all the members private and added accessors for them. That way, code using the shared values cannot inadvertently change them. > > WebCore/platform/graphics/chromium/LayerRendererChromium.h:153 > + CanvasLayerChromium::CanvasLayerSharedValues m_canvasLayerSharedValues; > I recommend changing these to OwnPtr<T>, and instantiating them in LayerRendererChromium::initializeSharedObjects. (For example: m_layerSharedValues = adoptPtr(new LayerChromium::SharedValues()).) This will let you make the OpenGL context current before initializing them. You can then clear out the OwnPtrs in cleanupSharedObjects, again giving you the chance to make the OpenGL context current. Then you can move the logic in initializeSharedObjects and cleanupSharedObjects in the LayerChromium subclasses into the constructors and destructors of the SharedValues structs. This will encapsulate this logic more cleanly. I like this idea a lot. I eliminated the initialize/cleanupSharedObjects method and added the logic in the SharedValues constructor/destructor. > > > WebCore/platform/graphics/chromium/LayerRendererChromium.h:159 > + #define GLC(x) { x, LayerRendererChromium::debugGLCall(#x, __FILE__, __LINE__); } > Does this macro definition style handle the trailing semicolon at the invocations on all platforms? Could you use it with assignments (like "GLC(val = glGetUniformLocation(...))")? Do you need to parenthesize (x) as I believe is common in some macros? > I parenthesized (x) so now expressions of the type GLC(x = gl*()) work. Trailing semicolon seems to compile fine with both GCC and msvc. > > WebCore/platform/graphics/chromium/LayerChromium.h:202 > + static const unsigned m_texCoordAttribLocation; > These should be prefixed with s_. Done. Comment on attachment 64810 [details]
Proposed patch addressing review comments
Looks good. r=me. I noticed that the method list in the ChangeLog is slightly out of date; this isn't critical, but you can fix this by saving off your text edits to the ChangeLog, svn reverting it, and running prepare-ChangeLog again. Feel free to commit using webkit-patch after this rather than submitting another patch for the commit queue. Removing cq? since you are a committer and can set it yourself if you want.
Committed r65717: <http://trac.webkit.org/changeset/65717> |