Simplify platform_canvas.h by recognizing that PlatformCanvas does not actually extend

Simplify platform_canvas.h by recognizing that PlatformCanvas does not actually extend
SkCanvas in any way, other than provide a host of constructors (and delayed constructors
in the form of 'initialize' methods).

These late initializers are a problem, as SkCanvas is deprecating its setDevice() call,
moving to model where the backingstore/device for the canvas must be created before the
canvas is created. This is necessary to allow skia to continue to extend SkCanvas for
its backends (e.g. GPU, PDF, Picture, Pipe, etc.).

The practical change in this CL is to make PlatformCanvas just a typedef for SkCanvas,
and change the call-sites that want to call initialize() to instead create the canvas
using one of the provided Factory functions (e.g. CreatePlatformCanvas). The modifier
Platform is maintained, to document that this canvas may be backed by platform-specific
pixels (e.g. allocated by GDI or cairo).
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=167669

[reed1, 2012-10-30 17:48:08]

control https://chromiumcodereview.appspot.com/10823204

[reed1, 2012-11-01 15:28:26]

Its a biggie, but the organizing principles were:

1. PlatformCanvas was not functionally a subclass, as much as a wrapper for
constructors and factories. This CL makes that explicit.

2. This reorg makes it clearer when a factory call will crash or return_null if
it fails to allocate the backing pixels.

3. This allows us to move forward with changes where we *do* want subclasses of
SkCanvas for GPU, Pictures, etc.

4. Part of optimizing #3 is to completely remove SkCanvas::setDevice(), which
allowed clients to mix-and-match canvas subclasses w/ device subclasses
(yikes!). Since this CL removes "initialize()", we move the major caller of
setDevice() from chrome.

[sky, 2012-11-01 17:41:39]

I didn't look at the whole thing, just a couple of higher level things.

https://codereview.chromium.org/11138024/diff/77125/chrome/renderer/automatio...
File chrome/renderer/automation/automation_renderer_helper.cc (right):

