Refactor IsScrolledBy and IsClosestScrollAncestor to use ScrollNode ids

cc/trees/layer_tree_host_impl.cc

Index: cc/trees/layer_tree_host_impl.cc
diff --git a/cc/trees/layer_tree_host_impl.cc b/cc/trees/layer_tree_host_impl.cc
index 75194f6628628b5b8c7a27940422183a691cd0ad..91131a6e4a78cad2d82909adb79182990a570fcf 100644
--- a/cc/trees/layer_tree_host_impl.cc
+++ b/cc/trees/layer_tree_host_impl.cc
@@ -154,8 +154,8 @@ void RecordCompositorSlowScrollMetric(InputHandler::ScrollInputType type,
   }
 }
 
-// Return true if scrollable 'ancestor' is the same layer as 'child' or its
-// ancestor along the scroll tree.
+// Return true if scrollable 'ancestor' has the same scroll node as 'child' or
+// an ancestor along the scroll tree.
 bool IsScrolledBy(LayerImpl* child, LayerImpl* ancestor) {
   DCHECK(ancestor && ancestor->scrollable());
   if (!child)
@@ -165,7 +165,7 @@ bool IsScrolledBy(LayerImpl* child, LayerImpl* ancestor) {
       child->layer_tree_impl()->property_trees()->scroll_tree;
   for (ScrollNode* scroll_node = scroll_tree.Node(child->scroll_tree_index());
        scroll_node; scroll_node = scroll_tree.parent(scroll_node)) {
-    if (scroll_node->owning_layer_id == ancestor->id())
+    if (scroll_node->id == ancestor->scroll_tree_index())

[ajuma, 2017/02/21 16:05:25]

Just wanted to point out that this could change behavior if IsScrolledBy was
called with |ancestor| not owning a scroll node (since previously, that would
always return false, but now it could return true). But it looks like this only
gets called when |ancestor| owns a scroll node.

[bokan, 2017/02/21 19:45:25]

Ali, could you help me understand how that could happen? If ancestor doesn't own
a ScrollNode, won't scroll_tree_index be invalid and this check always be false?

[ajuma, 2017/02/21 20:07:56]

Every layer has a scroll_tree_index. If a layer doesn't own a ScrollNode, it's
scroll_tree_index will be that of the closest ancestor that owns one (where
'ancestor' is either real ancestor in the main-thread Layer tree or
Layer::scroll_parent ancestor if there is one). Non-test LayerImpls should never
have invalid indices for any of the property trees.

[bokan, 2017/02/21 20:25:22]

Ah, got it - I was misunderstanding how that worked. Thanks.

[pdr., 2017/02/23 05:44:26]

+1, I had the same misunderstanding.

       return true;
   }
   return false;
@@ -2626,7 +2626,7 @@ static bool IsClosestScrollAncestor(LayerImpl* child,
   for (; scroll_tree.parent(scroll_node);
        scroll_node = scroll_tree.parent(scroll_node)) {
     if (scroll_node->scrollable)
-      return scroll_node->owning_layer_id == scroll_ancestor->id();
+      return scroll_node->id == scroll_ancestor->scroll_tree_index();

[ajuma, 2017/02/21 16:05:25]

Same comment as above, but this time it looks like this can indeed get called
(from ScrollBegin) with |scroll_ancestor| not owning a scroll node (it can be a
scrollbar layer that isn't itself scrollable). For background on why scrollbar
layers are a special case, see https://codereview.chromium.org/1878323002 and
https://codereview.chromium.org/238803005. I wonder if we just need to make that
special-casing more explicit somehow? sunxd@, any ideas?

[sunxd, 2017/02/22 22:25:19]

In some cases we have MainThreadScrollingReason::kScrollbarScrolling for
scrollbars, but at least for SolidColorScrollbarLayer, we do not have such a
reason, so probably we do not have a scroll node for this layer.

   }
   return false;
 }