cc : Stop pushing layers from hidden subtrees at commit

cc/trees/property_tree_builder.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/trees/property_tree_builder.h"
 
 #include <stddef.h>
 
 #include <map>
 #include <set>
@@ skipping 636 matching lines @@
         NumLayerOrDescendantsThatDrawContentRecursive(child_layer);
   }
   return num_descendants_that_draw_content;
 }
 
 static inline float EffectiveOpacity(Layer* layer) {
   return layer->EffectiveOpacity();
 }
 
 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) {
   return layer->opacity();
 }
 
 static inline float Opacity(LayerImpl* layer) {
   return layer->test_properties()->opacity;
 }
 
@@ skipping 19 matching lines @@
 
 static inline const FilterOperations& BackgroundFilters(LayerImpl* layer) {
   return layer->test_properties()->background_filters;
 }
 
 static inline bool HideLayerAndSubtree(Layer* layer) {
   return layer->hide_layer_and_subtree();
 }
 
 static inline bool HideLayerAndSubtree(LayerImpl* layer) {
-  return layer->test_properties()->hide_layer_and_subtree;
+  return false;
 }
 
 static inline bool HasCopyRequest(Layer* layer) {
   return layer->HasCopyRequest();
 }
 
 static inline bool HasCopyRequest(LayerImpl* layer) {
   return !layer->test_properties()->copy_requests.empty();
 }
 
@@ skipping 119 matching lines @@
     copy_requests->push_back(std::move(request));
   layer->test_properties()->copy_requests.clear();
 }
 
 static void SetSubtreeHasCopyRequest(Layer* layer,
                                      bool subtree_has_copy_request) {
   layer->SetSubtreeHasCopyRequest(subtree_has_copy_request);
 }
 
 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>
 bool UpdateSubtreeHasCopyRequestRecursive(LayerType* layer) {
   bool subtree_has_copy_request = false;
   if (HasCopyRequest(layer))
     subtree_has_copy_request = true;
   for (size_t i = 0; i < Children(layer).size(); ++i) {
     LayerType* current_child = ChildAt(layer, i);
     subtree_has_copy_request |=
@@ skipping 272 matching lines @@
 }
 
 static void SetLayerPropertyChangedForChild(Layer* parent, Layer* child) {
   if (parent->subtree_property_changed())
     child->SetSubtreePropertyChanged();
 }
 
 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 AddPropertyTreeNodesIfNeeded(
+    LayerType* layer,
+    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);
+  bool created_render_surface =
+      AddEffectNodeIfNeeded(data_from_parent, layer, data_for_children);
+
+  bool created_transform_node = AddTransformNodeIfNeeded(
+      data_from_parent, layer, created_render_surface, data_for_children);
+  AddClipNodeIfNeeded(data_from_parent, layer, created_transform_node,
+                      data_for_children);
+
+  AddScrollNodeIfNeeded(data_from_parent, layer, data_for_children);
+
+  SetBackfaceVisibilityTransform(layer, created_transform_node);
+  SetSafeOpaqueBackgroundColor(data_from_parent, layer, data_for_children);
+}
+
 template <typename LayerType>
 void BuildPropertyTreesInternal(
     LayerType* layer,
     const DataForRecursion<LayerType>& data_from_parent) {
-  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);
+  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.

 
-  bool created_transform_node = AddTransformNodeIfNeeded(
-      data_from_parent, layer, created_render_surface, &data_for_children);
-  AddClipNodeIfNeeded(data_from_parent, layer, created_transform_node,
-                      &data_for_children);
-
-  AddScrollNodeIfNeeded(data_from_parent, layer, &data_for_children);
-
-  SetBackfaceVisibilityTransform(layer, created_transform_node);
-  SetSafeOpaqueBackgroundColor(data_from_parent, layer, &data_for_children);
+  // 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
           ? true
           : !AnimationsPreserveAxisAlignment(layer) ||
                 !Transform(layer).Preserves2dAxisAlignment();
   bool has_non_axis_aligned_clip =
       not_axis_aligned_since_last_clip && LayerClipsSubtree(layer);
   data_for_children.not_axis_aligned_since_last_clip =
       !has_non_axis_aligned_clip;
@@ skipping 13 matching lines @@
   if (ScrollChildren(layer)) {
     for (LayerType* scroll_child : *ScrollChildren(layer)) {
       DCHECK_EQ(ScrollParent(scroll_child), layer);
       DCHECK(Parent(scroll_child));
       data_for_children.effect_tree_parent =
           Parent(scroll_child)->effect_tree_index();
       BuildPropertyTreesInternal(scroll_child, data_for_children);
     }
   }
 
+  if (data_for_children.is_hidden)
+    return;
+
   if (MaskLayer(layer)) {
     MaskLayer(layer)->set_property_tree_sequence_number(
         data_from_parent.property_trees->sequence_number);
     MaskLayer(layer)->set_offset_to_transform_parent(
         layer->offset_to_transform_parent());
     MaskLayer(layer)->SetTransformTreeIndex(layer->transform_tree_index());
     MaskLayer(layer)->SetClipTreeIndex(layer->clip_tree_index());
     MaskLayer(layer)->SetEffectTreeIndex(layer->effect_tree_index());
     MaskLayer(layer)->SetScrollTreeIndex(layer->scroll_tree_index());
   }
@@ skipping 182 matching lines @@
       root_layer, page_scale_layer, inner_viewport_scroll_layer,
       outer_viewport_scroll_layer, overscroll_elasticity_layer,
       elastic_overscroll, page_scale_factor, device_scale_factor, viewport,
       device_transform, property_trees, color);
   property_trees->effect_tree.CreateOrReuseRenderSurfaces(
       &render_surfaces, root_layer->layer_tree_impl());
   property_trees->ResetCachedData();
 }
 
 }  // namespace cc