WIP RasterCanvasLayerAllocator experiment 2

WIP RasterCanvasLayerAllocator experiment 2

Encapsulates allocating and freeing the raster backing for a canvas.
Allows a client's subclass to provide a situationally-appropriate
backing: a BITMAP on Windows, a CGBitmapContext on Mac, on Linux a
cairo_surface_t.

skia/ext proof of concept at https://codereview.chromium.org/2304273002/

BUG=skia:3940
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2309483002

[tomhudson, 2016-09-02 18:53:58]

Description was changed from

==========
WIP RasterCanvasLayerAllocator experiment 2

BUG=skia:3940
==========

to

==========
WIP RasterCanvasLayerAllocator experiment 2

BUG=skia:3940
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2309483002
==========

[tomhudson, 2016-09-16 12:48:28]

https://codereview.chromium.org/2309483002/diff/60001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/2309483002/diff/60001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:802: 
Should probably put an SkASSERT() here to guarantee that the bitmap backing, if
any, comes from the allocator, but that's complicated.

[reed1, 2016-09-16 13:14:59]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2016-09-16 13:15:00]

https://codereview.chromium.org/2309483002/diff/60001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/2309483002/diff/60001/include/core/SkCanvas.h...
include/core/SkCanvas.h:1697: std::unique_ptr<SkRasterCanvasLayerAllocator>
fLayerAllocator;
Why is the type SkRefCnt, but we have a unique_ptr to it? Perhaps it doesn't
need to inherit from SkRefCnt?

https://codereview.chromium.org/2309483002/diff/60001/include/core/SkRasterCa...
File include/core/SkRasterCanvasLayerAllocator.h (right):

https://codereview.chromium.org/2309483002/diff/60001/include/core/SkRasterCa...
include/core/SkRasterCanvasLayerAllocator.h:54: * clip and transform are in sync
between native and Skia representations.
// What is "buffer"?

I wonder if it would help document things if we had some alias types...

typedef void* NativeContext;
typedef void* LayerBuffer;  // not sure what to name this guy

https://codereview.chromium.org/2309483002/diff/60001/include/core/SkRasterCa...
include/core/SkRasterCanvasLayerAllocator.h:64: * Perform any OS-dependent
synchronization necessary to
// When is this called?

https://codereview.chromium.org/2309483002/diff/60001/include/core/SkRasterCa...
include/core/SkRasterCanvasLayerAllocator.h:72: * native context.
// When is this called?

https://codereview.chromium.org/2309483002/diff/60001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/2309483002/diff/60001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:699: , fLayerAllocator(nullptr)
Are any of these (nullptr) initializers needed, given that fLayerAllocator is a
smart container?

https://codereview.chromium.org/2309483002/diff/60001/src/core/SkRasterCanvas...
File src/core/SkRasterCanvasLayerAllocator.cpp (right):

https://codereview.chromium.org/2309483002/diff/60001/src/core/SkRasterCanvas...
src/core/SkRasterCanvasLayerAllocator.cpp:27: SkBitmapDevice* device = new
SkBitmapDevice(bitmap, props);
why do we allocate a device here?

[reed1, 2016-09-16 13:15:35]

Is there a corresponding WIP CL showing how this might be used in skia/ext? That
would help validate the api design.

[tomhudson, 2016-09-16 13:17:30]

Description was changed from

==========
WIP RasterCanvasLayerAllocator experiment 2

BUG=skia:3940
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2309483002
==========

to

==========
WIP RasterCanvasLayerAllocator experiment 2

skia/ext proof of concept at https://codereview.chromium.org/2304273002/

BUG=skia:3940
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2309483002
==========

[tomhudson, 2016-09-16 13:18:25]

Link added to description: https://codereview.chromium.org/2304273002/

[f(malita), 2016-09-16 13:39:00]

fmalita@chromium.org changed reviewers:
+ fmalita@chromium.org

[f(malita), 2016-09-16 13:39:01]

https://codereview.chromium.org/2309483002/diff/60001/include/core/SkRasterCa...
File include/core/SkRasterCanvasLayerAllocator.h (right):

https://codereview.chromium.org/2309483002/diff/60001/include/core/SkRasterCa...
include/core/SkRasterCanvasLayerAllocator.h:48: SkCanvas* CreateCanvas(const
SkImageInfo& info,
nit: non-static -> "createCanvas()"?

Also, would it make sense to return a sk_sp<SkCanvas> instead of raw ptr?

[tomhudson, 2016-09-28 20:33:55]

Description was changed from

==========
WIP RasterCanvasLayerAllocator experiment 2

skia/ext proof of concept at https://codereview.chromium.org/2304273002/

BUG=skia:3940
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2309483002
==========

to

==========
WIP RasterCanvasLayerAllocator experiment 2

Encapsulates allocating and freeing the raster backing for a canvas.
Allows a client's subclass to provide a situationally-appropriate
backing: a BITMAP on Windows, a CGBitmapContext on Mac, on Linux a
cairo_surface_t.

skia/ext proof of concept at https://codereview.chromium.org/2304273002/

BUG=skia:3940
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2309483002
==========

[tomhudson, 2016-09-28 21:23:58]

I changed [cC]reateCanvas() to return a smart pointer, as Florin suggested - but
the Chrome CreatePlatformCanvas() expects SkCanvas*, so we're going to throw
away that smartness until another fun round of refactoring is done.

https://codereview.chromium.org/2309483002/diff/60001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/2309483002/diff/60001/include/core/SkCanvas.h...
include/core/SkCanvas.h:1697: std::unique_ptr<SkRasterCanvasLayerAllocator>
fLayerAllocator;
On 2016/09/16 13:15:00, reed1 wrote:
> Why is the type SkRefCnt, but we have a unique_ptr to it? Perhaps it doesn't
> need to inherit from SkRefCnt?

Done.

https://codereview.chromium.org/2309483002/diff/60001/include/core/SkRasterCa...
File include/core/SkRasterCanvasLayerAllocator.h (right):

https://codereview.chromium.org/2309483002/diff/60001/include/core/SkRasterCa...
include/core/SkRasterCanvasLayerAllocator.h:48: SkCanvas* CreateCanvas(const
SkImageInfo& info,
On 2016/09/16 13:39:01, f(malita) wrote:
> nit: non-static -> "createCanvas()"?
> 
> Also, would it make sense to return a sk_sp<SkCanvas> instead of raw ptr?

Done.

Although it's really a weird member function, since it returns a thing that
takes unique ownership of the object. There's probably a saner way of
constructing these things.

https://codereview.chromium.org/2309483002/diff/60001/include/core/SkRasterCa...
include/core/SkRasterCanvasLayerAllocator.h:64: * Perform any OS-dependent
synchronization necessary to
On 2016/09/16 13:15:00, reed1 wrote:
> // When is this called?

Acknowledged.

https://codereview.chromium.org/2309483002/diff/60001/include/core/SkRasterCa...
include/core/SkRasterCanvasLayerAllocator.h:72: * native context.
On 2016/09/16 13:15:00, reed1 wrote:
> // When is this called?

Acknowledged.

https://codereview.chromium.org/2309483002/diff/60001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/2309483002/diff/60001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:699: , fLayerAllocator(nullptr)
On 2016/09/16 13:15:00, reed1 wrote:
> Are any of these (nullptr) initializers needed, given that fLayerAllocator is
a
> smart container?

Done.