cc: Fix touch hit-testing for overlapping layers.

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 c9d453f11159dbf48755631fe0f21e7f4c3dc0c4..426239584f93af355dd99b3a7f8af62399bb13e2 100644
--- a/cc/trees/layer_tree_host_common.cc
+++ b/cc/trees/layer_tree_host_common.cc
@@ -2377,34 +2377,20 @@ LayerImpl* LayerTreeHostCommon::FindLayerThatIsHitByPoint(
 LayerImpl* LayerTreeHostCommon::FindLayerThatIsHitByPointInTouchHandlerRegion(
     gfx::PointF screen_space_point,
     const LayerImplList& render_surface_layer_list) {
-  LayerImpl* found_layer = NULL;
-
-  typedef LayerIterator<LayerImpl,
-                        LayerImplList,
-                        RenderSurfaceImpl,
-                        LayerIteratorActions::FrontToBack> 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);
-
-    if (!LayerHasTouchEventHandlersAt(screen_space_point, current_layer))
-      continue;

[enne (OOO), 2013/10/18 19:18:38]

I would have expected the fix here to be turning this continue into a return
NULL statement.

-
-    found_layer = current_layer;
-    break;
+  // First find out which layer was hit from the saved list of visible layers
+  // in the most recent frame.
+  LayerImpl* layer_impl = LayerTreeHostCommon::FindLayerThatIsHitByPoint(
+      screen_space_point,
+      render_surface_layer_list);
+
+  // Walk up the hierarchy and look for a layer with a touch event handler

[enne (OOO), 2013/10/18 19:18:38]

Can you help me understand why you need to walk up the hierarchy? It seems like
there could be sibling layers with touch handlers that could also be hit by that
point.

[sadrul, 2013/10/18 19:57:44]

Consider this example: http://jsfiddle.net/zcjJe/3/

The child div (green) extends out of the parent div (yellow), and into the gray
box. The yellow div has a touch event handler. When tapping on the green box
that overlaps with the gray box, but is outside of the yellow box, still should
trigger the touch handler on the parent (since the JS event propagates through
the ancestor nodes). This is why it is necessary to go up the tree.

If a non-touch layer is on top of a touch layer, then touching the non-touch
layer should not trigger the touch-event handler on the touch-layer (except for
crbug.com/269598 cases, but we need to fix that properly anyway).

Does that address your concerns?

[enne (OOO), 2013/10/21 19:52:04]

I understand that you need to do *a* walk and not just stop at the first layer
you hit.  However, I'm a little confused about what walk needs to be done.  Can
you clarify?

You're talking about propagating JS events, but this can't be a DOM tree walk,
because the GraphicsLayers are constructed via the RenderLayer z-order lists and
a RenderLayer's real parent will be a sibling in the GraphicsLayer tree if its
parent is not a stacking context.

As per my previous comments, I was expecting this to be a draw-order walk, but
maybe that's not right either.

(Does cc::Layer need to know an event propagation layer id?)

[sadrul, 2013/10/21 21:36:46]

I had to look at the history of the change to figure out why this is done like
this: https://chromiumcodereview.appspot.com/11534024

From what I can see, this was done since at that time, the touch-event handling
regions were being set on the root layer. So each individual layer wouldn't have
the hit-region, and so it was necessary to walk up the tree.

Now that that is not the case (thanks to
https://chromiumcodereview.appspot.com/11534024 etc.), we probably don't need to
walk up the tree at all anymore. A quick test that removes the loop below
doesn't seem to break any of the cc-tests. I can explore this farther, but I
would rather change the runtime-behaviour in a subsequent CL, and keep this CL
to simply make sure we test the same code we run.
You are right. I wasn't making sense.

+  // region that the given point hits.
+  for (; layer_impl; layer_impl = layer_impl->parent()) {
+    if (LayerTreeHostCommon::LayerHasTouchEventHandlersAt(screen_space_point,
+                                                          layer_impl))
+      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;
+  return layer_impl;
 }
 
 bool LayerTreeHostCommon::LayerHasTouchEventHandlersAt(