cc : Stop pushing layers from hidden subtrees at commit

cc/trees/tree_synchronizer.cc

Index: cc/trees/tree_synchronizer.cc
diff --git a/cc/trees/tree_synchronizer.cc b/cc/trees/tree_synchronizer.cc
index 87efee06cab3859b3382d1fbd7e7c8d6e07037b5..ede919e155a7f031ae00ecb6e10818e7019f0da2 100644
--- a/cc/trees/tree_synchronizer.cc
+++ b/cc/trees/tree_synchronizer.cc
@@ -35,6 +35,13 @@ void SynchronizeTreesInternal(LayerTreeType* source_tree,
 
   PushLayerList(&old_layer_map, source_tree, tree_impl);
 
+  // We also push hidden surface layer ids as they maybe needed to handle copy
+  // requests on surfaces.
+  tree_impl->ClearHiddenSurfaceLayerIds();

[enne (OOO), 2017/05/19 22:27:05]

This seems like something that should happen as a part of layer tree push
properties to and not inside of tree synchronizer?

[jaydasika, 2017/05/23 00:25:14]

Done.

+  tree_impl->SetHiddenSurfaceLayerIds(source_tree->HiddenSurfaceLayerIds());
+  // Reset for next update.
+  source_tree->ClearHiddenSurfaceLayerIds();
+
   for (int id : property_trees->effect_tree.mask_layer_ids()) {
     std::unique_ptr<LayerImpl> layer_impl(ReuseOrCreateLayerImpl(
         &old_layer_map, source_tree->LayerById(id), tree_impl));
@@ -93,26 +100,35 @@ static bool LayerHasValidPropertyTreeIndices(LayerImpl* layer) {
 }
 #endif
 
+static bool IsHidden(Layer* layer) {
+  return layer->is_hidden();
+}
+
+static bool IsHidden(LayerImpl* layer) {
+  return false;
+}
+
 template <typename LayerTreeType>
 void PushLayerList(OwnedLayerImplMap* old_layers,
                    LayerTreeType* host,
                    LayerTreeImpl* tree_impl) {
   tree_impl->ClearLayerList();
   for (auto* layer : *host) {
-    std::unique_ptr<LayerImpl> layer_impl(
-        ReuseOrCreateLayerImpl(old_layers, layer, tree_impl));
+    if (!IsHidden(layer)) {

[enne (OOO), 2017/05/19 22:27:05]

I'm not sure if this needs to be done in this patch, but I think that
https://cs.chromium.org/chromium/src/cc/trees/draw_property_utils.cc?l=808 and
this likely need to converge at some point.  It seems like anything that we're
skipping for property tree updates probably also needs to not be pushed.

(Similarly, do we need to push non-drawing layers at this point outside of
tests?)

I think this is fine for this patch, but could you leave a TODO for me to skip
more layers here?

[jaydasika, 2017/05/23 00:25:14]

Acknowledged.

+      std::unique_ptr<LayerImpl> layer_impl(
+          ReuseOrCreateLayerImpl(old_layers, layer, tree_impl));
 
 #if DCHECK_IS_ON()
-    // Every layer should have valid property tree indices
-    AssertValidPropertyTreeIndices(layer);
-    // Every layer_impl should either have valid property tree indices already
-    // or the corresponding layer should push them onto layer_impl.
-    DCHECK(LayerHasValidPropertyTreeIndices(layer_impl.get()) ||
-           host->LayerNeedsPushPropertiesForTesting(layer));
+      // Every layer should have valid property tree indices
+      AssertValidPropertyTreeIndices(layer);
+      // Every layer_impl should either have valid property tree indices already
+      // or the corresponding layer should push them onto layer_impl.
+      DCHECK(LayerHasValidPropertyTreeIndices(layer_impl.get()) ||
+             host->LayerNeedsPushPropertiesForTesting(layer));
 #endif
-
-    tree_impl->AddToLayerList(layer_impl.get());
-    tree_impl->AddLayer(std::move(layer_impl));
+      tree_impl->AddToLayerList(layer_impl.get());
+      tree_impl->AddLayer(std::move(layer_impl));
+    }
   }
   tree_impl->OnCanDrawStateChangedForTree();
 }
@@ -122,9 +138,11 @@ static void PushLayerPropertiesInternal(
     std::unordered_set<LayerType*> layers_that_should_push_properties,
     LayerTreeImpl* impl_tree) {
   for (auto layer : layers_that_should_push_properties) {
-    LayerImpl* layer_impl = impl_tree->LayerById(layer->id());
-    DCHECK(layer_impl);
-    layer->PushPropertiesTo(layer_impl);
+    if (!IsHidden(layer)) {

[enne (OOO), 2017/05/19 22:27:05]

Maybe setting IsHidden should just SetNeedsCommit instead of
SetNeedsPushProperties? It doesn't seem like the layer should have anything to
push here.

[jaydasika, 2017/05/23 00:25:14]

I will remove SetNeedsPushProperties from Layer::SetIsHidden. But, I think this
is still needed. If some layer property changes on a hidden layer, it will end
up in the push_properties layer list

+      LayerImpl* layer_impl = impl_tree->LayerById(layer->id());
+      DCHECK(layer_impl);
+      layer->PushPropertiesTo(layer_impl);
+    }
   }
 }