Fixes for SkPictureShader.

Fixes for SkPictureShader.

Update comment in header to make it more clear that the picture
should be unaltered after creating the shader. We want our shaders
to be immutable, and this supports that.

Make the factory return NULL if the shader would have never drawn
anyway i.e. for a null picture or picture with no width/height.

Addresses comments I brought up in
https://codereview.chromium.org/221923007/#msg16.

BUG=skia:1976
Committed: http://code.google.com/p/skia/source/detail?r=14288

[scroggo, 2014-04-15 19:15:48]

https://codereview.chromium.org/238253005/diff/1/include/core/SkShader.h
File include/core/SkShader.h (right):

https://codereview.chromium.org/238253005/diff/1/include/core/SkShader.h#newc...
include/core/SkShader.h:381: *              FIXME: It may make more sense to
take ownership than to add a
From a code search of Chromium, it looks like they're not using this yet, but
perhaps code search is out of date. If Chromium is not using this, I say let's
go ahead and change the model from ref to takes ownership.

(OTOH, if we change to the read only playback portion of an SkPicture, it's fine
to ref.)

https://codereview.chromium.org/238253005/diff/1/src/core/SkPictureShader.cpp
File src/core/SkPictureShader.cpp (right):

https://codereview.chromium.org/238253005/diff/1/src/core/SkPictureShader.cpp...
src/core/SkPictureShader.cpp:33: // For backwards compatibility, create a dummy
picture. Older versions allowed
This code is only for handling already existing SKPs. If chromium is not yet
using this class, this code can be removed.

[robertphillips, 2014-04-15 19:26:44]

https://codereview.chromium.org/238253005/diff/1/include/core/SkShader.h
File include/core/SkShader.h (right):

https://codereview.chromium.org/238253005/diff/1/include/core/SkShader.h#newc...
include/core/SkShader.h:381: *              FIXME: It may make more sense to
take ownership than to add a
SkPictures that are functionally immutable should land this week. At that point
this entry point should be able to just take a ref on a const SkPicture.

[reed1, 2014-04-15 19:40:29]

api lgtm

[f(malita), 2014-04-15 19:40:40]

lgtm

https://codereview.chromium.org/238253005/diff/1/include/core/SkShader.h
File include/core/SkShader.h (right):

https://codereview.chromium.org/238253005/diff/1/include/core/SkShader.h#newc...
include/core/SkShader.h:381: *              FIXME: It may make more sense to
take ownership than to add a
On 2014/04/15 19:15:49, scroggo wrote:
> From a code search of Chromium, it looks like they're not using this yet, but
> perhaps code search is out of date. If Chromium is not using this, I say let's
> go ahead and change the model from ref to takes ownership.
> 
> (OTOH, if we change to the read only playback portion of an SkPicture, it's
fine
> to ref.)

Chromium is not using this yet, so it would be fine to switch. But with const
pictures around the corner, I'd prefer to move to that model instead.

https://codereview.chromium.org/238253005/diff/1/src/core/SkPictureShader.cpp
File src/core/SkPictureShader.cpp (right):

https://codereview.chromium.org/238253005/diff/1/src/core/SkPictureShader.cpp...
src/core/SkPictureShader.cpp:33: // For backwards compatibility, create a dummy
picture. Older versions allowed
On 2014/04/15 19:15:49, scroggo wrote:
> This code is only for handling already existing SKPs. If chromium is not yet
> using this class, this code can be removed.

Yes, should be safe to remove.

https://codereview.chromium.org/238253005/diff/1/src/core/SkPictureShader.cpp...
src/core/SkPictureShader.cpp:69: // greater than zero.
It's safe to disregard old SKPs - the shader is not used at this point.

[scroggo, 2014-04-15 20:33:19]

https://codereview.chromium.org/238253005/diff/1/include/core/SkShader.h
File include/core/SkShader.h (right):

https://codereview.chromium.org/238253005/diff/1/include/core/SkShader.h#newc...
include/core/SkShader.h:381: *              FIXME: It may make more sense to
take ownership than to add a
On 2014/04/15 19:40:40, Florin Malita wrote:
> On 2014/04/15 19:15:49, scroggo wrote:
> > From a code search of Chromium, it looks like they're not using this yet,
but
> > perhaps code search is out of date. If Chromium is not using this, I say
let's
> > go ahead and change the model from ref to takes ownership.
> > 
> > (OTOH, if we change to the read only playback portion of an SkPicture, it's
> fine
> > to ref.)
> 
> Chromium is not using this yet, so it would be fine to switch. But with const
> pictures around the corner, I'd prefer to move to that model instead.

Sounds good. I've just left it a ref, and made a note that it cannot be const
due to internals. IIUC Robert's comment, SkPicture will soon be the const
SkPicture, so I'll leave the comment out about the future playback class.

https://codereview.chromium.org/238253005/diff/1/src/core/SkPictureShader.cpp
File src/core/SkPictureShader.cpp (right):

https://codereview.chromium.org/238253005/diff/1/src/core/SkPictureShader.cpp...
src/core/SkPictureShader.cpp:33: // For backwards compatibility, create a dummy
picture. Older versions allowed
On 2014/04/15 19:40:40, Florin Malita wrote:
> On 2014/04/15 19:15:49, scroggo wrote:
> > This code is only for handling already existing SKPs. If chromium is not yet
> > using this class, this code can be removed.
> 
> Yes, should be safe to remove.

Done.

https://codereview.chromium.org/238253005/diff/1/src/core/SkPictureShader.cpp...
src/core/SkPictureShader.cpp:69: // greater than zero.
On 2014/04/15 19:40:40, Florin Malita wrote:
> It's safe to disregard old SKPs - the shader is not used at this point.

Done.

[scroggo, 2014-04-21 18:55:05]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2014-04-21 18:55:11]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/scroggo@google.com/238253005/40001

[commit-bot: I haz the power, 2014-04-21 19:33:18]

Change committed as 14288