cc: Fix hit-testing in zero-opacity layers.

cc/trees/layer_tree_host_common_unittest.cc

Index: cc/trees/layer_tree_host_common_unittest.cc
diff --git a/cc/trees/layer_tree_host_common_unittest.cc b/cc/trees/layer_tree_host_common_unittest.cc
index e9182e845a2fad6065b0ed3f6844ebb2f3f587fd..baf32bd028a836c589d636cb41bf06bb11279e69 100644
--- a/cc/trees/layer_tree_host_common_unittest.cc
+++ b/cc/trees/layer_tree_host_common_unittest.cc
@@ -5416,6 +5416,192 @@ TEST_F(LayerTreeHostCommonTest, HitTestingForEmptyLayers) {
   EXPECT_EQ(4, result_layer->id());
 }
 
+TEST_F(LayerTreeHostCommonTest, ZeroOpacityLayersWithEventHandlerChild) {
+  FakeImplProxy proxy;
+  FakeLayerTreeHostImpl host_impl(&proxy);
+  const int kRootId = 1;
+  const int kNoHandlerChildId = 4;
+  const int kGrandChildWithEventId = 5;
+  const int kGrandChildWithoutEventId = 6;
+
+  scoped_ptr<LayerImpl> root =
+      LayerImpl::Create(host_impl.active_tree(), 1);
+  gfx::Transform identity_matrix;
+  gfx::PointF anchor;
+  gfx::PointF position;
+  gfx::Size bounds(100, 100);
+  SetLayerPropertiesForTesting(root.get(),
+                               identity_matrix,
+                               identity_matrix,
+                               anchor,
+                               position,
+                               bounds,
+                               false);
+  root->SetDrawsContent(true);
+
+  // Child - Zero opacity, without any event handler.
+  scoped_ptr<LayerImpl> child =
+      LayerImpl::Create(host_impl.active_tree(), kNoHandlerChildId);
+  SetLayerPropertiesForTesting(child.get(),
+                               identity_matrix,
+                               identity_matrix,
+                               anchor,
+                               gfx::PointF(10.f, 10.f),
+                               gfx::Size(30, 30),
+                               false);
+  child->SetOpacity(0.f);
+  child->SetDrawsContent(true);
+
+  // Grandchild - with a wheel handler.
+  {
+    scoped_ptr<LayerImpl> grand_child =
+        LayerImpl::Create(host_impl.active_tree(), kGrandChildWithEventId);
+    SetLayerPropertiesForTesting(grand_child.get(),
+                                 identity_matrix,
+                                 identity_matrix,
+                                 gfx::PointF(),
+                                 gfx::PointF(10, 10),
+                                 gfx::Size(10, 10),
+                                 false);
+    grand_child->SetHaveWheelEventHandlers(true);
+    grand_child->SetDrawsContent(true);
+    child->AddChild(grand_child.Pass());
+  }
+
+  // Grandchild - without any event handlers.
+  {
+    scoped_ptr<LayerImpl> grand_child =
+        LayerImpl::Create(host_impl.active_tree(), kGrandChildWithoutEventId);
+    SetLayerPropertiesForTesting(grand_child.get(),
+                                 identity_matrix,
+                                 identity_matrix,
+                                 gfx::PointF(),
+                                 gfx::PointF(20, 10),
+                                 gfx::Size(10, 10),
+                                 false);
+    grand_child->SetDrawsContent(true);
+    child->AddChild(grand_child.Pass());
+  }
+
+  root->AddChild(child.Pass());
+
+  LayerImplList render_surface_layer_list;
+  LayerTreeHostCommon::CalcDrawPropsImplInputsForTesting inputs(
+      root.get(), root->bounds(), &render_surface_layer_list);
+  inputs.can_adjust_raster_scales = true;
+  LayerTreeHostCommon::CalculateDrawProperties(&inputs);
+
+  // Verify that both the root layer and zero-opacity child layer are in the
+  // RSLL (since the child layer has multiple drawing children).
+  ASSERT_EQ(2u, render_surface_layer_list.size());
+  EXPECT_EQ(kRootId, render_surface_layer_list[0]->id());
+  EXPECT_EQ(kNoHandlerChildId, render_surface_layer_list[1]->id());
+
+  // The zero-opacity child layer should not be in the layer-list of the root's
+  // surface.
+  ASSERT_EQ(1u, root->render_surface()->layer_list().size());
+  EXPECT_EQ(kRootId, root->render_surface()->layer_list().at(0)->id());
+
+  ASSERT_EQ(2u, root->children()[0]->render_surface()->layer_list().size());
+  EXPECT_EQ(kNoHandlerChildId,

[danakj, 2013/10/09 19:14:59]

This shouldn't be present, it doesn't have a handler and it doesn't draw.

+            root->children()[0]->render_surface()->layer_list().at(0)->id());
+  EXPECT_EQ(kGrandChildWithEventId,
+            root->children()[0]->render_surface()->layer_list().at(1)->id());
+}
+
+TEST_F(LayerTreeHostCommonTest,
+       ZeroOpacityLayerRenderSurfaceChildEventHandlerGrandChild) {
+  FakeImplProxy proxy;
+  FakeLayerTreeHostImpl host_impl(&proxy);
+  const int kRootId = 1;
+  const int kForceSurfaceLayer = 4;
+  const int kZeroOpacityLayer = 5;
+  const int kTouchHandlerLayer = 6;
+
+  scoped_ptr<LayerImpl> root =
+      LayerImpl::Create(host_impl.active_tree(), 1);
+  gfx::Transform identity_matrix;
+  gfx::PointF anchor;
+  gfx::PointF position;
+  gfx::Size bounds(100, 100);
+  SetLayerPropertiesForTesting(root.get(),
+                               identity_matrix,
+                               identity_matrix,
+                               anchor,
+                               position,
+                               bounds,
+                               false);
+  root->SetDrawsContent(true);
+
+  // Layer with zero opacity.
+  scoped_ptr<LayerImpl> zero_opacity_layer =
+      LayerImpl::Create(host_impl.active_tree(), kZeroOpacityLayer);
+  SetLayerPropertiesForTesting(zero_opacity_layer.get(),
+                               identity_matrix,
+                               identity_matrix,
+                               anchor,
+                               gfx::PointF(10.f, 10.f),
+                               gfx::Size(50, 50),
+                               false);
+  zero_opacity_layer->SetOpacity(0.f);
+
+  // Child with a render surface.
+  scoped_ptr<LayerImpl> render_surface_layer =
+      LayerImpl::Create(host_impl.active_tree(), kForceSurfaceLayer);
+  SetLayerPropertiesForTesting(render_surface_layer.get(),
+                               identity_matrix,
+                               identity_matrix,
+                               gfx::PointF(),
+                               gfx::PointF(10, 10),
+                               gfx::Size(40, 40),
+                               false);
+  render_surface_layer->SetForceRenderSurface(true);
+  render_surface_layer->SetDrawsContent(true);
+
+  // Grandchild with a touch handler.
+  scoped_ptr<LayerImpl> touch_handler_layer =
+      LayerImpl::Create(host_impl.active_tree(), kTouchHandlerLayer);
+  SetLayerPropertiesForTesting(touch_handler_layer.get(),
+                               identity_matrix,
+                               identity_matrix,
+                               gfx::PointF(),
+                               gfx::PointF(10, 10),
+                               gfx::Size(10, 10),
+                               false);
+  touch_handler_layer->SetTouchEventHandlerRegion(gfx::Rect(0, 0, 10, 10));
+  touch_handler_layer->SetDrawsContent(true);
+
+  render_surface_layer->AddChild(touch_handler_layer.Pass());
+  zero_opacity_layer->AddChild(render_surface_layer.Pass());
+  root->AddChild(zero_opacity_layer.Pass());
+
+  LayerImplList render_surface_layer_list;
+  LayerTreeHostCommon::CalcDrawPropsImplInputsForTesting inputs(
+      root.get(), root->bounds(), &render_surface_layer_list);
+  inputs.can_adjust_raster_scales = true;
+  LayerTreeHostCommon::CalculateDrawProperties(&inputs);
+
+  // Verify that both the root layer and child layer are in the RSLL (since both
+  // own surfaces).
+  ASSERT_EQ(3u, render_surface_layer_list.size());
+  EXPECT_EQ(kRootId, render_surface_layer_list[0]->id());

[sadrul, 2013/10/10 18:01:31]

The other issue I am hitting is, when we get kRootId from a hit-test on
|touch_handler_layer| (looks like because the root is first in the RSLL, and the
hit-test on the root is successful, and the touch_handler_layer is not in the
root's layer_list()). Should the list be reversed for hit-test?

[enne (OOO), 2013/10/15 17:56:49]

Yes.  The front-to-back iterator that the
LayerTreeHostCommon::FindLayerThatIsHitByPoint function uses goes in reverse
order.

+  EXPECT_EQ(kZeroOpacityLayer, render_surface_layer_list[1]->id());
+  EXPECT_EQ(kForceSurfaceLayer, render_surface_layer_list[2]->id());
+
+  ASSERT_EQ(1u, root->render_surface()->layer_list().size());
+  EXPECT_EQ(kRootId, root->render_surface()->layer_list().at(0)->id());
+
+  LayerImpl* zero_opacity_raw = root->children()[0];
+  ASSERT_EQ(0u,
+            zero_opacity_raw->render_surface()->layer_list().size());

[sadrul, 2013/10/10 17:08:46]

One of the issues I ran into is that RemoveSurfaceForEarlyExit() removes the
surfaces that don't have any layers. So I introduced a
|SubtreeGlobals::can_remove_surface_for_early_exit| (and a corresponding entry
in CalcDrawPropsInput), which defaults to true, and I set it to false from the
test here. This sounds good to you?

[danakj, 2013/10/10 17:16:38]

Hm...

Maybe it's okay and better to not have those surfaces with 0 layers. As long as
the LayerIterator can correctly iterate over everything then we're okay. I just
want to be careful to not end up with an invalid RSLL that the LayerIterator
blows up on.

Since those surfaces will not have a layer representing them in another
surface's layer_list() either, there's maybe no reason to keep them.

[sadrul, 2013/10/10 17:41:39]

|RemoveSurfaceForEarlyExit()| also removes all surfaces after the empty surface.
So we also lose the |kForceSurfaceLayer| from the RSLL :(

  // Technically, we know that the layer we want to remove should be
  // at the back of the render_surface_layer_list. However, we have had
  // bugs before that added unnecessary layers here
  // (https://bugs.webkit.org/show_bug.cgi?id=74147), but that causes
  // things to crash. So here we proactively remove any additional
  // layers from the end of the list.
  while (render_surface_layer_list->back() != layer_to_remove) {
    render_surface_layer_list->back()->ClearRenderSurface();
    render_surface_layer_list->pop_back();
  }
  DCHECK_EQ(render_surface_layer_list->back(), layer_to_remove);

Should I change this piece of code to do something different?

[enne (OOO), 2013/10/15 17:56:49]

Ack.  That's really unfortunate.  As the comment says, this safety has been
useful in the past in preventing crashes here.

I am also not fully convinced that the front to back iterator will do the right
thing if these surfaces are skipped.

One alternative that Dana and I talked out a little bit was just to make one
flattened list hit-testing list as an additional output from
CalculateDrawProperties.  This would keep the RSLL (things that get drawn)
separate from hit-testing.  Would that make things easier?

+
+  LayerImpl* render_surface_raw = zero_opacity_raw->children()[0];
+  ASSERT_EQ(1u,
+            render_surface_raw->render_surface()->layer_list().size());
+  EXPECT_EQ(kTouchHandlerLayer,
+            render_surface_raw->render_surface()->layer_list().at(0)->id());
+}
+
 TEST_F(LayerTreeHostCommonTest,
        HitCheckingTouchHandlerRegionsForEmptyLayerList) {
   // Hit checking on an empty render_surface_layer_list should return a null