cc : Stop pushing layers from hidden subtrees at commit

cc/trees/property_tree_builder.cc

Index: cc/trees/property_tree_builder.cc
diff --git a/cc/trees/property_tree_builder.cc b/cc/trees/property_tree_builder.cc
index a7efa0a71556ec6e68c77605d7e907fe504f32b3..8f0468f99ab09e4097f1bb526d4d8312ad51383d 100644
--- a/cc/trees/property_tree_builder.cc
+++ b/cc/trees/property_tree_builder.cc
@@ -654,9 +654,7 @@ static inline float EffectiveOpacity(Layer* layer) {
 }
 
 static inline float EffectiveOpacity(LayerImpl* layer) {
-  return layer->test_properties()->hide_layer_and_subtree
-             ? 0.f
-             : layer->test_properties()->opacity;
+  return layer->test_properties()->opacity;
 }
 
 static inline float Opacity(Layer* layer) {
@@ -696,7 +694,7 @@ static inline bool HideLayerAndSubtree(Layer* layer) {
 }
 
 static inline bool HideLayerAndSubtree(LayerImpl* layer) {
-  return layer->test_properties()->hide_layer_and_subtree;
+  return false;
 }
 
 static inline bool HasCopyRequest(Layer* layer) {
@@ -836,16 +834,14 @@ static void SetSubtreeHasCopyRequest(Layer* layer,
 }
 
 static void SetSubtreeHasCopyRequest(LayerImpl* layer,
-                                     bool subtree_has_copy_request) {
-  layer->test_properties()->subtree_has_copy_request = subtree_has_copy_request;
-}
+                                     bool subtree_has_copy_request) {}
 
 static bool SubtreeHasCopyRequest(Layer* layer) {
   return layer->SubtreeHasCopyRequest();
 }
 
 static bool SubtreeHasCopyRequest(LayerImpl* layer) {
-  return layer->test_properties()->subtree_has_copy_request;
+  return false;
 }
 
 template <typename LayerType>
@@ -1138,28 +1134,59 @@ static void SetLayerPropertyChangedForChild(Layer* parent, Layer* child) {
 static void SetLayerPropertyChangedForChild(LayerImpl* parent,
                                             LayerImpl* child) {}
 
+static bool SubtreeIsHidden(Layer* layer) {
+  return !SubtreeHasCopyRequest(layer) && layer->parent() &&
+         layer->hide_layer_and_subtree();
+}
+
+static bool SubtreeIsHidden(LayerImpl* layer) {
+  return false;
+}
+
+static void SetIsHidden(Layer* layer, bool is_hidden) {
+  layer->SetIsHidden(is_hidden);
+  if (MaskLayer(layer))

[enne (OOO), 2017/05/29 23:14:10]

Is this required? I worry that this is one more avenue for inconstency (like
bounds), and would rather just have mask layers never have their hidden state
queried or set anywhere.

+    MaskLayer(layer)->SetIsHidden(is_hidden);
+}
+
+static void SetIsHidden(LayerImpl* layer, bool is_hidden) {}
+
 template <typename LayerType>
-void BuildPropertyTreesInternal(
+void AddPropertyTreeNodesIfNeeded(
     LayerType* layer,
-    const DataForRecursion<LayerType>& data_from_parent) {
+    const DataForRecursion<LayerType>& data_from_parent,
+    DataForRecursion<LayerType>* data_for_children) {
+  DCHECK(!data_for_children->is_hidden);
   layer->set_property_tree_sequence_number(
       data_from_parent.property_trees->sequence_number);
-
-  DataForRecursion<LayerType> data_for_children(data_from_parent);
-
   bool created_render_surface =
-      AddEffectNodeIfNeeded(data_from_parent, layer, &data_for_children);
-
+      AddEffectNodeIfNeeded(data_from_parent, layer, data_for_children);
 
   bool created_transform_node = AddTransformNodeIfNeeded(
-      data_from_parent, layer, created_render_surface, &data_for_children);
+      data_from_parent, layer, created_render_surface, data_for_children);
   AddClipNodeIfNeeded(data_from_parent, layer, created_transform_node,
-                      &data_for_children);
+                      data_for_children);
 
-  AddScrollNodeIfNeeded(data_from_parent, layer, &data_for_children);
+  AddScrollNodeIfNeeded(data_from_parent, layer, data_for_children);
 
   SetBackfaceVisibilityTransform(layer, created_transform_node);
-  SetSafeOpaqueBackgroundColor(data_from_parent, layer, &data_for_children);
+  SetSafeOpaqueBackgroundColor(data_from_parent, layer, data_for_children);
+}
+
+template <typename LayerType>
+void BuildPropertyTreesInternal(
+    LayerType* layer,
+    const DataForRecursion<LayerType>& data_from_parent) {
+  DataForRecursion<LayerType> data_for_children(data_from_parent);
+
+  data_for_children.is_hidden =
+      data_from_parent.is_hidden || SubtreeIsHidden(layer);
+
+  SetIsHidden(layer, data_for_children.is_hidden);

[enne (OOO), 2017/05/29 23:14:10]

I am not that keen on setting (more) data on layers during property tree
building.  I think I can understand building property trees setting property
tree indexes, but this is like extra calculated data that gets set.  It just
makes the data flow (and understanding which data is read and which is write on
layers) harder to understand.

For instance, with this patch, somebody could call Layer::SetIsHidden(true) and
then this function could come through and clobber it to be SetIsHidden(false),
which would be quite confusing.

I think maybe having an internal SetIsInHiddenSubtree function (named
differently than IsHidden) that was protected/friended could help this. 
Alternatively, maybe the callers in ui could recursively call SetIsHidden and we
could drop the SubtreeIsHidden API, although that's a larger change.

+
+  // We do not create property tree nodes for hidden layers.
+  if (!data_for_children.is_hidden)
+    AddPropertyTreeNodesIfNeeded(layer, data_from_parent, &data_for_children);
 
   bool not_axis_aligned_since_last_clip =
       data_from_parent.not_axis_aligned_since_last_clip
@@ -1193,6 +1220,9 @@ void BuildPropertyTreesInternal(
     }
   }
 
+  if (data_for_children.is_hidden)
+    return;
+
   if (MaskLayer(layer)) {
     MaskLayer(layer)->set_property_tree_sequence_number(
         data_from_parent.property_trees->sequence_number);