Track the currently scrolling ScrollNode instead of the scrolling layer

cc/trees/layer_tree_impl.cc

Index: cc/trees/layer_tree_impl.cc
diff --git a/cc/trees/layer_tree_impl.cc b/cc/trees/layer_tree_impl.cc
index fae5f7c6d62f063e7e0508d78287300a3795fe1a..2c16b5f4377ca16d35f07a170919b10556931a9e 100644
--- a/cc/trees/layer_tree_impl.cc
+++ b/cc/trees/layer_tree_impl.cc
@@ -63,7 +63,7 @@ LayerTreeImpl::LayerTreeImpl(
       hud_layer_(nullptr),
       background_color_(0),
       has_transparent_background_(false),
-      last_scrolled_layer_id_(Layer::INVALID_ID),
+      last_scrolled_scroll_node_index_(ScrollTree::kInvalidNodeId),
       overscroll_elasticity_layer_id_(Layer::INVALID_ID),
       page_scale_layer_id_(Layer::INVALID_ID),
       inner_viewport_scroll_layer_id_(Layer::INVALID_ID),
@@ -436,9 +436,9 @@ void LayerTreeImpl::PushPropertiesTo(LayerTreeImpl* target_tree) {
   // The request queue should have been processed and does not require a push.
   DCHECK_EQ(ui_resource_request_queue_.size(), 0u);
 
-  LayerImpl* layer = target_tree->CurrentlyScrollingLayer();
+  auto* currently_scrolling_node = target_tree->CurrentlyScrollingNode();

[ajuma, 2017/02/28 22:59:37]

This gets a pointer to a node in the target tree's old screen. It looks like
that could get destroyed or overwritten on the next line when the new trees get
set (say if property tree topology changes). It seems safer to use a stable id
for this step (which, for now, would be owning_layer_id).

[pdr., 2017/03/01 07:21:03]

You're right, and fixing this makes the TreeSynch test much saner.

Longer-term, is there any existing mapping between property tree nodes across
active->pending? It shouldn't be too hard to make one for the current scroll
node, but it'll be a little awkward doing that using just stable layers.

[ajuma, 2017/03/01 15:29:20]

I think the longer term approach would be to use the mapping from element id to
index (element_id_to_scroll_node_index). In fact that might even work now, since
it looks like all scrollable layers and nodes are already assigned an ElementId.

   target_tree->SetPropertyTrees(&property_trees_);
-  target_tree->SetCurrentlyScrollingLayer(layer);
+  target_tree->SetCurrentlyScrollingNode(currently_scrolling_node);
   target_tree->property_trees()->scroll_tree.PushScrollUpdatesFromPendingTree(
       &property_trees_, target_tree);
 
@@ -629,45 +629,47 @@ LayerImpl* LayerTreeImpl::OuterViewportContainerLayer() const {
              : NULL;
 }
 
-LayerImpl* LayerTreeImpl::CurrentlyScrollingLayer() const {
+ScrollNode* LayerTreeImpl::CurrentlyScrollingNode() {
   DCHECK(IsActiveTree());
-  const ScrollNode* scroll_node =
-      property_trees_.scroll_tree.CurrentlyScrollingNode();
-  return LayerById(scroll_node ? scroll_node->owning_layer_id
-                               : Layer::INVALID_ID);
+  return property_trees_.scroll_tree.CurrentlyScrollingNode();
 }
 
-int LayerTreeImpl::LastScrolledLayerId() const {
-  return last_scrolled_layer_id_;
+const ScrollNode* LayerTreeImpl::CurrentlyScrollingNode() const {
+  return property_trees_.scroll_tree.CurrentlyScrollingNode();
 }
 
-void LayerTreeImpl::SetCurrentlyScrollingLayer(LayerImpl* layer) {
+int LayerTreeImpl::LastScrolledScrollNodeIndex() const {
+  return last_scrolled_scroll_node_index_;
+}
+
+void LayerTreeImpl::SetCurrentlyScrollingNode(ScrollNode* node) {
+  if (node)
+    last_scrolled_scroll_node_index_ = node->id;
+
   ScrollTree& scroll_tree = property_trees()->scroll_tree;
-  ScrollNode* scroll_node = scroll_tree.CurrentlyScrollingNode();
-  int old_id = scroll_node ? scroll_node->owning_layer_id : Layer::INVALID_ID;
-  int new_id = layer ? layer->id() : Layer::INVALID_ID;
-  int new_scroll_node_id =
-      layer ? layer->scroll_tree_index() : ScrollTree::kInvalidNodeId;
-  if (layer)
-    last_scrolled_layer_id_ = new_id;
-
-  if (old_id == new_id)
+  ScrollNode* old_node = scroll_tree.CurrentlyScrollingNode();
+  // TODO(pdr): Refactor these to not use owning_layer_id.
+  int old_layer_id = old_node ? old_node->owning_layer_id : Layer::INVALID_ID;
+  int new_layer_id = node ? node->owning_layer_id : Layer::INVALID_ID;
+
+  if (old_layer_id == new_layer_id)
     return;
 
   ScrollbarAnimationController* old_animation_controller =
-      layer_tree_host_impl_->ScrollbarAnimationControllerForId(old_id);
+      layer_tree_host_impl_->ScrollbarAnimationControllerForId(old_layer_id);
   ScrollbarAnimationController* new_animation_controller =
-      layer_tree_host_impl_->ScrollbarAnimationControllerForId(new_id);
+      layer_tree_host_impl_->ScrollbarAnimationControllerForId(new_layer_id);
 
   if (old_animation_controller)
     old_animation_controller->DidScrollEnd();
-  scroll_tree.set_currently_scrolling_node(new_scroll_node_id);
+  scroll_tree.set_currently_scrolling_node(node ? node->id
+                                                : ScrollTree::kInvalidNodeId);
   if (new_animation_controller)
     new_animation_controller->DidScrollBegin();
 }
 
-void LayerTreeImpl::ClearCurrentlyScrollingLayer() {
-  SetCurrentlyScrollingLayer(NULL);
+void LayerTreeImpl::ClearCurrentlyScrollingNode() {
+  SetCurrentlyScrollingNode(nullptr);
 }
 
 float LayerTreeImpl::ClampPageScaleFactorToLimits(