Replace SkPicture(SkStream) constructors with a factory.

Replace SkPicture(SkStream) constructors with a factory.

SkPicture:
Remove the constructors which take an SkStream as an argument. Rather
than having to check a variable for success, the factory will return
NULL on failure.
Add a protected function for determining if an SkStream is an SKP
to share code with SkTimedPicture.
In the factory, check for a NULL SkStream.
Use a default decoder (from BUG:
https://code.google.com/p/skia/issues/detail?id=1325)

SkDebuggerGUI:
Call SkPicture::CreateFromStream when necessary.
Write a factory for creating SkTimedPictures and use it.

Use the factory throughout tools.

Add include/lazy to utils and effects gyp include_dirs so SkPicture.h
can reference SkImageDecoder.h which references SkBitmapFactory.h (in
include/lazy).

Changes code Chromium uses, so this will require a temporary Skia
and then a change to Chromium to use the new Skia code.

TODO: Create a decoder that does nothing to be used by pinspect,
lua pictures, etc, and allow it to not assert in SkOrderedReadBuffer.

R=reed@google.com

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

[scroggo, 2013-06-24 16:58:46]

How does this API change sound? If we like it, I'll write up another change to
preserve the current behavior to keep chrome building (that change will then be
reverted once chrome switches to the new behavior).

[robertphillips, 2013-06-24 17:22:09]

I like it.

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

https://codereview.chromium.org/17113004/diff/8001/debugger/QT/SkDebuggerGUI....
debugger/QT/SkDebuggerGUI.cpp:250: 
Does it make sense for there to be a "(playback, width, height)" flavor of
SkTimedPicture ctor?

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

https://codereview.chromium.org/17113004/diff/8001/src/core/SkPicture.cpp#new...
src/core/SkPicture.cpp:288: 
Again, could we use SkNEW_ARGS here?

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

https://codereview.chromium.org/17113004/diff/8001/src/core/SkPicturePlayback...
src/core/SkPicturePlayback.cpp:524: fPictureRefs[i] =
SkPicture::CreateFromStream(stream, proc);
I don't think this comment is true.

https://codereview.chromium.org/17113004/diff/8001/src/core/SkPicturePlayback...
src/core/SkPicturePlayback.cpp:532: // Delete the array
Don't we need to also loop backwards across the array freeing the SkPictures
that were allocated earlier?

[scroggo, 2013-06-24 18:22:19]

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

https://codereview.chromium.org/17113004/diff/8001/debugger/QT/SkDebuggerGUI....
debugger/QT/SkDebuggerGUI.cpp:250: 
On 2013/06/24 17:22:09, robertphillips wrote:
> Does it make sense for there to be a "(playback, width, height)" flavor of
> SkTimedPicture ctor?

Absolutely! Done.

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

https://codereview.chromium.org/17113004/diff/8001/src/core/SkPicture.cpp#new...
src/core/SkPicture.cpp:288: 
On 2013/06/24 17:22:09, robertphillips wrote:
> Again, could we use SkNEW_ARGS here?

Done.

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

https://codereview.chromium.org/17113004/diff/8001/src/core/SkPicturePlayback...
src/core/SkPicturePlayback.cpp:524: fPictureRefs[i] =
SkPicture::CreateFromStream(stream, proc);
On 2013/06/24 17:22:09, robertphillips wrote:
> I don't think this comment is true.

How else can it fail?

https://codereview.chromium.org/17113004/diff/8001/src/core/SkPicturePlayback...
src/core/SkPicturePlayback.cpp:532: // Delete the array
On 2013/06/24 17:22:09, robertphillips wrote:
> Don't we need to also loop backwards across the array freeing the SkPictures
> that were allocated earlier?

Yes. Done.

[reed1, 2013-06-24 22:24:14]

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

https://codereview.chromium.org/17113004/diff/16001/src/core/SkPicture.cpp#ne...
src/core/SkPicture.cpp:268: if (NULL == stream || NULL == info) {
Why do we return false on info==null?

1. crashing is one option
2. just retuning a bool is another

Since we can return false *after* reading info, perhaps it would be better to
always perform that on a stack object, and then only if we're going to return
true do we (after checking for null) copy that into the caller's info.

https://codereview.chromium.org/17113004/diff/16001/src/core/SkPicturePlaybac...
File src/core/SkPicturePlayback.cpp (right):

https://codereview.chromium.org/17113004/diff/16001/src/core/SkPicturePlaybac...
src/core/SkPicturePlayback.cpp:531: if (NULL == fPictureRefs[i]) {
so we want this, or is it better to crash? Do we check anywhere else for read
failures, and try to recover?

[scroggo, 2013-06-25 15:36:50]

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

https://codereview.chromium.org/17113004/diff/16001/src/core/SkPicture.cpp#ne...
src/core/SkPicture.cpp:268: if (NULL == stream || NULL == info) {
On 2013/06/24 22:24:14, reed1 wrote:
> Why do we return false on info==null?
> 
> 1. crashing is one option
> 2. just retuning a bool is another
> 
> Since we can return false *after* reading info, perhaps it would be better to
> always perform that on a stack object, and then only if we're going to return
> true do we (after checking for null) copy that into the caller's info.

Done.

https://codereview.chromium.org/17113004/diff/16001/src/core/SkPicturePlaybac...
File src/core/SkPicturePlayback.cpp (right):

https://codereview.chromium.org/17113004/diff/16001/src/core/SkPicturePlaybac...
src/core/SkPicturePlayback.cpp:531: if (NULL == fPictureRefs[i]) {
On 2013/06/24 22:24:14, reed1 wrote:
> so we want this, or is it better to crash? Do we check anywhere else for read
> failures, and try to recover?

We do not check for read failures in other places. Removed.

[reed1, 2013-06-25 15:40:10]

lgtm w/ comment-request

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

https://codereview.chromium.org/17113004/diff/26001/src/core/SkPicture.cpp#ne...
src/core/SkPicture.cpp:279: if (!stream->readBool()) {
// What is the bool we are checking?

[scroggo, 2013-06-25 16:54:34]

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

https://codereview.chromium.org/17113004/diff/26001/src/core/SkPicture.cpp#ne...
src/core/SkPicture.cpp:279: if (!stream->readBool()) {
On 2013/06/25 15:40:10, reed1 wrote:
> // What is the bool we are checking?

In SkPicture::serialize, we record true if there was a playback to write to the
stream (whether it was created by the call to serialize or it was on the picture
to begin with). If not, we record false.

I started to say that false meant there was nothing to draw, but you could have
a picture with a record or playback which is empty.

This actually changes the behavior in an unintentional way though. The intended
change in behavior is:

remove the pointer to success, and return NULL instead of an empty picture.

In addition, it includes the following change to behavior:

if there was no playback, return NULL (analogous to setting success to false in
the former case)

The latest patch retains the old behavior: i.e. a picture without a playback
will deserialize into an empty picture. That may be a good thing to reconsider,
though. Is there a benefit to deserializing an empty picture?

[scroggo, 2013-06-28 21:32:06]

Committed patchset #7 manually as r9822 (presubmit successful).