Change HitTestQuery to operate in floating point and add short circuit.

1. Changed HitTestQuery to operate in floating point for precision and
updated tests.

2. Re-enabled SitePerProcessNonIntegerScaleFactorHitTestBrowserTest.
MouseClickWithNonIntegerScaleFactor for viz hit-test.

3. Previously, SitePerProcessNonIntegerScaleFactorHitTestBrowserTest.
MouseClickWithNonIntegerScaleFactor/0 actually went into the short
circuit block (when there's only one RenderWidgetHostView) for non-viz
hit-test, so it was not testing targeting. Added
SitePerProcessNonIntegerScaleFactorHitTestBrowserTest.
NestedSurfaceHitTestTest to test targeting for non-integer DSF.

4. Moved short circuit block to be for both viz hit-test and non-viz
hit-test (tested MouseClickWithNonIntegerScaleFactor for viz hit-test
before adding this short circuit).

Bug: 816746
Test: site_per_process_hit_test_browsertests viz_unittests
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I0bbefff2d1ca0367fa28c17df586f63d7a925114
Reviewed-on: https://chromium-review.googlesource.com/938964
Commit-Queue: Ria Jiang <riajiang@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539930}

components/viz/host/hit_test/hit_test_query.cc

@@ -5,6 +5,7 @@
 #include "components/viz/host/hit_test/hit_test_query.h"
 
 #include "services/viz/public/interfaces/hit_test/hit_test_region_list.mojom.h"
