Scroll on main if impl-hit testing isn't guaranteed to be correct

cc/trees/layer_tree_host_common.cc

Index: cc/trees/layer_tree_host_common.cc
diff --git a/cc/trees/layer_tree_host_common.cc b/cc/trees/layer_tree_host_common.cc
index 4dc35522a28d6ca42ad70936f73eedb5b48ac3d8..ee8e864eff66f89eba3362ec95a3e2de0c535e90 100644
--- a/cc/trees/layer_tree_host_common.cc
+++ b/cc/trees/layer_tree_host_common.cc
@@ -2437,6 +2437,53 @@ static bool PointIsClippedBySurfaceOrClipRect(
   return false;
 }
 
+LayerImpl* LayerTreeHostCommon::FindFirstScrollingLayerThatIsHitByPoint(

[enne (OOO), 2014/04/15 21:25:54]

Ew, code duplication.  You're going to want something like this for raycasting
touch regions too.

How about a base::Callback or or templatize this with some class that has a bool
Query(LayerImpl*, gfx::PointF) function?

[Ian Vollick, 2014/04/15 23:27:27]

Yeah, it's pretty gross. Haste makes.. this cl. :)

It's not so cute, but I refactored the common bits in the loop to cut down on
duplication. WDYT?

+    const gfx::PointF& screen_space_point,
+    const LayerImplList& render_surface_layer_list) {
+  LayerImpl* found_layer = NULL;
+
+  typedef LayerIterator<LayerImpl> LayerIteratorType;
+  LayerIteratorType end = LayerIteratorType::End(&render_surface_layer_list);
+  for (LayerIteratorType it =
+           LayerIteratorType::Begin(&render_surface_layer_list);
+       it != end;
+       ++it) {
+    // We don't want to consider render_surfaces for hit testing.
+    if (!it.represents_itself())
+      continue;
+
+    LayerImpl* current_layer = (*it);
+
+    gfx::RectF content_rect(current_layer->content_bounds());
+    if (!PointHitsRect(screen_space_point,
+                       current_layer->screen_space_transform(),
+                       content_rect))
+      continue;
+
+    // At this point, we think the point does hit the layer, but we need to walk
+    // up the parents to ensure that the layer was not clipped in such a way
+    // that the hit point actually should not hit the layer.
+    if (PointIsClippedBySurfaceOrClipRect(screen_space_point, current_layer))
+      continue;
+
+    // Skip the HUD layer.

[enne (OOO), 2014/04/15 21:25:54]

Sadness.

[Ian Vollick, 2014/04/15 23:27:27]

yeh.

+    if (current_layer == current_layer->layer_tree_impl()->hud_layer())
+      continue;
+
+    if (current_layer->scrollable()) {
+      found_layer = current_layer;

[enne (OOO), 2014/04/15 21:25:54]

readability/debuggability nit: just return current_layer here, and return NULL
instead of break and at the end?

[Ian Vollick, 2014/04/15 23:27:27]

Done.

+      break;
+    }
+
+    if (current_layer->contents_opaque())

[enne (OOO), 2014/04/15 21:25:54]

I think for correctness, it is always ok to return NULL from this function
(since it will punt to the main thread).  However, what case is this solving?

A (scrollable)
-B (not scrollable child, scrolls with A, opaque)

For a point that hits A+B, you would return NULL from this function and scroll
on the main thread.  That seems inefficient.  Why not just return A from this
function and not have this case?

[Ian Vollick, 2014/04/15 23:27:27]

Good point. This was a stale idea from the algorithm Rick and I had been
discussing, but it's not actually helpful here. Removed.

+      break;
+  }
+
+  // This can potentially return NULL, which means the screen_space_point did
+  // not successfully hit test any layers, not even the root layer.
+  return found_layer;
+}
+
 LayerImpl* LayerTreeHostCommon::FindLayerThatIsHitByPoint(
     const gfx::PointF& screen_space_point,
     const LayerImplList& render_surface_layer_list) {