Allow creating a picture from skp to fail.

Allow creating a picture from skp to fail.

Replace the current constructor for creating an
SkPicturePlayback to a factory. In the factory,
check for incorrect data that would result in an invalid
playback. If the playback is invalid, return NULL, and
return NULL from SkPicture's factory as well.

Update SkTimedPicture(Playback) as well.

BUG=skia:1672
R=caryclark@google.com

Committed: https://code.google.com/p/skia/source/detail?r=11554

[scroggo, 2013-09-26 21:06:28]

https://codereview.chromium.org/24826002/diff/1/debugger/QT/SkDebuggerGUI.cpp
File debugger/QT/SkDebuggerGUI.cpp (right):

https://codereview.chromium.org/24826002/diff/1/debugger/QT/SkDebuggerGUI.cpp...
debugger/QT/SkDebuggerGUI.cpp:1: /*
Patchset 1 reverts r7844: "Remove bogus ability for creating an
SkPicturePlayback to fail." (shah 7b1af5ada71ec8ea0f13e731a6e401def48df18d) with
some slight changes to build today.

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

https://codereview.chromium.org/24826002/diff/1/src/core/SkPicturePlayback.cp...
src/core/SkPicturePlayback.cpp:527: if (NULL == fPictureRefs[i]) {
This bit is slightly different from a straight revert. The old code was calling
the SkPicture constructor with a pointer to bool for success, while a more
recent change switched to using a factory which can return NULL. So now we
compare against NULL and only need to delete up to i-1 down below.

https://codereview.chromium.org/24826002/diff/3001/src/core/SkPicturePlayback...
File src/core/SkPicturePlayback.cpp (right):

https://codereview.chromium.org/24826002/diff/3001/src/core/SkPicturePlayback...
src/core/SkPicturePlayback.cpp:492: if (stream->read(storage.get(), size) !=
size) {
Patch set 2 contains the interesting parts: return false if read failed to read
the amount requested. This means there was an error writing or reading the
stream.

[caryclark, 2013-09-26 21:12:46]

https://codereview.chromium.org/24826002/diff/3001/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.chromium.org/24826002/diff/3001/src/core/SkPicture.cpp#new...
src/core/SkPicture.cpp:313: bool isValid = false;
It looks like this argues for creating a SkPicturePlayback::Create call (or
factory or whatever it should be) instead of using the constructor. Then the
function result could be NULL and isValid wouldn't needed, and the subsequent
delete call could be eliminated as well.

[reed1, 2013-09-26 21:16:06]

https://codereview.chromium.org/24826002/diff/3001/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.chromium.org/24826002/diff/3001/src/core/SkPicture.cpp#new...
src/core/SkPicture.cpp:313: bool isValid = false;
On 2013/09/26 21:12:46, caryclark wrote:
> It looks like this argues for creating a SkPicturePlayback::Create call (or
> factory or whatever it should be) instead of using the constructor. Then the
> function result could be NULL and isValid wouldn't needed, and the subsequent
> delete call could be eliminated as well.

+1

[scroggo, 2013-09-26 22:17:46]

https://codereview.chromium.org/24826002/diff/3001/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.chromium.org/24826002/diff/3001/src/core/SkPicture.cpp#new...
src/core/SkPicture.cpp:313: bool isValid = false;
On 2013/09/26 21:16:07, reed1 wrote:
> On 2013/09/26 21:12:46, caryclark wrote:
> > It looks like this argues for creating a SkPicturePlayback::Create call (or
> > factory or whatever it should be) instead of using the constructor. Then the
> > function result could be NULL and isValid wouldn't needed, and the
subsequent
> > delete call could be eliminated as well.
> 
> +1

I think that is generally a better pattern (this change only revives the old
code which has a pointer to bool). On the other hand, that is a more invasive
change with more potential for errors. Further, SkPicturePlayback is only used
internally. I think we can put that off for a later change, and fix the crashes
that Cary is seeing in the meantime.

[caryclark, 2013-09-27 11:50:00]

On 2013/09/26 22:17:46, scroggo wrote:
> https://codereview.chromium.org/24826002/diff/3001/src/core/SkPicture.cpp
> File src/core/SkPicture.cpp (right):
> 
>
https://codereview.chromium.org/24826002/diff/3001/src/core/SkPicture.cpp#new...
> src/core/SkPicture.cpp:313: bool isValid = false;
> On 2013/09/26 21:16:07, reed1 wrote:
> > On 2013/09/26 21:12:46, caryclark wrote:
> > > It looks like this argues for creating a SkPicturePlayback::Create call
(or
> > > factory or whatever it should be) instead of using the constructor. Then
the
> > > function result could be NULL and isValid wouldn't needed, and the
> subsequent
> > > delete call could be eliminated as well.
> > 
> > +1
> 
> I think that is generally a better pattern (this change only revives the old
> code which has a pointer to bool). On the other hand, that is a more invasive
> change with more potential for errors. Further, SkPicturePlayback is only used
> internally. I think we can put that off for a later change, and fix the
crashes
> that Cary is seeing in the meantime.

I suspect that the Create-style code would be a smaller change than your
proposed change. In any case, this fix is not holding me up -- I've excluded the
erroneous tests locally.

[scroggo, 2013-09-30 21:26:37]

Latest patch replaces the constructor with a factory.

[caryclark, 2013-10-01 12:42:09]

lgtm

[scroggo, 2013-10-01 15:31:06]

Committed patchset #5 manually as r11554 (presubmit successful).