Use factory for creating SkPicture from a stream.

Use factory for creating SkPicture from a stream.

https://codereview.chromium.org/17113004 in Skia removes the
constructor which takes a stream, and replaces it with a factory.
Switch to using the factory.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=210047

[vmpstr, 2013-06-27 16:23:44]

https://chromiumcodereview.appspot.com/18078003/diff/2001/cc/resources/pictur...
File cc/resources/picture.cc (right):

https://chromiumcodereview.appspot.com/18078003/diff/2001/cc/resources/pictur...
cc/resources/picture.cc:99: return picture;
Can you return NULL here explicitly and declare |picture| closer to where it's
being initialized?

https://chromiumcodereview.appspot.com/18078003/diff/2001/cc/resources/pictur...
cc/resources/picture.cc:133: picture = new
Picture(SkPicture::CreateFromStream(&stream, &DecodeBitmap),
What does CreateFromStream return in case of failure? I'm a bit weary that if we
can't create the picture, then we still return a valid object (because of new
Picture). If CreateFromStream returns NULL, then we should return NULL.

[scroggo, 2013-06-27 16:45:44]

https://chromiumcodereview.appspot.com/18078003/diff/2001/cc/resources/pictur...
File cc/resources/picture.cc (right):

https://chromiumcodereview.appspot.com/18078003/diff/2001/cc/resources/pictur...
cc/resources/picture.cc:99: return picture;
On 2013/06/27 16:23:44, vmpstr wrote:
> Can you return NULL here explicitly and declare |picture| closer to where it's
> being initialized?

Done.

https://chromiumcodereview.appspot.com/18078003/diff/2001/cc/resources/pictur...
cc/resources/picture.cc:133: picture = new
Picture(SkPicture::CreateFromStream(&stream, &DecodeBitmap),
On 2013/06/27 16:23:44, vmpstr wrote:
> What does CreateFromStream return in case of failure?

NULL.

> I'm a bit weary that if we
> can't create the picture, then we still return a valid object (because of new
> Picture). If CreateFromStream returns NULL, then we should return NULL.

Done.

[vmpstr, 2013-06-27 17:22:53]

lgtm with nits. Also please make sure cc_unittests --gtest_filter="PictureTest*"
passes

https://chromiumcodereview.appspot.com/18078003/diff/6001/cc/resources/pictur...
File cc/resources/picture.cc (right):

https://chromiumcodereview.appspot.com/18078003/diff/6001/cc/resources/pictur...
cc/resources/picture.cc:97: if (!raw_value->GetAsDictionary(&value)) {
nit: you don't need braces here and below (where there's one line return)

https://chromiumcodereview.appspot.com/18078003/diff/6001/cc/resources/pictur...
cc/resources/picture.cc:133: if (NULL == skpicture) {
nit: I think we typically do the skpicture == NULL order.

[scroggo, 2013-06-27 18:28:59]

> Also please make sure cc_unittests
> --gtest_filter="PictureTest*"
> passes

Done. It does give me the message

YOU HAVE 1 DISABLED TEST

which I did not do on purpose, but it passes everything else.

https://codereview.chromium.org/18078003/diff/6001/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/18078003/diff/6001/cc/resources/picture.cc#ne...
cc/resources/picture.cc:97: if (!raw_value->GetAsDictionary(&value)) {
On 2013/06/27 17:22:53, vmpstr wrote:
> nit: you don't need braces here and below (where there's one line return)

Done.

https://codereview.chromium.org/18078003/diff/6001/cc/resources/picture.cc#ne...
cc/resources/picture.cc:133: if (NULL == skpicture) {
On 2013/06/27 17:22:53, vmpstr wrote:
> nit: I think we typically do the skpicture == NULL order.

Done.

[vmpstr, 2013-06-27 18:51:17]

On 2013/06/27 18:28:59, scroggo wrote:
> > Also please make sure cc_unittests
> > --gtest_filter="PictureTest*"
> > passes
> 
> Done. It does give me the message
> 
> YOU HAVE 1 DISABLED TEST
> 
> which I did not do on purpose, but it passes everything else.
>

That's there even without your patch, so that's fine :) Thanks.

[commit-bot: I haz the power, 2013-07-02 18:12:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scroggo@google.com/18078003/18001

[commit-bot: I haz the power, 2013-07-02 20:21:04]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-07-03 14:28:06]

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

[commit-bot: I haz the power, 2013-07-03 16:26:13]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-07-03 16:59:05]

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

[commit-bot: I haz the power, 2013-07-03 21:49:54]

Change committed as 210047