https://codereview.chromium.org/11138024/diff/77125/chrome/renderer/automatio...
chrome/renderer/automation/automation_renderer_helper.cc:70: SkCanvas* canvas =
skia::CreatePlatformCanvas(new_size.width, new_size.height,
Is it possible to return a value like we did before so that folks don't have to
remember to wrap in an SkAutoUnref?

https://codereview.chromium.org/11138024/diff/77125/skia/ext/bitmap_platform_...
File skia/ext/bitmap_platform_device_win.cc (right):

https://codereview.chromium.org/11138024/diff/77125/skia/ext/bitmap_platform_...
skia/ext/bitmap_platform_device_win.cc:273: SkCanvas* CreatePlatformCanvas(int
width, int height, bool is_opaque,
On windows we need to support creating with the BitmapPlatformDevice and
SkDevice. See skia.gyp where it excludes platform_canvas_win when use_aura == 1
and use_canvas_skia are set.

https://codereview.chromium.org/11138024/diff/77125/skia/ext/platform_canvas.h
File skia/ext/platform_canvas.h (right):

https://codereview.chromium.org/11138024/diff/77125/skia/ext/platform_canvas....
skia/ext/platform_canvas.h:38: SK_API
nit: see base/file_util.h for how we generally use declarations like this, eg:
SK_API SkCanvas* Cre...

https://codereview.chromium.org/11138024/diff/77125/skia/ext/platform_canvas....
skia/ext/platform_canvas.h:39: SkCanvas* CreatePlatformCanvas(int width, int
height, bool is_opaque,
nit: when you wrap, one param per line.

https://codereview.chromium.org/11138024/diff/77125/skia/ext/platform_canvas....
skia/ext/platform_canvas.h:41: OnFailureType = CRASH_ON_FAILURE);
Style guide says no default values.

[reed1, 2012-11-01 21:07:21]

https://codereview.chromium.org/11138024/diff/77125/chrome/renderer/automatio...
File chrome/renderer/automation/automation_renderer_helper.cc (right):

https://codereview.chromium.org/11138024/diff/77125/chrome/renderer/automatio...
chrome/renderer/automation/automation_renderer_helper.cc:70: SkCanvas* canvas =
skia::CreatePlatformCanvas(new_size.width, new_size.height,
On 2012/11/01 17:41:39, sky wrote:
> Is it possible to return a value like we did before so that folks don't have
to
> remember to wrap in an SkAutoUnref?

Makes sense. I'll see what I can come up with.

https://codereview.chromium.org/11138024/diff/77125/skia/ext/bitmap_platform_...
File skia/ext/bitmap_platform_device_win.cc (right):

https://codereview.chromium.org/11138024/diff/77125/skia/ext/bitmap_platform_...
skia/ext/bitmap_platform_device_win.cc:273: SkCanvas* CreatePlatformCanvas(int
width, int height, bool is_opaque,
On 2012/11/01 17:41:39, sky wrote:
> On windows we need to support creating with the BitmapPlatformDevice and
> SkDevice. See skia.gyp where it excludes platform_canvas_win when use_aura ==
1
> and use_canvas_skia are set.

Not sure what you're saying. I have also also updated platform_canvas_skia.cc to
work with the new interface. Did I miss a try-bot or test to catch what you are
referring to?

https://codereview.chromium.org/11138024/diff/77125/skia/ext/platform_canvas.h
File skia/ext/platform_canvas.h (right):

https://codereview.chromium.org/11138024/diff/77125/skia/ext/platform_canvas....
skia/ext/platform_canvas.h:38: SK_API
On 2012/11/01 17:41:39, sky wrote:
> nit: see base/file_util.h for how we generally use declarations like this, eg:
> SK_API SkCanvas* Cre...

Done.

https://codereview.chromium.org/11138024/diff/77125/skia/ext/platform_canvas....
skia/ext/platform_canvas.h:39: SkCanvas* CreatePlatformCanvas(int width, int
height, bool is_opaque,
On 2012/11/01 17:41:39, sky wrote:
> nit: when you wrap, one param per line.

Done.

https://codereview.chromium.org/11138024/diff/77125/skia/ext/platform_canvas....
skia/ext/platform_canvas.h:41: OnFailureType = CRASH_ON_FAILURE);
On 2012/11/01 17:41:39, sky wrote:
> Style guide says no default values.

Done.

[sky, 2012-11-01 21:44:57]

https://codereview.chromium.org/11138024/diff/77125/skia/ext/bitmap_platform_...
File skia/ext/bitmap_platform_device_win.cc (right):

https://codereview.chromium.org/11138024/diff/77125/skia/ext/bitmap_platform_...
skia/ext/bitmap_platform_device_win.cc:273: SkCanvas* CreatePlatformCanvas(int
width, int height, bool is_opaque,
On 2012/11/01 21:07:21, reed1 wrote:
> On 2012/11/01 17:41:39, sky wrote:
> > On windows we need to support creating with the BitmapPlatformDevice and
> > SkDevice. See skia.gyp where it excludes platform_canvas_win when use_aura
==
> 1
> > and use_canvas_skia are set.
> 
> Not sure what you're saying. I have also also updated platform_canvas_skia.cc
to
> work with the new interface. Did I miss a try-bot or test to catch what you
are
> referring to?

Windows has two configurations. One uses PlatformCanvasSkia, the other uses
PlatformCanvasWin. Which one is used is controlled by the use_aura GYP_DEFINE. I
believe your code forces using PlatformCanvasWin always. The win_aura bot should
build this configuration. I'm not sure that using PlatformCanvasWin always would
cause a failure, but it would certainly cause lots of unnecessary allocations.

[reed1, 2012-11-02 15:49:39]

https://codereview.chromium.org/11138024/diff/77125/chrome/renderer/automatio...
File chrome/renderer/automation/automation_renderer_helper.cc (right):

https://codereview.chromium.org/11138024/diff/77125/chrome/renderer/automatio...
chrome/renderer/automation/automation_renderer_helper.cc:70: SkCanvas* canvas =
skia::CreatePlatformCanvas(new_size.width, new_size.height,
On 2012/11/01 21:07:21, reed1 wrote:
> On 2012/11/01 17:41:39, sky wrote:
> > Is it possible to return a value like we did before so that folks don't have
> to
> > remember to wrap in an SkAutoUnref?
> 
> Makes sense. I'll see what I can come up with.

ScopedPlatformCanvas

Done.

[reed1, 2012-11-02 20:17:16]

All comments addressed, modulo I need to study more to understand the
repercussions for aura.

PTAL

[sky, 2012-11-05 14:43:10]

I only have nits.

https://codereview.chromium.org/11138024/diff/93090/skia/ext/bitmap_platform_...
File skia/ext/bitmap_platform_device_win.cc (right):

https://codereview.chromium.org/11138024/diff/93090/skia/ext/bitmap_platform_...
skia/ext/bitmap_platform_device_win.cc:274: HANDLE shared_section, OnFailureType
failureType) {
nit: indentation. Also, when you wrap each param should be on its own line.

https://codereview.chromium.org/11138024/diff/93090/skia/ext/platform_canvas.h
File skia/ext/platform_canvas.h (right):

https://codereview.chromium.org/11138024/diff/93090/skia/ext/platform_canvas....
skia/ext/platform_canvas.h:42: OnFailureType);
The style guide also says you should name all args.

https://codereview.chromium.org/11138024/diff/93090/skia/ext/platform_canvas....
skia/ext/platform_canvas.h:93:
SkAutoTUnref<SkCanvas>(CreatePlatformCanvas(width, height, is_opaque)) {}
nit: indent 4 and ':' on next line.

[reed1, 2012-11-05 19:14:13]

https://codereview.chromium.org/11138024/diff/93090/skia/ext/bitmap_platform_...
File skia/ext/bitmap_platform_device_win.cc (right):

https://codereview.chromium.org/11138024/diff/93090/skia/ext/bitmap_platform_...
skia/ext/bitmap_platform_device_win.cc:274: HANDLE shared_section, OnFailureType
failureType) {
On 2012/11/05 14:43:10, sky wrote:
> nit: indentation. Also, when you wrap each param should be on its own line.

Done.

https://codereview.chromium.org/11138024/diff/93090/skia/ext/platform_canvas.h
File skia/ext/platform_canvas.h (right):

https://codereview.chromium.org/11138024/diff/93090/skia/ext/platform_canvas....
skia/ext/platform_canvas.h:42: OnFailureType);
On 2012/11/05 14:43:10, sky wrote:
> The style guide also says you should name all args.

Done.

https://codereview.chromium.org/11138024/diff/93090/skia/ext/platform_canvas....
skia/ext/platform_canvas.h:93:
SkAutoTUnref<SkCanvas>(CreatePlatformCanvas(width, height, is_opaque)) {}
On 2012/11/05 14:43:10, sky wrote:
> nit: indent 4 and ':' on next line.

Done.

[reed1, 2012-11-09 22:12:18]

nits addressed. are we good?

[sky, 2012-11-12 16:36:32]

LGTM - You'll need to wait for Brett though.

[brettw, 2012-11-12 21:08:52]

LGTM, I only looked at platform_canvas.h but it looks fine.

https://codereview.chromium.org/11138024/diff/87146/skia/ext/platform_canvas.h
File skia/ext/platform_canvas.h (right):

https://codereview.chromium.org/11138024/diff/87146/skia/ext/platform_canvas....
skia/ext/platform_canvas.h:18: /*
Can you use //-style comments instead? We only do it your way in .c files.

[reed1, 2012-11-14 14:01:43]

https://codereview.chromium.org/11138024/diff/87146/skia/ext/platform_canvas.h
File skia/ext/platform_canvas.h (right):

https://codereview.chromium.org/11138024/diff/87146/skia/ext/platform_canvas....
skia/ext/platform_canvas.h:18: /*
On 2012/11/12 21:08:52, brettw wrote:
> Can you use //-style comments instead? We only do it your way in .c files.

Done.