Summary: | cleanup DrawingBufferChromium, correct comment | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | John Bates <jbates> | ||||||||
Component: | New Bugs | Assignee: | John Bates <jbates> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, jamesr, kbr | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
John Bates
2011-06-02 13:05:47 PDT
Created attachment 95794 [details]
Patch
Ready for review Comment on attachment 95794 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95794&action=review > Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:46 > +class Canvas2DLayerChromium { }; I doubt this will compile. RefPtr requires certain methods. Comment on attachment 95794 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95794&action=review Patch looks great, but R- for the odd forward declaration and lack of verbosity in the ChangeLog. Thanks a bunch for cleaning this up. > Source/WebCore/ChangeLog:8 > + https://bugs.webkit.org/show_bug.cgi?id=61953 > + > + * platform/graphics/chromium/DrawingBufferChromium.cpp: please put a more verbose description here if what this patch does. > Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:46 > +class Canvas2DLayerChromium { }; To forward declare Canvas2DLayerChromium just do: class Canvas2DLayerChromium; I don't think you need to do this, however - do you? I think all the references to Canvas2DLayerChromium are guarded by #if USE(ACCELERATED_COMPOSITING), or at least they should be. Comment on attachment 95794 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95794&action=review >>> Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:46 >>> +class Canvas2DLayerChromium { }; >> >> I doubt this will compile. RefPtr requires certain methods. > > To forward declare Canvas2DLayerChromium just do: > > class Canvas2DLayerChromium; > > I don't think you need to do this, however - do you? I think all the references to Canvas2DLayerChromium are guarded by #if USE(ACCELERATED_COMPOSITING), or at least they should be. It wasn't a forward declare, it was a dirty attempt to avoid referring to USE(ACCELERATED_COMPOSITING) in DrawingBuffer.h. But, Ken's right, it won't compile since it doesn't have the RefPtr methods. I'll just use #if USE(ACCELERATED_COMPOSITING) in DrawingBuffer.h. Created attachment 95804 [details]
Patch
Comment on attachment 95804 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95804&action=review > Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:83 > + , m_platformLayer(0) You don't need to 0-initialize RefPtr<>s, and in fact this'll stop compiling when we get stricter about RefPtr/PassRefPtr usage. Just leave the initialization out - the default c'tor for RefPtr does the right thing. Comment on attachment 95794 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95794&action=review >> Source/WebCore/ChangeLog:8 >> + * platform/graphics/chromium/DrawingBufferChromium.cpp: > > please put a more verbose description here if what this patch does. Done. Created attachment 95823 [details]
Patch
Comment on attachment 95804 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95804&action=review >> Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:83 >> + , m_platformLayer(0) > > You don't need to 0-initialize RefPtr<>s, and in fact this'll stop compiling when we get stricter about RefPtr/PassRefPtr usage. Just leave the initialization out - the default c'tor for RefPtr does the right thing. Done. Comment on attachment 95823 [details]
Patch
Rock
Comment on attachment 95823 [details] Patch Clearing flags on attachment: 95823 Committed r87987: <http://trac.webkit.org/changeset/87987> All reviewed patches have been landed. Closing bug. |