Moving RenderSurface creation outside of CalcDrawProps

cc/layers/layer.cc

Index: cc/layers/layer.cc
diff --git a/cc/layers/layer.cc b/cc/layers/layer.cc
index 08f39b9a21d6d67805fef6150237b57f3c51f810..455e1b6536582bdfed297f1b13a9479b112b5fa6 100644
--- a/cc/layers/layer.cc
+++ b/cc/layers/layer.cc
@@ -67,6 +67,7 @@ Layer::Layer()
       draw_checkerboard_for_missing_tiles_(false),
       force_render_surface_(false),
       transform_is_invertible_(true),
+      has_render_surface_(false),
       background_color_(0),
       opacity_(1.f),
       blend_mode_(SkXfermode::kSrcOver_Mode),
@@ -590,6 +591,10 @@ void Layer::SetTransformOrigin(const gfx::Point3F& transform_origin) {
   SetNeedsCommit();
 }
 
+bool Layer::HasNotAxisAlignedTransformAnimation() const {

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

nit: don't not put negatives in function names; maybe just
AnimationsPreserveAxisAlignment?

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

Done.

+  return !layer_animation_controller_->AnimationsPreserveAxisAlignment();
+}
+
 bool Layer::TransformIsAnimating() const {
   return layer_animation_controller_->IsAnimatingProperty(Animation::Transform);
 }
@@ -877,9 +882,9 @@ void Layer::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(has_render_surface_);
   if (!layer->FilterIsAnimatingOnImplOnly() && !FilterIsAnimating())
     layer->SetFilters(filters_);
   DCHECK(!(FilterIsAnimating() && layer->FilterIsAnimatingOnImplOnly()));
@@ -1080,19 +1085,27 @@ scoped_refptr<base::debug::ConvertableToTraceFormat> Layer::TakeDebugInfo() {
     return nullptr;
 }
 
+void Layer::SetHasRenderSurface(bool has_render_surface) {
+  if (has_render_surface_ == has_render_surface)
+    return;
+  has_render_surface_ = has_render_surface;
+  // We do not need SetNeedsCommit here, since this is only ever called

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

Could this be less special somehow?

It seems like you're trying to avoid a SetNeedsCommit here because
SetNeedsCommit during LayerTreeHost::UpdateLayers will still cause another
commit.  Could we pull out the CalcDrawProps / RenderSurface calculation prior
to UpdateLayers so that we could cancel commits that get issued as a part of it?
 You could also tack it on to be called at the end of LayerTreeHost::Layout()
instead of at the beginning of LayerTreeHost::UpdateLayers?  Not sure how I feel
about either of those, but they would work.

If none of that sounds palatable, could you make this function private/friended
by the one function that calls it in the right place?

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

I opted to friend the caller (LayerTreeHostCommon). The main reason for the
decision is because we would have to make sure every call of
CalculateDrawProperties is preceded by a call to CreateRenderSurfaces. This
happens a ton in tests. It would also remove what makes sense as a draw
property, from the calculation of the rest of the draw properties.

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

I kind of feel like we should separate out those steps, but maybe we can talk
about that as a follow up.

[awoloszyn, 2014/12/16 15:50:43]

Done.

+  // during a commit, from CalculateDrawProperties.
+  SetNeedsPushProperties();
+}
+
 void Layer::CreateRenderSurface() {
-  DCHECK(!draw_properties_.render_surface);
-  draw_properties_.render_surface = make_scoped_ptr(new RenderSurface(this));
-  draw_properties_.render_target = this;
+  DCHECK(!render_surface_);
+  render_surface_ = make_scoped_ptr(new RenderSurface(this));
 }
 
 void Layer::ClearRenderSurface() {
-  draw_properties_.render_surface = nullptr;
+  render_surface_ = nullptr;
 }
 
 void Layer::ClearRenderSurfaceLayerList() {
-  if (draw_properties_.render_surface)
-    draw_properties_.render_surface->layer_list().clear();
+  if (render_surface_)
+    render_surface_->layer_list().clear();
 }
 
 gfx::ScrollOffset Layer::ScrollOffsetForAnimation() const {