Refactor SkCanvas so that backends don't need to override it.

Refactor SkCanvas so that backends don't need to override it.

Methods or classes that should go away are marked deprecated. The only thing I know of that breaks backward compatibility is SkCanvas((SkDevice*)NULL), but that is fairly unlikely to occur in the wild because that constructor had a default value of NULL.

Committed: http://code.google.com/p/skia/source/detail?r=604

[Steve VanDeBogart, 2010-09-08 21:26:44]

Let me know what you think.

[reed, 2010-09-09 16:25:57]

http://codereview.appspot.com/2103045/diff/1/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

http://codereview.appspot.com/2103045/diff/1/include/core/SkCanvas.h#newcode54
include/core/SkCanvas.h:54: public:
Perhaps we just add another blank constructor

SkCanvas()

rather than having the (factor = NULL) argument. Very minor, just makes it a
little easier to say that we don't take NULL arguments for either the factory,
or the device, variants.

http://codereview.appspot.com/2103045/diff/1/include/core/SkDevice.h
File include/core/SkDevice.h (right):

http://codereview.appspot.com/2103045/diff/1/include/core/SkDevice.h#newcode40
include/core/SkDevice.h:40: bool isOpaque, bool isForLayer);
Perhaps we roll these two bools into a bit-field enum, for future expansion, and
to possibly make the call sites more readable.

enum Flags {
    kOpaque_Flag = 1 << 0,
    kForLayer_Flag = 1 << 1,
    etc.
};

Not necessary for this CL, but what do you think in general?

http://codereview.appspot.com/2103045/diff/1/include/core/SkDevice.h#newcode42
include/core/SkDevice.h:42: 
This is fine.

We might also want to make the base class pure-virtual. i.e.

newDevice(...) = 0;

And then show a concreate subclass that makes "raster" devices.

e.g. SkRasterDeviceFactory

This could edge us towards making SkDevice more abstract, and separating out the
notion of a raster-subclass (e.g. SkRasterDevice).

Not really necessary for this CL, but I'd like to hear what you think of that
direction.

http://codereview.appspot.com/2103045/diff/1/include/utils/SkGLCanvas.h
File include/utils/SkGLCanvas.h (right):

