Refactor unreliable hit test code to avoid owning_layers

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 132616ef303232e01822a4a4dd03ebb7dd1dbb86..49db2ae765959942ca6660ae764df551d0f19d23 100644
--- a/cc/trees/layer_tree_host_impl.cc
+++ b/cc/trees/layer_tree_host_impl.cc
@@ -162,15 +162,10 @@ bool IsScrolledBy(LayerImpl* child, LayerImpl* ancestor) {
     return false;
 
   auto* property_trees = child->layer_tree_impl()->property_trees();
-  auto ancestor_scroll_id =
-      property_trees->layer_id_to_scroll_node_index.find(ancestor->id());
-  if (ancestor_scroll_id == property_trees->layer_id_to_scroll_node_index.end())
-    return false;
-
   ScrollTree& scroll_tree = 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->id == ancestor_scroll_id->second)
+    if (scroll_node->id == ancestor->scroll_tree_index())
       return true;
   }
   return false;
@@ -2629,28 +2624,6 @@ LayerImpl* LayerTreeHostImpl::FindScrollLayerForDeviceViewportPoint(
   return active_tree_->LayerById(impl_scroll_node->owning_layer_id);
 }
 
-static bool IsClosestScrollAncestor(LayerImpl* child,
-                                    LayerImpl* scroll_ancestor) {
-  DCHECK(scroll_ancestor);
-  if (!child)
-    return false;
-
-  auto* property_trees = child->layer_tree_impl()->property_trees();
-  auto ancestor_scroll_id =
-      property_trees->layer_id_to_scroll_node_index.find(scroll_ancestor->id());
-  if (ancestor_scroll_id == property_trees->layer_id_to_scroll_node_index.end())
-    return false;
-
-  ScrollTree& scroll_tree = property_trees->scroll_tree;
-  ScrollNode* scroll_node = scroll_tree.Node(child->scroll_tree_index());
-  for (; scroll_tree.parent(scroll_node);
-       scroll_node = scroll_tree.parent(scroll_node)) {
-    if (scroll_node->scrollable)

[pdr., 2017/02/27 21:40:05]

The refactoring in this patch is possible because
FindFirstScrollingLayerOrScrollbarLayerThatIsHitByPoint could return a layer
that directly created a scrollable scroll node, or a layer that did not (aka a
non-scrolling drawn scrollbar). If the layer created was a scrollable scroll
node, we can just use the scroll tree to directly check that we're in the
expected scroll subtree. If the layer did not create a scrollable scroll node,
the existing code would always return false; this subtle point has been made
explicit in IsInitialScrollHitTestReliable by falling back to the main thread
for drawn scrollbars.

-      return scroll_node->id == ancestor_scroll_id->second;
-  }
-  return false;
-}
-
 InputHandler::ScrollStatus LayerTreeHostImpl::ScrollBeginImpl(
     ScrollState* scroll_state,
     LayerImpl* scrolling_layer_impl,
@@ -2724,11 +2697,7 @@ InputHandler::ScrollStatus LayerTreeHostImpl::ScrollBegin(
         active_tree_->FindLayerThatIsHitByPoint(device_viewport_point);
 
     if (layer_impl) {
-      LayerImpl* scroll_layer_impl =
-          active_tree_->FindFirstScrollingLayerOrScrollbarLayerThatIsHitByPoint(
-              device_viewport_point);
-      if (scroll_layer_impl &&
-          !IsClosestScrollAncestor(layer_impl, scroll_layer_impl)) {
+      if (!IsInitialScrollHitTestReliable(layer_impl, device_viewport_point)) {
         scroll_status.thread = SCROLL_UNKNOWN;
         scroll_status.main_thread_scrolling_reasons =
             MainThreadScrollingReason::kFailedHitTest;
@@ -2754,6 +2723,49 @@ InputHandler::ScrollStatus LayerTreeHostImpl::ScrollBegin(
   return ScrollBeginImpl(scroll_state, scrolling_layer_impl, type);
 }
 
+// Some initial scroll tests are known to be unreliable and require falling
+// back to main thread scrolling.
+bool LayerTreeHostImpl::IsInitialScrollHitTestReliable(
+    LayerImpl* layer_impl,
+    const gfx::PointF& device_viewport_point) {
+  LayerImpl* first_scrolling_layer_or_drawn_scrollbar =
+      active_tree_->FindFirstScrollingLayerOrDrawnScrollbarThatIsHitByPoint(
+          device_viewport_point);
+
+  // If |layer_impl| is hit, we can reliably return true.
+  if (!first_scrolling_layer_or_drawn_scrollbar ||
+      first_scrolling_layer_or_drawn_scrollbar == layer_impl)

[ajuma, 2017/02/28 00:38:58]

If layer_impl is itself a non-scrollable drawn scrollbar, this returns true. I
think the old logic fell back to main thread scrolling in that case?

[pdr., 2017/02/28 01:01:35]

True, this should not check for equality. Updated and added a test.

+    return true;
+
+  ScrollNode* closest_scroll_node = nullptr;
+  auto& scroll_tree = active_tree_->property_trees()->scroll_tree;
+  ScrollNode* scroll_node = scroll_tree.Node(layer_impl->scroll_tree_index());
+  for (; scroll_tree.parent(scroll_node);
+       scroll_node = scroll_tree.parent(scroll_node)) {
+    if (scroll_node->scrollable) {
+      closest_scroll_node = scroll_node;
+      break;
+    }
+  }
+  if (!closest_scroll_node)
+    return false;
+
+  // If |first_scrolling_layer_or_drawn_scrollbar| is scrollable, it will
+  // create a scroll node. If this scroll node corresponds to first scrollable
+  // ancestor along the scroll tree for |layer_impl|, the hit test has not
+  // escaped to other areas of the scroll tree and is reliable.
+  if (first_scrolling_layer_or_drawn_scrollbar->scrollable()) {
+    return closest_scroll_node->id ==
+           first_scrolling_layer_or_drawn_scrollbar->scroll_tree_index();
+  }
+
+  // If |first_scrolling_layer_or_drawn_scrollbar| is not scrollable, it must
+  // be a drawn scrollbar. These hit tests require falling back to main-thread
+  // scrolling.
+  DCHECK(first_scrolling_layer_or_drawn_scrollbar->IsDrawnScrollbar());
+  return false;
+}
+
 InputHandler::ScrollStatus LayerTreeHostImpl::ScrollAnimatedBegin(
     const gfx::Point& viewport_point) {
   InputHandler::ScrollStatus scroll_status;