Serialize PictureLayer properties.

cc/layers/picture_layer.cc

Index: cc/layers/picture_layer.cc
diff --git a/cc/layers/picture_layer.cc b/cc/layers/picture_layer.cc
index 811e8068634f7db9b036886c133c1e5ebc9a6c7c..485d15ff35f90abec9eb49e505fb6c2b9d851f41 100644
--- a/cc/layers/picture_layer.cc
+++ b/cc/layers/picture_layer.cc
@@ -8,6 +8,9 @@
 #include "cc/layers/content_layer_client.h"
 #include "cc/layers/picture_layer_impl.h"
 #include "cc/playback/display_list_recording_source.h"
+#include "cc/proto/cc_conversions.h"
+#include "cc/proto/gfx_conversions.h"
+#include "cc/proto/layer.pb.h"
 #include "cc/trees/layer_tree_host.h"
 #include "cc/trees/layer_tree_impl.h"
 #include "third_party/skia/include/core/SkPictureRecorder.h"
@@ -50,28 +53,9 @@ void PictureLayer::PushPropertiesTo(LayerImpl* base_layer) {
   PictureLayerImpl* layer_impl = static_cast<PictureLayerImpl*>(base_layer);
   // TODO(danakj): Make is_mask_ a constructor parameter for PictureLayer.
   DCHECK_EQ(layer_impl->is_mask(), is_mask_);
-
-  int source_frame_number = layer_tree_host()->source_frame_number();
-  gfx::Size impl_bounds = layer_impl->bounds();

[David Trainor- moved to gerrit, 2015/12/15 17:54:30]

So, it looks like LayerImpl::bounds() is set a few lines above at
Layer::PushPropertiesTo().  So I think if I can access the right bounds
(Layer::bounds() vs Layer::paint_properties().bounds) we should be okay just
using those instead of LayerImpl.

My only concern is that LayerImpl applies a bounds_delta to the bounds it
returns, so this change doesn't result in the same behavior.  Do we need to
worry about that delta here?  If we do, I could account for it on the
PushPropertiesTo method and just use the correct Layer bounds on the
serialization method.

-  gfx::Size recording_source_bounds = recording_source_->GetSize();
-
-  // If update called, then recording source size must match bounds pushed to
-  // impl layer.
-  DCHECK(update_source_frame_number_ != source_frame_number ||
-         impl_bounds == recording_source_bounds)
-      << " bounds " << impl_bounds.ToString() << " recording source "
-      << recording_source_bounds.ToString();
-
-  if (update_source_frame_number_ != source_frame_number &&
-      recording_source_bounds != impl_bounds) {
-    // Update may not get called for the layer (if it's not in the viewport
-    // for example, even though it has resized making the recording source no
-    // longer valid. In this case just destroy the recording source.
-    recording_source_->SetEmptyBounds();
-  }
-
   layer_impl->SetNearestNeighbor(nearest_neighbor_);
 
+  ValidateRecordingSource();
   // Preserve lcd text settings from the current raster source.
   bool can_use_lcd_text = layer_impl->RasterSourceUsesLCDText();
   scoped_refptr<DisplayListRasterSource> raster_source =
@@ -198,8 +182,66 @@ bool PictureLayer::HasDrawableContent() const {
   return client_ && Layer::HasDrawableContent();
 }
 
+void PictureLayer::LayerSpecificPropertiesToProto(
+    proto::LayerProperties* proto) {
+  Layer::LayerSpecificPropertiesToProto(proto);
+  ValidateRecordingSource();
+
+  proto::PictureLayerProperties* picture = proto->mutable_picture();
+  recording_source_->ToProtobuf(picture->mutable_recording_source());
+  RegionToProto(*invalidation_.region(), picture->mutable_invalidation());
+  RectToProto(last_updated_visible_layer_rect_,
+              picture->mutable_last_updated_visible_layer_rect());
+  picture->set_is_mask(is_mask_);
+  picture->set_nearest_neighbor(nearest_neighbor_);
+
+  picture->set_update_source_frame_number(update_source_frame_number_);
+
+  invalidation_.Clear();
+}
+
+void PictureLayer::FromLayerSpecificPropertiesProto(
+    const proto::LayerProperties& proto) {
+  Layer::FromLayerSpecificPropertiesProto(proto);
+  const proto::PictureLayerProperties& picture = proto.picture();
+  recording_source_->FromProtobuf(picture.recording_source());
+
+  Region new_invalidation = RegionFromProto(picture.invalidation());
+  invalidation_.Swap(&new_invalidation);
+  last_updated_visible_layer_rect_ =
+      ProtoToRect(picture.last_updated_visible_layer_rect());
+  is_mask_ = picture.is_mask();
+  nearest_neighbor_ = picture.nearest_neighbor();
+
+  update_source_frame_number_ = picture.update_source_frame_number();
+}
+
 void PictureLayer::RunMicroBenchmark(MicroBenchmark* benchmark) {
   benchmark->RunOnLayer(this);
 }
 
+void PictureLayer::ValidateRecordingSource() {
+  int source_frame_number = layer_tree_host()->source_frame_number();
+  gfx::Size recording_source_bounds = recording_source_->GetSize();
+
+  gfx::Size layer_bounds = bounds();
+  if (paint_properties().source_frame_number == source_frame_number)
+    layer_bounds = paint_properties().bounds;
+
+  // If update called, then recording source size must match bounds pushed to
+  // impl layer.
+  DCHECK(update_source_frame_number_ != source_frame_number ||
+         layer_bounds == recording_source_bounds)
+      << " bounds " << layer_bounds.ToString() << " recording source "
+      << recording_source_bounds.ToString();
+
+  if (update_source_frame_number_ != source_frame_number &&

[vmpstr, 2015/12/16 04:21:08]

This if block should not be in this function. Validate sounds very much line
just a dcheck function, not something that can mutate the class.

Also, when you move this out, you can wrap the insides of this function in #if
DCHECK_IS_ON(), kinda like:

void PictureLayer::ValidateRecordingSource() {
#if DCHECK_IS_ON()
  ...
#endif  // DCHECK_IS_ON()
}

[David Trainor- moved to gerrit, 2015/12/16 06:51:52]

I'll have to rename the function then.  We need this behavior in two places,
which is why I pulled it out from PushPropertiesTo (where the logic used to be).
 It's basically checking whether or not the recording source is valid and, if
not, destroying it before the commit.

[vmpstr, 2015/12/16 18:55:39]

Renaming the function is fine as well. You can also make this two functions: one
that verifies the thing and one that changes the recording_source. My main
concern is that Verify* type of functions should be const.

[David Trainor- moved to gerrit, 2015/12/16 19:10:58]

Changed it to DropRecordingSourceContentIfInvalid().  Is that more along the
lines of what you want?

+      recording_source_bounds != layer_bounds) {
+    // Update may not get called for the layer (if it's not in the viewport
+    // for example), even though it has resized making the recording source no
+    // longer valid. In this case just destroy the recording source.
+    recording_source_->SetEmptyBounds();
+  }
+}
+
 }  // namespace cc