Revise LayerTreeHostImpl to use new element-id-to-node-index maps.

cc/trees/layer_tree_host_impl.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/layer_tree_host_impl.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
@@ skipping 3876 matching lines @@
 
 void LayerTreeHostImpl::SetMutatorsNeedRebuildPropertyTrees() {}
 
 void LayerTreeHostImpl::SetTreeLayerFilterMutated(
     ElementId element_id,
     LayerTreeImpl* tree,
     const FilterOperations& filters) {
   if (!tree)
     return;
 
-  const int layer_id = tree->LayerIdByElementId(element_id);
-  DCHECK(tree->property_trees()->IsInIdToIndexMap(
-      PropertyTrees::TreeType::EFFECT, layer_id));
-  const int effect_id =
-      tree->property_trees()->effect_id_to_index_map[layer_id];
-  if (effect_id != EffectTree::kInvalidNodeId)
-    tree->property_trees()->effect_tree.OnFilterAnimated(filters, effect_id,
-                                                         tree);
+  const int effect_node_index =

[wkorman, 2017/01/09 23:46:16]

All unit and layout tests seem to pass (locally, CQ in flight) with this change,
so I'm not
sure whether we still need a DCHECK here (and similar below), thought to solicit
feedback.

The logic for the new maps only inserts an (element id -> node index) if the
layer has an element
id, so if it doesn't exist when we go to look it up I think we'd get a "default
value" as 0 per:
http://stackoverflow.com/a/25517832

It looks like these methods are only called if we actually have an animation and
so an element id
(see ElementAnimations::On{Filter,Opacity,Transform,ScrollOffset}Animated),
which means we
should have an associated effect/transform node. It's possible there is some
edge case around
animations being added/deleted in conjunction with layer/property state changing
that could
cause problems but I would expect that to show up as at least a few test
failures.

Since we're just switching from using one map to a different map I also don't
see an obvious
need to add/change unit tests but again feedback welcome.

[ajuma, 2017/01/10 00:39:11]

What if we DCHECK before looking up? e.g.:
DCHECK(element_id_to_effect_node_index.count(element_id) == 1);
Agreed.

[wkorman, 2017/01/10 21:34:12]

Done.

+      tree->property_trees()->element_id_to_effect_node_index[element_id];
+  tree->property_trees()->effect_tree.OnFilterAnimated(filters,
+                                                       effect_node_index, tree);
 }
 
 void LayerTreeHostImpl::SetTreeLayerOpacityMutated(ElementId element_id,
                                                    LayerTreeImpl* tree,
                                                    float opacity) {
   if (!tree)
     return;
 
-  const int layer_id = tree->LayerIdByElementId(element_id);
-  DCHECK(tree->property_trees()->IsInIdToIndexMap(
-      PropertyTrees::TreeType::EFFECT, layer_id));
-  const int effect_id =
-      tree->property_trees()->effect_id_to_index_map[layer_id];
-  if (effect_id != EffectTree::kInvalidNodeId)
-    tree->property_trees()->effect_tree.OnOpacityAnimated(opacity, effect_id,
-                                                          tree);
+  const int effect_node_index =
+      tree->property_trees()->element_id_to_effect_node_index[element_id];
+  tree->property_trees()->effect_tree.OnOpacityAnimated(
+      opacity, effect_node_index, tree);
 }
 
 void LayerTreeHostImpl::SetTreeLayerTransformMutated(
     ElementId element_id,
     LayerTreeImpl* tree,
     const gfx::Transform& transform) {
   if (!tree)
     return;
 
+  const int transform_node_index =
+      tree->property_trees()->element_id_to_transform_node_index[element_id];
+  tree->property_trees()->transform_tree.OnTransformAnimated(
+      transform, transform_node_index, tree);
   const int layer_id = tree->LayerIdByElementId(element_id);

[ajuma, 2017/01/10 00:39:11]

You can use LayerByElementId here to get the layer directly (without layer_id as
an intermediate step).

[wkorman, 2017/01/10 21:34:12]

Done.

-  DCHECK(tree->property_trees()->IsInIdToIndexMap(
-      PropertyTrees::TreeType::TRANSFORM, layer_id));
-  const int transform_id =
-      tree->property_trees()->transform_id_to_index_map[layer_id];
-  if (transform_id != TransformTree::kInvalidNodeId)
-    tree->property_trees()->transform_tree.OnTransformAnimated(
-        transform, transform_id, tree);
-  LayerImpl* layer = tree->LayerById(layer_id);
-  if (layer)
+  if (LayerImpl* layer = tree->LayerById(layer_id))
     layer->set_was_ever_ready_since_last_transform_animation(false);
 }
 
 void LayerTreeHostImpl::SetTreeLayerScrollOffsetMutated(
     ElementId element_id,
     LayerTreeImpl* tree,
     const gfx::ScrollOffset& scroll_offset) {
   if (!tree)
     return;
 
   const int layer_id = tree->LayerIdByElementId(element_id);
   DCHECK(tree->property_trees()->IsInIdToIndexMap(
-      PropertyTrees::TreeType::TRANSFORM, layer_id));
-  DCHECK(tree->property_trees()->IsInIdToIndexMap(
       PropertyTrees::TreeType::SCROLL, layer_id));
-  const int transform_id =
-      tree->property_trees()->transform_id_to_index_map[layer_id];
-  const int scroll_id =
+  int transform_node_index =
+      tree->property_trees()->element_id_to_transform_node_index[element_id];
+  // TODO(wkorman): Build map from element id to scroll node in property tree
+  // builder and make use of it below.
+  const int scroll_node_index =
       tree->property_trees()->scroll_id_to_index_map[layer_id];
-  if (transform_id != TransformTree::kInvalidNodeId &&
-      scroll_id != ScrollTree::kInvalidNodeId) {
+  if (scroll_node_index != ScrollTree::kInvalidNodeId) {
     tree->property_trees()->scroll_tree.OnScrollOffsetAnimated(
-        layer_id, transform_id, scroll_id, scroll_offset, tree);
+        layer_id, transform_node_index, scroll_node_index, scroll_offset, tree);
     // Run mutation callbacks to respond to updated scroll offset.
     Mutate(CurrentBeginFrameArgs().frame_time);
   }
 }
 
 bool LayerTreeHostImpl::AnimationsPreserveAxisAlignment(
     const LayerImpl* layer) const {
   return mutator_host_->AnimationsPreserveAxisAlignment(layer->element_id());
 }
 
@@ skipping 119 matching lines @@
       worker_context_visibility_ =
           worker_context->CacheController()->ClientBecameVisible();
     } else {
       worker_context->CacheController()->ClientBecameNotVisible(
           std::move(worker_context_visibility_));
     }
   }
 }
 
 }  // namespace cc