Moving RenderSurface creation outside of CalcDrawProps

cc/layers/layer_impl.cc

Index: cc/layers/layer_impl.cc
diff --git a/cc/layers/layer_impl.cc b/cc/layers/layer_impl.cc
index a99ede899fca62e89af1d22c636500cca24a081a..e1b729630fd73385d71aa9fb3948053c3b2a9986 100644
--- a/cc/layers/layer_impl.cc
+++ b/cc/layers/layer_impl.cc
@@ -60,7 +60,6 @@ LayerImpl::LayerImpl(LayerTreeImpl* tree_impl, int id)
       draw_checkerboard_for_missing_tiles_(false),
       draws_content_(false),
       hide_layer_and_subtree_(false),
-      force_render_surface_(false),
       transform_is_invertible_(true),
       is_container_for_fixed_position_layers_(false),
       background_color_(0),
@@ -204,7 +203,7 @@ void LayerImpl::SetClipChildren(std::set<LayerImpl*>* children) {
 void LayerImpl::PassCopyRequests(ScopedPtrVector<CopyOutputRequest>* requests) {
   if (requests->empty())
     return;
-
+  DCHECK(render_surface());
   bool was_empty = copy_requests_.empty();
   copy_requests_.insert_and_take(copy_requests_.end(), requests);
   requests->clear();
@@ -218,6 +217,7 @@ void LayerImpl::TakeCopyRequestsAndTransformToTarget(
     ScopedPtrVector<CopyOutputRequest>* requests) {
   DCHECK(!copy_requests_.empty());
   DCHECK(layer_tree_impl()->IsActiveTree());
+  DCHECK(render_target() == this);
 
   size_t first_inserted_request = requests->size();
   requests->insert_and_take(requests->end(), &copy_requests_);
@@ -238,20 +238,9 @@ void LayerImpl::TakeCopyRequestsAndTransformToTarget(
   layer_tree_impl()->RemoveLayerWithCopyOutputRequest(this);
 }
 
-void LayerImpl::CreateRenderSurface() {
-  DCHECK(!draw_properties_.render_surface);
-  draw_properties_.render_surface =
-      make_scoped_ptr(new RenderSurfaceImpl(this));
-  draw_properties_.render_target = this;
-}
-
-void LayerImpl::ClearRenderSurface() {
-  draw_properties_.render_surface = nullptr;
-}
-
 void LayerImpl::ClearRenderSurfaceLayerList() {
-  if (draw_properties_.render_surface)
-    draw_properties_.render_surface->layer_list().clear();
+  if (render_surface_)
+    render_surface_->layer_list().clear();
 }
 
 void LayerImpl::PopulateSharedQuadState(SharedQuadState* state) const {
@@ -519,9 +508,9 @@ void LayerImpl::PushPropertiesTo(LayerImpl* layer) {
   layer->SetDoubleSided(double_sided_);
   layer->SetDrawCheckerboardForMissingTiles(
       draw_checkerboard_for_missing_tiles_);
-  layer->SetForceRenderSurface(force_render_surface_);
   layer->SetDrawsContent(DrawsContent());
   layer->SetHideLayerAndSubtree(hide_layer_and_subtree_);
+  layer->SetHasRenderSurface(!!render_surface());
   layer->SetFilters(filters());
   layer->SetBackgroundFilters(background_filters());
   layer->SetMasksToBounds(masks_to_bounds_);
@@ -739,8 +728,8 @@ void LayerImpl::ResetAllChangeTrackingForSubtree() {
   update_rect_ = gfx::Rect();
   damage_rect_ = gfx::RectF();
 
-  if (draw_properties_.render_surface)
-    draw_properties_.render_surface->ResetPropertyChangedFlag();
+  if (render_surface_)
+    render_surface_->ResetPropertyChangedFlag();
 
   if (mask_layer_)
     mask_layer_->ResetAllChangeTrackingForSubtree();
@@ -929,6 +918,7 @@ SkColor LayerImpl::SafeOpaqueBackgroundColor() const {
 void LayerImpl::SetFilters(const FilterOperations& filters) {
   if (filters_ == filters)
     return;
+  DCHECK(filters.IsEmpty() || render_surface());

[enne (OOO), 2014/12/12 21:26:32]

I'm not sure this is the right place for this.  It seems like you're creating an
unnecessary ordering dependency here that SetHasRenderSurface is called before
SetFilters.

[awoloszyn, 2014/12/15 20:24:53]

This was done primarily for the sake of tests. We make assumptions in CDP that
you have a render surface if you have filters, which hold when running normally,
but have to be set up manually when running tests. (Since we no longer get our
render surfaces from main-thread). Removing may make tests a bit harder to
debug, when this assumption is violated.

I have removed for now.

[enne (OOO), 2014/12/15 22:46:50]

I think the check makes a lot of sense and will help catch such errors in the
future, but I think this is just in the wrong place.  If you want to have such a
debug check, maybe add a post-calcdraw iteration over layers? Or, a
pre-appendquads step?

 
   filters_ = filters;
   NoteLayerPropertyChangedForSubtree();
@@ -948,6 +938,7 @@ void LayerImpl::SetBackgroundFilters(
     const FilterOperations& filters) {
   if (background_filters_ == filters)
     return;
+  DCHECK(filters.IsEmpty() || render_surface());
 
   background_filters_ = filters;
   NoteLayerPropertyChanged();
@@ -1595,4 +1586,15 @@ void LayerImpl::NotifyAnimationFinished(
     layer_tree_impl_->InputScrollAnimationFinished();
 }
 
+void LayerImpl::SetHasRenderSurface(bool should_have_render_surface) {
+  if (!!render_surface() == should_have_render_surface)
+    return;
+
+  if (should_have_render_surface) {

[enne (OOO), 2014/12/12 21:26:32]

This function should SetNeedsPushProperties and
set_needs_update_draw_properties, right? Can you add that to the needs update
draw properties test?

[awoloszyn, 2014/12/15 20:24:53]

Done.

+    render_surface_ = make_scoped_ptr(new RenderSurfaceImpl(this));
+    return;
+  }
+  render_surface_.reset();
+}
+
 }  // namespace cc