Refactor unreliable hit test code to avoid owning_layers

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 144 matching lines @@
 }
 
 // Return true if scrollable node for 'ancestor' is the same as 'child' or an
 // ancestor along the scroll tree.
 bool IsScrolledBy(LayerImpl* child, LayerImpl* ancestor) {
   DCHECK(ancestor && ancestor->scrollable());
   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(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;
 }
 
 }  // namespace
 
 DEFINE_SCOPED_UMA_HISTOGRAM_TIMER(PendingTreeDurationHistogramTimer,
                                   "Scheduling.%s.PendingTreeDuration");
 
@@ skipping 2438 matching lines @@
     }
   }
 
   // TODO(pdr): Refactor this function to directly return |impl_scroll_node|
   // instead of using ScrollNode's owning_layer_id to return a LayerImpl.
   if (!impl_scroll_node)
     return nullptr;
   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,
     InputHandler::ScrollInputType type) {
   DCHECK(scroll_state);
   DCHECK(scroll_state->delta_x() == 0 && scroll_state->delta_y() == 0);
 
   InputHandler::ScrollStatus scroll_status;
   scroll_status.main_thread_scrolling_reasons =
       MainThreadScrollingReason::kNotScrollingOnMain;
@@ skipping 53 matching lines @@
 
     gfx::Point viewport_point(scroll_state->position_x(),
                               scroll_state->position_y());
 
     gfx::PointF device_viewport_point = gfx::ScalePoint(
         gfx::PointF(viewport_point), active_tree_->device_scale_factor());
     LayerImpl* layer_impl =
         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;
         return scroll_status;
       }
     }
 
     scrolling_layer_impl = FindScrollLayerForDeviceViewportPoint(
         device_viewport_point, type, layer_impl, &scroll_on_main_thread,
         &scroll_status.main_thread_scrolling_reasons);
   }
 
   if (scroll_on_main_thread) {
     RecordCompositorSlowScrollMetric(type, MAIN_THREAD);
 
     scroll_status.thread = SCROLL_ON_MAIN_THREAD;
     return scroll_status;
   } else if (scrolling_layer_impl) {
     scroll_affects_scroll_handler_ =
         scrolling_layer_impl->layer_tree_impl()->have_scroll_event_handlers();
   }
 
   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;
   scroll_status.main_thread_scrolling_reasons =
       MainThreadScrollingReason::kNotScrollingOnMain;
   ScrollTree& scroll_tree = active_tree_->property_trees()->scroll_tree;
   ScrollNode* scroll_node = scroll_tree.CurrentlyScrollingNode();
   if (scroll_node) {
     gfx::Vector2dF delta;
 
@@ skipping 1373 matching lines @@
       worker_context_visibility_ =
           worker_context->CacheController()->ClientBecameVisible();
     } else {
       worker_context->CacheController()->ClientBecameNotVisible(
           std::move(worker_context_visibility_));
     }
   }
 }
 
 }  // namespace cc