Use factory for creating SkPicture from a stream.

cc/resources/picture.cc

Index: cc/resources/picture.cc
diff --git a/cc/resources/picture.cc b/cc/resources/picture.cc
index 4c85e7d2092def7d2ce79297abeb3d57fb7b79cd..c63d3b34bb2da37d796e7f1867bdcebcb34d175a 100644
--- a/cc/resources/picture.cc
+++ b/cc/resources/picture.cc
@@ -86,61 +86,61 @@ scoped_refptr<Picture> Picture::Create(gfx::Rect layer_rect) {
   return make_scoped_refptr(new Picture(layer_rect));
 }
 
-scoped_refptr<Picture> Picture::CreateFromValue(const base::Value* value) {
-  bool success;
-  scoped_refptr<Picture> picture =
-    make_scoped_refptr(new Picture(value, &success));
-  if (!success)
-    picture = NULL;
-  return picture;
-}
-
 Picture::Picture(gfx::Rect layer_rect)
     : layer_rect_(layer_rect) {
   // Instead of recording a trace event for object creation here, we wait for
   // the picture to be recorded in Picture::Record.
 }
 
-Picture::Picture(const base::Value* raw_value, bool* success) {
+scoped_refptr<Picture> Picture::CreateFromValue(const base::Value* raw_value) {
+  scoped_refptr<Picture> picture(NULL);
   const base::DictionaryValue* value = NULL;
   if (!raw_value->GetAsDictionary(&value)) {
-    *success = false;
-    return;
+    return picture;

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

Can you return NULL here explicitly and declare |picture| closer to where it's
being initialized?

[scroggo, 2013/06/27 16:45:45]

Done.

   }
 
   // Decode the picture from base64.
   std::string encoded;
   if (!value->GetString("skp64", &encoded)) {
-    *success = false;
-    return;
+    return picture;
   }
 
   std::string decoded;
   base::Base64Decode(encoded, &decoded);
   SkMemoryStream stream(decoded.data(), decoded.size());
 
-  const base::Value* layer_rect = NULL;
-  if (!value->Get("params.layer_rect", &layer_rect)) {
-    *success = false;
-    return;
+  const base::Value* layer_rect_value = NULL;
+  if (!value->Get("params.layer_rect", &layer_rect_value)) {
+    return picture;
   }
-  if (!MathUtil::FromValue(layer_rect, &layer_rect_)) {
-    *success = false;
-    return;
+
+  gfx::Rect layer_rect;
+  if (!MathUtil::FromValue(layer_rect_value, &layer_rect)) {
+    return picture;
   }
 
-  const base::Value* opaque_rect = NULL;
-  if (!value->Get("params.opaque_rect", &opaque_rect)) {
-    *success = false;
-    return;
+  const base::Value* opaque_rect_value = NULL;
+  if (!value->Get("params.opaque_rect", &opaque_rect_value)) {
+    return picture;
   }
-  if (!MathUtil::FromValue(opaque_rect, &opaque_rect_)) {
-    *success = false;
-    return;
+
+  gfx::Rect opaque_rect;
+  if (!MathUtil::FromValue(opaque_rect_value, &opaque_rect)) {
+    return picture;
   }
 
   // Read the picture. This creates an empty picture on failure.
-  picture_ = skia::AdoptRef(new SkPicture(&stream, success, &DecodeBitmap));
+  picture = new Picture(SkPicture::CreateFromStream(&stream, &DecodeBitmap),

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

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:45]

NULL.
Done.

+                        layer_rect, opaque_rect);
+  return picture;
+}
+
+Picture::Picture(SkPicture* picture,
+                 gfx::Rect layer_rect,
+                 gfx::Rect opaque_rect) :
+    layer_rect_(layer_rect),
+    opaque_rect_(opaque_rect),
+    picture_(skia::AdoptRef(picture)) {
 }
 
 Picture::Picture(const skia::RefPtr<SkPicture>& picture,