Switching to SkDeferredCanvas factory API in preparation for constructor deprecation

Switching to SkDeferredCanvas factory API in preparation for constructor deprecation

R=senorblanco
BUG=270143
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155809

[Justin Novosad, 2013-08-08 17:48:36]

PTAL

[abarth-chromium, 2013-08-08 17:58:53]

LGTM for public/

We should add a per-file * OWNER for blink_skia_config.gyp

[Stephen White, 2013-08-08 18:04:59]

LGTM, but let's see if we can clean up the deferred canvas ownership somehow.

https://codereview.chromium.org/22634006/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp (right):

https://codereview.chromium.org/22634006/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp:69:
OwnPtr<Canvas2DLayerBridge> layerBridge = adoptPtr(new
Canvas2DLayerBridge(context, canvas, opacityMode));
Not new to this patch, but the ownership semantics of the deferred canvas are
really unclear here.

The canvas is allocated here, and passed to the Canavs2DLayerBridge constructor.
The bridge never refs it, but also never unrefs it, and neither does this
factory function. It seems like it's just temporarily unowned, until the
ImageBuffer refs it over in ImageBuffer, while Canvas2DLayerBridge keeps a bare
pointer to it. That seems really fragile. If you just create a bridge, and never
ref the canvas in it, it'll leak. If the bridge ever outlives the ImageBuffer,
it'll blow up (I realize that doesn't happen now, but the dependency is not
explicit).

Is there any way we could clean this up?

[Stephen White, 2013-08-08 18:05:37]

On 2013/08/08 18:04:59, Stephen White wrote:
> LGTM, but let's see if we can clean up the deferred canvas ownership somehow.
> 
>
https://codereview.chromium.org/22634006/diff/1/Source/core/platform/graphics...
> File Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp (right):
> 
>
https://codereview.chromium.org/22634006/diff/1/Source/core/platform/graphics...
> Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp:69:
> OwnPtr<Canvas2DLayerBridge> layerBridge = adoptPtr(new
> Canvas2DLayerBridge(context, canvas, opacityMode));
> Not new to this patch, but the ownership semantics of the deferred canvas are
> really unclear here.
> 
> The canvas is allocated here, and passed to the Canavs2DLayerBridge
constructor.
> The bridge never refs it, but also never unrefs it, and neither does this
> factory function. It seems like it's just temporarily unowned, until the
> ImageBuffer refs it over in ImageBuffer, while Canvas2DLayerBridge keeps a
bare
> pointer to it. That seems really fragile. If you just create a bridge, and
never
> ref the canvas in it, it'll leak. If the bridge ever outlives the ImageBuffer,
> it'll blow up (I realize that doesn't happen now, but the dependency is not
> explicit).
> 
> Is there any way we could clean this up?

s/ImageBuffer refs it/ImageBuffer adopts it/

[Justin Novosad, 2013-08-08 18:19:31]

On 2013/08/08 18:05:37, Stephen White wrote:
> On 2013/08/08 18:04:59, Stephen White wrote:
> > LGTM, but let's see if we can clean up the deferred canvas ownership
somehow.
> > 
> >
>
https://codereview.chromium.org/22634006/diff/1/Source/core/platform/graphics...
> > File Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp (right):
> > 
> >
>
https://codereview.chromium.org/22634006/diff/1/Source/core/platform/graphics...
> > Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp:69:
> > OwnPtr<Canvas2DLayerBridge> layerBridge = adoptPtr(new
> > Canvas2DLayerBridge(context, canvas, opacityMode));
> > Not new to this patch, but the ownership semantics of the deferred canvas
are
> > really unclear here.
> > 
> > The canvas is allocated here, and passed to the Canavs2DLayerBridge
> constructor.
> > The bridge never refs it, but also never unrefs it, and neither does this
> > factory function. It seems like it's just temporarily unowned, until the
> > ImageBuffer refs it over in ImageBuffer, while Canvas2DLayerBridge keeps a
> bare
> > pointer to it. That seems really fragile. If you just create a bridge, and
> never
> > ref the canvas in it, it'll leak. If the bridge ever outlives the
ImageBuffer,
> > it'll blow up (I realize that doesn't happen now, but the dependency is not
> > explicit).
> > 
> > Is there any way we could clean this up?
> 
> s/ImageBuffer refs it/ImageBuffer adopts it/

Yeah, I was just think that when I was looking at this code.  It would
definitely be more robust with fewer dumb pointers.  I have another refactoring
CL in flight that partially addresses this.

[commit-bot: I haz the power, 2013-08-08 18:21:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/junov@chromium.org/22634006/1

[commit-bot: I haz the power, 2013-08-09 01:07:14]

Change committed as 155809