make SkDeviceFactory reference counted

[Steve VanDeBogart, 2011-04-26 00:26:45]

Of course this will require a Chrome side change as well..

LGTM with one comment.

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

http://codereview.appspot.com/4436056/diff/9/src/core/SkCanvas.cpp#newcode438
src/core/SkCanvas.cpp:438: if (factory) {
factory should never be NULL (if it is, you have a misbehaving device).  Maybe
just SkASSERT(factory); ?  Or leave this the way it was?

[mike3, 2011-04-26 00:34:20]

Thought about that. We have setDeviceFactory() which someone could pass NULL to
as well. I am considering changing the 
On 2011/04/26 00:26:45, Steve VanDeBogart wrote:
> Of course this will require a Chrome side change as well..
> 
> LGTM with one comment.
> 
> http://codereview.appspot.com/4436056/diff/9/src/core/SkCanvas.cpp
> File src/core/SkCanvas.cpp (right):
> 
> http://codereview.appspot.com/4436056/diff/9/src/core/SkCanvas.cpp#newcode438
> src/core/SkCanvas.cpp:438: if (factory) {
> factory should never be NULL (if it is, you have a misbehaving device).  Maybe
> just SkASSERT(factory); ?  Or leave this the way it was?

[mike3, 2011-04-26 00:35:13]

Ignore that garbled message.

Its a good point. Uploaded a change to make this consistent with the bitmap
constructor.

On 2011/04/26 00:34:20, mike3 wrote:
> Thought about that. We have setDeviceFactory() which someone could pass NULL
to
> as well. I am considering changing the 
> On 2011/04/26 00:26:45, Steve VanDeBogart wrote:
> > Of course this will require a Chrome side change as well..
> > 
> > LGTM with one comment.
> > 
> > http://codereview.appspot.com/4436056/diff/9/src/core/SkCanvas.cpp
> > File src/core/SkCanvas.cpp (right):
> > 
> >
http://codereview.appspot.com/4436056/diff/9/src/core/SkCanvas.cpp#newcode438
> > src/core/SkCanvas.cpp:438: if (factory) {
> > factory should never be NULL (if it is, you have a misbehaving device). 
Maybe
> > just SkASSERT(factory); ?  Or leave this the way it was?

[bsalomon, 2011-04-26 12:27:56]

LGTM. (Of course needs corresponding Chrome changes on next DEPS roll)

I was thinking that we suffer a bit from not being able to have pure virtuals on
SkDevice. SkDevice is serving as both the raster implementation and the
interface definition. We get no errors or warnings if a subclass forgets to
implement something (or a new virtual is added to the SkDevice). It might not be
a bad idea for us to create an SkBaseDevice and make everyone inherit from it.
It might not quite so easy since PDF and GPU probably rely on quite a bit of
functionality in SkDevice and it'd take time to figure out what should be
promoted up to the base class.

On 2011/04/26 00:35:13, mike3 wrote:
> Ignore that garbled message.
> 
> Its a good point. Uploaded a change to make this consistent with the bitmap
> constructor.
> 
> On 2011/04/26 00:34:20, mike3 wrote:
> > Thought about that. We have setDeviceFactory() which someone could pass NULL
> to
> > as well. I am considering changing the 
> > On 2011/04/26 00:26:45, Steve VanDeBogart wrote:
> > > Of course this will require a Chrome side change as well..
> > > 
> > > LGTM with one comment.
> > > 
> > > http://codereview.appspot.com/4436056/diff/9/src/core/SkCanvas.cpp
> > > File src/core/SkCanvas.cpp (right):
> > > 
> > >
> http://codereview.appspot.com/4436056/diff/9/src/core/SkCanvas.cpp#newcode438
> > > src/core/SkCanvas.cpp:438: if (factory) {
> > > factory should never be NULL (if it is, you have a misbehaving device). 
> Maybe
> > > just SkASSERT(factory); ?  Or leave this the way it was?

[Steve VanDeBogart, 2011-04-26 16:29:55]

On 2011/04/26 12:27:56, bsalomon wrote:
> LGTM. (Of course needs corresponding Chrome changes on next DEPS roll)
> 
> I was thinking that we suffer a bit from not being able to have pure virtuals
on
> SkDevice. SkDevice is serving as both the raster implementation and the
> interface definition. We get no errors or warnings if a subclass forgets to
> implement something (or a new virtual is added to the SkDevice). It might not
be
> a bad idea for us to create an SkBaseDevice and make everyone inherit from it.
> It might not quite so easy since PDF and GPU probably rely on quite a bit of
> functionality in SkDevice and it'd take time to figure out what should be
> promoted up to the base class.

+1  I'm all in favor of an abstract base class for devices.

LGTM