cc : Stop pushing layers from hidden subtrees at commit

cc/trees/tree_synchronizer.cc

 // Copyright 2011 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/tree_synchronizer.h"
 
 #include <stddef.h>
 
 #include <set>
 
@@ skipping 17 matching lines @@
   std::unique_ptr<OwnedLayerImplList> old_layers(tree_impl->DetachLayers());
 
   OwnedLayerImplMap old_layer_map;
   for (auto& it : *old_layers) {
     DCHECK(it);
     old_layer_map[it->id()] = std::move(it);
   }
 
   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));
     tree_impl->AddLayer(std::move(layer_impl));
   }
 }
 
 void TreeSynchronizer::SynchronizeTrees(Layer* layer_root,
                                         LayerTreeImpl* tree_impl) {
   if (!layer_root) {
@@ skipping 38 matching lines @@
 
 static bool LayerHasValidPropertyTreeIndices(LayerImpl* layer) {
   DCHECK(layer);
   return layer->transform_tree_index() != TransformTree::kInvalidNodeId &&
          layer->effect_tree_index() != EffectTree::kInvalidNodeId &&
          layer->clip_tree_index() != ClipTree::kInvalidNodeId &&
          layer->scroll_tree_index() != ScrollTree::kInvalidNodeId;
 }
 #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();
 }
 
 template <typename LayerType>
 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);
+    }
   }
 }
 
 void TreeSynchronizer::PushLayerProperties(LayerTreeImpl* pending_tree,
                                            LayerTreeImpl* active_tree) {
   PushLayerPropertiesInternal(pending_tree->LayersThatShouldPushProperties(),
                               active_tree);
 }
 
 void TreeSynchronizer::PushLayerProperties(LayerTreeHost* host_tree,
                                            LayerTreeImpl* impl_tree) {
   PushLayerPropertiesInternal(host_tree->LayersThatShouldPushProperties(),
                               impl_tree);
 }
 
 }  // namespace cc