[webkit-reviews] review canceled: [Bug 86050] [chromium] Move deferral-related logic out of Canvas2DLayerChromium : [Attachment 142597] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has canceled Stephen White
<senorblanco at chromium.org>'s request for review:
Bug 86050: [chromium] Move deferral-related logic out of Canvas2DLayerChromium
https://bugs.webkit.org/show_bug.cgi?id=86050

Attachment 142597: Patch
https://bugs.webkit.org/attachment.cgi?id=142597&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=142597&action=review


I'll refresh this with the simplifications Justin suggested.

>> Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.h:47
>> +	// always be retained in an SkAutoTUnref or similar (but not a WTF
smart pointer type).
> 
> I think this comment no longer applies.

You're right, it does not

>> Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.h:71
>> +	SkAutoTUnref<AcceleratedDeviceContext> m_deferredDeviceContext;
> 
> Is this ref count really necessary?  This object is already ref counted in
SkDeferredCanvas::DeferredDevice.  I am pretty sure there is no need to even
hold on to a reference to it in this class. Method deferredDeviceContext()
could be replaced with createDeferredDeviceContext().
> 
> Something like:
> 
> Canvas2DLayerBridge::createDeferredDeviceContext() {
>     ASSERT(m_deferralMode == Deferred);
>     return new AcceleratedDeviceContext(m_context.get(), m_layer.get(),
m_useDoubleBuffering)
> }

Hm - I think that would work, since ADC holds refs to everything it needs. 
I'll do that instead. Thanks!