http://codereview.appspot.com/2103045/diff/1/include/utils/SkGLCanvas.h#newco...
include/utils/SkGLCanvas.h:33: virtual bool setViewport(int width, int height) {
I think we can make setViewport be deprecated, or at least a non-virtual helper
in SkCanvas, since what you typed below is generic, now that we have a device
factory.

http://codereview.appspot.com/2103045/diff/1/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

http://codereview.appspot.com/2103045/diff/1/src/core/SkCanvas.cpp#newcode412
src/core/SkCanvas.cpp:412: factory->safeRef();
I'm starting to use SkSafeRef(factory) instead of the above, since I'm told
proper C++ may remove the null-check on "this".

http://codereview.appspot.com/2103045/diff/1/src/core/SkCanvas.cpp#newcode433
src/core/SkCanvas.cpp:433: this->init(device)->unref();
Do we need a null-check on the return value from getDeviceFactory() ?

http://codereview.appspot.com/2103045/diff/1/src/core/SkCanvas.cpp#newcode441
src/core/SkCanvas.cpp:441: fBounder->safeUnref();
Same comment about C++ and null-this-checks. Try SkSafeUnref(fDeviceFactory)

http://codereview.appspot.com/2103045/diff/1/src/core/SkDevice.cpp
File src/core/SkDevice.cpp (right):

http://codereview.appspot.com/2103045/diff/1/src/core/SkDevice.cpp#newcode117
src/core/SkDevice.cpp:117: bitmap.allocPixels();
Especially this allocPixels() call makes this very raster-specific. Another
motivation for calling out this particular impl as for "raster" devices.

[James Hawkins, 2010-09-10 19:44:07]

http://codereview.appspot.com/2103045/diff/1/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

http://codereview.appspot.com/2103045/diff/1/include/core/SkCanvas.h#newcode101
include/core/SkCanvas.h:101: /** Depreceated - Specify a bitmap for the canvas
to draw into. This is a
s/Depreceated/Deprecated/

http://codereview.appspot.com/2103045/diff/1/include/core/SkCanvas.h#newcode102
include/core/SkCanvas.h:102: help method for setDevice(), and it creates a
device for the bitmap by
s/help/helper/

[Steve VanDeBogart, 2010-09-10 23:16:16]

http://codereview.appspot.com/2103045/diff/1/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

http://codereview.appspot.com/2103045/diff/1/include/core/SkCanvas.h#newcode54
include/core/SkCanvas.h:54: public:
On 2010/09/09 16:25:57, reed wrote:
> Perhaps we just add another blank constructor
> 
> SkCanvas()
> 
> rather than having the (factor = NULL) argument. Very minor, just makes it a
> little easier to say that we don't take NULL arguments for either the factory,
> or the device, variants.

Ideally, I'd like the SkCanvas constructor to require either a device or device
factory - this way you always explicitly describe what backend you are using. 
Doing that would break backward compatiblity though, so I didn't do it.  If you
disagree with this and think defaulting to the raster backend is the right thing
to do, then a blank constructor is the way to go, yes.

http://codereview.appspot.com/2103045/diff/1/include/core/SkCanvas.h#newcode101
include/core/SkCanvas.h:101: /** Depreceated - Specify a bitmap for the canvas
to draw into. This is a
On 2010/09/10 19:44:07, James Hawkins wrote:
> s/Depreceated/Deprecated/

Done.

http://codereview.appspot.com/2103045/diff/1/include/core/SkCanvas.h#newcode102
include/core/SkCanvas.h:102: help method for setDevice(), and it creates a
device for the bitmap by
On 2010/09/10 19:44:07, James Hawkins wrote:
> s/help/helper/

Done.

http://codereview.appspot.com/2103045/diff/1/include/core/SkDevice.h
File include/core/SkDevice.h (right):

http://codereview.appspot.com/2103045/diff/1/include/core/SkDevice.h#newcode40
include/core/SkDevice.h:40: bool isOpaque, bool isForLayer);
On 2010/09/09 16:25:57, reed wrote:
> Perhaps we roll these two bools into a bit-field enum, for future expansion,
and
> to possibly make the call sites more readable.
> 
> enum Flags {
>     kOpaque_Flag = 1 << 0,
>     kForLayer_Flag = 1 << 1,
>     etc.
> };
> 
> Not necessary for this CL, but what do you think in general?

I made this change, but then decided I didn't like it.  In each newDevice body,
you need to break out each bit in order to use it.  SkCanvas::createDevice still
takes two bools (for backward compatibility), so you need to test each bool and
set the flag set there.  It seems that the number of flags isn't likely to grow.
 If it does by one or two, then the change is probably worthwhile, but not until
then.

http://codereview.appspot.com/2103045/diff/1/include/core/SkDevice.h#newcode42
include/core/SkDevice.h:42: 
On 2010/09/09 16:25:57, reed wrote:
> This is fine.
> 
> We might also want to make the base class pure-virtual. i.e.
> 
> newDevice(...) = 0;
> 
> And then show a concreate subclass that makes "raster" devices.
> 
> e.g. SkRasterDeviceFactory

SGTM Done.

> This could edge us towards making SkDevice more abstract, and separating out
the
> notion of a raster-subclass (e.g. SkRasterDevice).
> 
> Not really necessary for this CL, but I'd like to hear what you think of that
> direction.

I think makeing SkDevice pure virtual is the right direction to take things.  I
might have done it, but it would break backward compatibility.

http://codereview.appspot.com/2103045/diff/1/include/utils/SkGLCanvas.h
File include/utils/SkGLCanvas.h (right):

http://codereview.appspot.com/2103045/diff/1/include/utils/SkGLCanvas.h#newco...
include/utils/SkGLCanvas.h:33: virtual bool setViewport(int width, int height) {
On 2010/09/09 16:25:57, reed wrote:
> I think we can make setViewport be deprecated, or at least a non-virtual
helper
> in SkCanvas, since what you typed below is generic, now that we have a device
> factory.

SkProxyCanvas needs to override these.  But otherwise, I made SkCanvas the only
implementer.

http://codereview.appspot.com/2103045/diff/1/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

http://codereview.appspot.com/2103045/diff/1/src/core/SkCanvas.cpp#newcode412
src/core/SkCanvas.cpp:412: factory->safeRef();
On 2010/09/09 16:25:57, reed wrote:
> I'm starting to use SkSafeRef(factory) instead of the above, since I'm told
> proper C++ may remove the null-check on "this".

Done.

Der - of course.  According to some random Internet comments the behavior is
undefined.  Maybe that method should be removed.  I'll send a cleanup CL to get
rid of it.

http://codereview.appspot.com/2103045/diff/1/src/core/SkCanvas.cpp#newcode433
src/core/SkCanvas.cpp:433: this->init(device)->unref();
On 2010/09/09 16:25:57, reed wrote:
> Do we need a null-check on the return value from getDeviceFactory() ?

Hmm... new is supposed to throw std::bad_alloc on out of memory.  We could make
getDeviceFactory return NULL for a bad set of parameters, but one could just
call new device(...) with the same set of parameters and the device
would have to do something with that.  It's easier to not try to catch bad
combinations in the factory, since the device will have to deal with it anyway.

http://codereview.appspot.com/2103045/diff/1/src/core/SkCanvas.cpp#newcode441
src/core/SkCanvas.cpp:441: fBounder->safeUnref();
On 2010/09/09 16:25:57, reed wrote:
> Same comment about C++ and null-this-checks. Try SkSafeUnref(fDeviceFactory)

Done.

[reed, 2010-09-17 21:21:24]

LGTM