+#include "ui/gfx/geometry/point_conversions.h"
 
 namespace viz {
 namespace {
@@ -51,7 +52,7 @@ void HitTestQuery::SwitchActiveAggregatedHitTestRegionList(
 
 Target HitTestQuery::FindTargetForLocation(
     EventSource event_source,
-    const gfx::Point& location_in_root) const {
+    const gfx::PointF& location_in_root) const {
   Target target;
   if (!active_hit_test_list_size_)
     return target;
@@ -61,14 +62,14 @@ Target HitTestQuery::FindTargetForLocation(
   return target;
 }
 
-gfx::Point HitTestQuery::TransformLocationForTarget(
+gfx::PointF HitTestQuery::TransformLocationForTarget(
     EventSource event_source,
     const std::vector<FrameSinkId>& target_ancestors,
-    const gfx::Point& location_in_root) const {
+    const gfx::PointF& location_in_root) const {
   if (!active_hit_test_list_size_)
     return location_in_root;
 
-  gfx::Point location_in_target(location_in_root);
+  gfx::PointF location_in_target(location_in_root);
   // TODO(riajiang): Cache the matrix product such that the transform can be
   // done immediately. crbug/758062.
   DCHECK(target_ancestors.size() > 0u &&
@@ -84,12 +85,12 @@ gfx::Point HitTestQuery::TransformLocationForTarget(
 
 bool HitTestQuery::FindTargetInRegionForLocation(
     EventSource event_source,
-    const gfx::Point& location_in_parent,
+    const gfx::PointF& location_in_parent,
     AggregatedHitTestRegion* region,
     Target* target) const {
-  gfx::Point location_transformed(location_in_parent);
+  gfx::PointF location_transformed(location_in_parent);
   region->transform.TransformPoint(&location_transformed);
-  if (!region->rect.Contains(location_transformed))
+  if (!gfx::RectF(region->rect).Contains(location_transformed))
     return false;
 
   if (region->child_count < 0 ||
@@ -100,8 +101,8 @@ bool HitTestQuery::FindTargetInRegionForLocation(
   AggregatedHitTestRegion* child_region = region + 1;
   AggregatedHitTestRegion* child_region_end =
       child_region + region->child_count;
-  gfx::Point location_in_target(location_transformed);
-  location_in_target.Offset(-region->rect.x(), -region->rect.y());
+  gfx::PointF location_in_target =
+      location_transformed - region->rect.OffsetFromOrigin();
   while (child_region < child_region_end) {
     if (FindTargetInRegionForLocation(event_source, location_in_target,
                                       child_region, target)) {
@@ -135,7 +136,7 @@ bool HitTestQuery::TransformLocationForTargetRecursively(
     const std::vector<FrameSinkId>& target_ancestors,
     size_t target_ancestor,
     AggregatedHitTestRegion* region,
-    gfx::Point* location_in_target) const {
+    gfx::PointF* location_in_target) const {
   bool match_touch_or_mouse_region =
       ShouldUseTouchBounds(event_source)
           ? (region->flags & mojom::kHitTestTouch) != 0u

components/viz/host/hit_test/hit_test_query.h

@@ -11,14 +11,14 @@
 #include "components/viz/common/hit_test/aggregated_hit_test_region.h"
 #include "components/viz/host/viz_host_export.h"
 #include "mojo/public/cpp/system/buffer.h"
-#include "ui/gfx/geometry/point.h"
+#include "ui/gfx/geometry/point_f.h"
 
 namespace viz {
 
 struct Target {
   FrameSinkId frame_sink_id;
   // Coordinates in the coordinate system of the target FrameSinkId.
-  gfx::Point location_in_target;
+  gfx::PointF location_in_target;
   // Different flags are defined in services/viz/public/interfaces/hit_test/
   // hit_test_region_list.mojom.
   uint32_t flags = 0;
@@ -77,24 +77,24 @@ class VIZ_HOST_EXPORT HitTestQuery {
   // transfrom-from-e-to-c and transform-from-c-to-b then we get 3 in the
   // coordinate system of b.
   Target FindTargetForLocation(EventSource event_source,
-                               const gfx::Point& location_in_root) const;
+                               const gfx::PointF& location_in_root) const;
 
   // When a target window is already known, e.g. capture/latched window, convert
   // |location_in_root| to be in the coordinate space of the target.
   // |target_ancestors| contains the FrameSinkId from target to root.
   // |target_ancestors.front()| is the target, and |target_ancestors.back()|
   // is the root.
-  gfx::Point TransformLocationForTarget(
+  gfx::PointF TransformLocationForTarget(
       EventSource event_source,
       const std::vector<FrameSinkId>& target_ancestors,
-      const gfx::Point& location_in_root) const;
+      const gfx::PointF& location_in_root) const;
 
  private:
   // Helper function to find |target| for |location_in_parent| in the |region|,
   // returns true if a target is found and false otherwise. |location_in_parent|
   // is in the coordinate space of |region|'s parent.
   bool FindTargetInRegionForLocation(EventSource event_source,
-                                     const gfx::Point& location_in_parent,
+                                     const gfx::PointF& location_in_parent,
                                      AggregatedHitTestRegion* region,
                                      Target* target) const;
 
@@ -106,7 +106,7 @@ class VIZ_HOST_EXPORT HitTestQuery {
       const std::vector<FrameSinkId>& target_ancestors,
       size_t target_ancestor,
       AggregatedHitTestRegion* region,
-      gfx::Point* location_in_target) const;
+      gfx::PointF* location_in_target) const;
 
   uint32_t handle_buffer_sizes_[2];
   mojo::ScopedSharedBufferMapping handle_buffers_[2];

content/browser/renderer_host/render_widget_host_input_event_router.cc

@@ -261,8 +261,14 @@ RenderWidgetTargetResult RenderWidgetHostInputEventRouter::FindViewAtLocation(
     const gfx::PointF& point_in_screen,
     viz::EventSource source,
     gfx::PointF* transformed_point) const {
-  viz::FrameSinkId frame_sink_id;
+  // Short circuit if owner_map has only one RenderWidgetHostView, no need for
+  // hit testing.
+  if (owner_map_.size() <= 1) {
+    *transformed_point = point;
+    return {root_view, false, *transformed_point};
+  }
 
+  viz::FrameSinkId frame_sink_id;
   bool query_renderer = false;
   if (use_viz_hit_test_) {
     const auto& display_hit_test_query_map =
@@ -275,35 +281,24 @@ RenderWidgetTargetResult RenderWidgetHostInputEventRouter::FindViewAtLocation(
     // display HitTestQuery does a hit test in the coordinate space of the root
     // window. The following translation should account for that discrepancy.
     // TODO(riajiang): Get rid of |point_in_screen| since it's not used.
-    gfx::Point point_in_root =
-        gfx::ToFlooredPoint(point) + root_view->GetOffsetFromRootSurface();
+    gfx::PointF point_in_root = point + root_view->GetOffsetFromRootSurface();
     viz::HitTestQuery* query = iter->second.get();
-    // TODO(kenrb): Add the short circuit to avoid hit testing when there is
-    // only one RenderWidgetHostView in the map. It is absent right now to
-    // make it easier to test the Viz hit testing code in development.
     float device_scale_factor = root_view->GetDeviceScaleFactor();
     DCHECK(device_scale_factor != 0.0f);
-    gfx::Point point_in_root_in_pixels =
+    gfx::PointF point_in_root_in_pixels =
         gfx::ConvertPointToPixel(device_scale_factor, point_in_root);
     viz::Target target =
         query->FindTargetForLocation(source, point_in_root_in_pixels);
     frame_sink_id = target.frame_sink_id;
     if (frame_sink_id.is_valid()) {
-      *transformed_point = gfx::ConvertPointToDIP(
-          device_scale_factor, gfx::PointF(target.location_in_target));
+      *transformed_point = gfx::ConvertPointToDIP(device_scale_factor,
+                                                  target.location_in_target);
     } else {
       *transformed_point = point;
     }
     if (target.flags & viz::mojom::kHitTestAsk)
       query_renderer = true;
   } else {
-    // Short circuit if owner_map has only one RenderWidgetHostView, no need for
-    // hit testing.
-    if (owner_map_.size() <= 1) {
-      *transformed_point = point;
-      return {root_view, false, *transformed_point};
-    }
-
     // The hittest delegate is used to reject hittesting quads based on extra
     // hittesting data send by the renderer.
     HittestDelegate delegate(hittest_data_);

content/browser/site_per_process_hit_test_browsertest.cc

@@ -3142,10 +3142,6 @@ IN_PROC_BROWSER_TEST_P(SitePerProcessGestureHitTestBrowserTest,
 #endif
 IN_PROC_BROWSER_TEST_P(SitePerProcessNonIntegerScaleFactorHitTestBrowserTest,
                        MAYBE_MouseClickWithNonIntegerScaleFactor) {
-  // TODO(riajiang): Update HitTestQuery to take in floating point.
-  if (GetParam())
-    return;
-
   GURL initial_url(embedded_test_server()->GetURL("a.com", "/title1.html"));
   EXPECT_TRUE(NavigateToURL(shell(), initial_url));
 
@@ -3184,9 +3180,17 @@ IN_PROC_BROWSER_TEST_P(SitePerProcessNonIntegerScaleFactorHitTestBrowserTest,
   router->RouteMouseEvent(rwhv, &mouse_event, ui::LatencyInfo());
 
   EXPECT_TRUE(event_monitor.EventWasReceived());
-  EXPECT_EQ(mouse_down_coords,
-            gfx::Point(event_monitor.event().PositionInWidget().x,
-                       event_monitor.event().PositionInWidget().y));
+  EXPECT_EQ(mouse_down_coords.x(), event_monitor.event().PositionInWidget().x);
+  // The transform from browser to renderer is (2, 35) in DIP. When we
+  // scale that to pixels, it's (3, 53). Note that 35 * 1.5 should be 52.5,
+  // so we already lost precision there in the transform from draw quad.
+  EXPECT_NEAR(mouse_down_coords.y(), event_monitor.event().PositionInWidget().y,
+              1);
+}
+
+IN_PROC_BROWSER_TEST_P(SitePerProcessNonIntegerScaleFactorHitTestBrowserTest,
+                       NestedSurfaceHitTestTest) {
+  NestedSurfaceHitTestTestHelper(shell(), embedded_test_server());
 }
 
 // Verify InputTargetClient works within an OOPIF process.

services/ui/ws/event_targeter.cc

@@ -59,7 +59,7 @@ void EventTargeter::FindTargetForLocationNow(
             event_location.display_id);
     if (hit_test_query) {
       viz::Target target = hit_test_query->FindTargetForLocation(
-          event_source, gfx::ToFlooredPoint(event_location.raw_location));
+          event_source, event_location.raw_location);
       if (target.frame_sink_id.is_valid()) {
         ServerWindow* target_window =
             event_targeter_delegate_->GetWindowFromFrameSinkId(

testing/buildbot/filters/mojo.fyi.viz.content_browsertests.filter

@@ -56,6 +56,7 @@
 -SitePerProcessHitTestBrowserTest.SubframeTouchEventRouting*
 -SitePerProcessHitTestBrowserTest.SurfaceHitTestPointerEventsNone*
 -SitePerProcessHitTestBrowserTest.SurfaceHitTestTest*
+-SitePerProcessNonIntegerScaleFactorHitTestBrowserTest.NestedSurfaceHitTestTest*
 
 # Copy Surface timing out http://crbug.com/785257
 -GLAndSoftwareCompositing/CompositingRenderWidgetHostViewBrowserTest.*

testing/buildbot/filters/viz.content_browsertests.filter

@@ -60,6 +60,7 @@
 -SitePerProcessHitTestBrowserTest.SubframeTouchEventRouting*
 -SitePerProcessHitTestBrowserTest.SurfaceHitTestPointerEventsNone*
 -SitePerProcessHitTestBrowserTest.SurfaceHitTestTest*
+-SitePerProcessNonIntegerScaleFactorHitTestBrowserTest.NestedSurfaceHitTestTest*
 
 # Copy Surface timing out http://crbug.com/785257
 -GLAndSoftwareCompositing/CompositingRenderWidgetHostViewBrowserTest.*