Switching Chromium to use new Skia SkPictureRecorder API

cc/resources/picture.cc

Index: cc/resources/picture.cc
diff --git a/cc/resources/picture.cc b/cc/resources/picture.cc
index 93b59aea7435532456ab03b9248113b8e5c8d46c..03ddec0226595e9814dbfa6379c64f9bd1f68423 100644
--- a/cc/resources/picture.cc
+++ b/cc/resources/picture.cc
@@ -249,17 +249,21 @@ void Picture::Record(ContentLayerClient* painter,
 
   DCHECK(!picture_);
   DCHECK(!tile_grid_info.fTileInterval.isEmpty());
-  picture_ = skia::AdoptRef(new SkTileGridPicture(
-      layer_rect_.width(), layer_rect_.height(), tile_grid_info));
+
+  skia::RefPtr<SkPictureFactory> factory =
+      skia::AdoptRef(new SkTileGridPictureFactory(tile_grid_info));
+  SkPictureRecorder recorder(factory.get());
 
   skia::RefPtr<SkCanvas> canvas;
+  canvas = skia::SharePtr(recorder.beginRecording(

[enne (OOO), 2014/04/15 01:10:39]

Should all the beginRecording calls be wrapped in RefPtr+SharePtr (or none of
them)?

[robertphillips, 2014/04/15 15:37:04]

As is, I think the behavior is correct here but mainly b.c. of the need to free
the NullCanvas. If not for that requirement, I think the canvas returned from
beginRecording shouldn't be reffed here at all. Note that there is some
discussion about make SkCanvas non-ref-counted which would resolve this issue.

Perhaps I misunderstood your question though?

[enne (OOO), 2014/04/15 16:45:16]

Gotcha.  The ref here is just RAII.  Makes sense.

+      layer_rect_.width(),
+      layer_rect_.height(),
+      SkPicture::kUsePathBoundsForClip_RecordingFlag |
+          SkPicture::kOptimizeForClippedPlayback_RecordingFlag));
+
   switch (recording_mode) {
     case RECORD_NORMALLY:
-      canvas = skia::SharePtr(picture_->beginRecording(
-          layer_rect_.width(),
-          layer_rect_.height(),
-          SkPicture::kUsePathBoundsForClip_RecordingFlag |
-              SkPicture::kOptimizeForClippedPlayback_RecordingFlag));
+      // Already setup for normal recording

[enne (OOO), 2014/04/15 01:10:39]

Why did the code move out of this switch?

[robertphillips, 2014/04/15 15:37:04]

The old code always generated an SkPicture (albeit empty in 2 of the cases). To
always get an SkPicture with the new API one needs to always call beginRecording
and endRecording.

[enne (OOO), 2014/04/15 16:45:16]

I guess I'm asking because in all other cases, the canvas gets clobbered.  So,
RECORD_WITH_PAINTING_DISABLED doesn't need that code above (because it deletes
that canvas) and RECORD_WITH_SK_NULL_CANVAS doesn't either because it creates a
new canvas (and also gets an endRecording call but no beginRecording? Is that
legit?).

[robertphillips, 2014/04/15 19:17:32]

The RECORD_WITH_PAINTING_DISABLED and RECORD_WITH_SK_NULL_CANVAS cases are just
playing a game with where the drawing calls get directed. In all three cases we
want an SkPicture at the end so we need to call (in the new system)
beginRecording and endRecording. The switching of the canvas just controls
whether the drawing calls actually end up in the picture, pass through a Null
Canvas or are completely ignored.

[enne (OOO), 2014/04/15 19:43:27]

I see.  So even though you delete the canvas that comes back from
beginRecording, you still have to make that call in all cases.  Thanks for all
the explanation.  :)

       break;
     case RECORD_WITH_SK_NULL_CANVAS:
       canvas = skia::AdoptRef(SkCreateNullCanvas());
@@ -267,6 +271,7 @@ void Picture::Record(ContentLayerClient* painter,
     case RECORD_WITH_PAINTING_DISABLED:
       // Blink's GraphicsContext will disable painting when given a NULL
       // canvas.
+      canvas.clear();
       break;
     default:
       NOTREACHED();
@@ -289,8 +294,7 @@ void Picture::Record(ContentLayerClient* painter,
 
   if (canvas)
     canvas->restore();
-  if (picture_->getRecordingCanvas())
-    picture_->endRecording();
+  picture_ = skia::AdoptRef(recorder.endRecording());

[enne (OOO), 2014/04/15 01:10:39]

Previously, picture_ was always non-null if the cc::Picture had been recorded. 
Are there any other NULL checks that need to be made elsewhere if this is now
optional?

[robertphillips, 2014/04/15 15:37:04]

picture_ should still always be non-null after a call to Picture::Record.

[enne (OOO), 2014/04/15 16:45:16]

Oh, ok! I misunderstood the change description here and thought that maybe an
invalid picture meant this could return NULL.  (Are there any cases it can
return NULL? Should this DCHECK?)

[robertphillips, 2014/04/15 19:17:32]

I have added a DCHECK.

 
   opaque_rect_ = gfx::ToEnclosedRect(opaque_layer_rect);