Bug 61953 – cleanup DrawingBufferChromium, correct comment

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[John Bates]

cleanup DrawingBufferChromium, correct comment

[John Bates]

Created attachment 95794 [details]
Patch

[John Bates]

Ready for review

[Kenneth Russell]

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.

[James Robinson]

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.

[John Bates]

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.

[John Bates]

Created attachment 95804 [details]
Patch

[James Robinson]

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.

[John Bates]

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.

[John Bates]

Created attachment 95823 [details]
Patch

[John Bates]

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.

[James Robinson]

Comment on attachment 95823 [details]
Patch

Rock

[WebKit Commit Bot]

Comment on attachment 95823 [details]
Patch

Clearing flags on attachment: 95823

Committed r87987: <http://trac.webkit.org/changeset/87987>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.