Enable browser process hit testing on Android

content/browser/renderer_host/render_widget_host_view_android.cc

Index: content/browser/renderer_host/render_widget_host_view_android.cc
diff --git a/content/browser/renderer_host/render_widget_host_view_android.cc b/content/browser/renderer_host/render_widget_host_view_android.cc
index 53c758f11f2760db6d1ec2218b6fdc475ba4ecbb..247cdd6d6948d030549ee9f298738c6e59d03262 100644
--- a/content/browser/renderer_host/render_widget_host_view_android.cc
+++ b/content/browser/renderer_host/render_widget_host_view_android.cc
@@ -34,6 +34,7 @@
 #include "cc/resources/single_release_callback.h"
 #include "cc/surfaces/surface.h"
 #include "cc/surfaces/surface_factory.h"
+#include "cc/surfaces/surface_hittest.h"
 #include "cc/surfaces/surface_manager.h"
 #include "cc/trees/layer_tree_host.h"
 #include "components/display_compositor/gl_helper.h"
@@ -42,6 +43,7 @@
 #include "content/browser/android/content_view_core_impl.h"
 #include "content/browser/android/overscroll_controller_android.h"
 #include "content/browser/android/synchronous_compositor_host.h"
+#include "content/browser/compositor/surface_utils.h"
 #include "content/browser/devtools/render_frame_devtools_agent_host.h"
 #include "content/browser/gpu/browser_gpu_channel_host_factory.h"
 #include "content/browser/gpu/compositor_util.h"
@@ -58,8 +60,10 @@
 #include "content/browser/renderer_host/render_view_host_delegate_view.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
 #include "content/browser/renderer_host/render_widget_host_impl.h"
+#include "content/browser/renderer_host/render_widget_host_input_event_router.h"
 #include "content/common/gpu_host_messages.h"
 #include "content/common/input_messages.h"
+#include "content/common/site_isolation_policy.h"
 #include "content/common/view_messages.h"
 #include "content/public/browser/android/compositor.h"
 #include "content/public/browser/android/synchronous_compositor_client.h"
@@ -458,6 +462,14 @@ RenderWidgetHostViewAndroid::RenderWidgetHostViewAndroid(
 
   if (GetTextInputManager())
     GetTextInputManager()->AddObserver(this);
+
+  // Let the page-level input event router know about our frame sink ID
+  // for surface-based hit testing.
+  if (host_->delegate() && host_->delegate()->GetInputEventRouter() &&
+      SiteIsolationPolicy::AreCrossProcessFramesPossible()) {

[alexmos, 2017/03/06 07:22:03]

Is the AreCrossProcessFramesPossible necessary here?  The equivalent in
RenderWidgetHostViewAura doesn't use it, and wouldn't ShouldRouteEvent check it
as necessary based on individual events?  Or would we not hit ShouldRouteEvent
on the Android paths?

[kenrb, 2017/03/07 19:20:19]

Done.

+    host_->delegate()->GetInputEventRouter()->AddFrameSinkIdOwner(
+        GetFrameSinkId(), this);
+  }
 }
 
 RenderWidgetHostViewAndroid::~RenderWidgetHostViewAndroid() {
@@ -796,6 +808,108 @@ void RenderWidgetHostViewAndroid::SetNeedsBeginFrames(bool needs_begin_frames) {
     ClearBeginFrameRequest(PERSISTENT_BEGIN_FRAME);
 }
 
+cc::SurfaceId RenderWidgetHostViewAndroid::SurfaceIdForTesting() const {
+  return delegated_frame_host_->SurfaceId();

[alexmos, 2017/03/06 07:22:03]

Does this need to worry about delegated_frame_host_ being null at all for any of
the new code (like some other functions in RenderWidgetHostViewAndroid)?  Looks
like its existence is gated on using_browser_compositor_, and there's a comment
in RenderWidgetHostViewAndroid::SupportsAnimation about this possibly being
false for WebView?

[kenrb, 2017/03/07 19:20:19]

Good point. I have added guards to account for that.

+}
+
+cc::FrameSinkId RenderWidgetHostViewAndroid::FrameSinkIdAtPoint(
+    cc::SurfaceHittestDelegate* delegate,
+    const gfx::Point& point,
+    gfx::Point* transformed_point) {
+  float scale_factor =
+      display::Screen::GetScreen()->GetPrimaryDisplay().device_scale_factor();
+  DCHECK_GT(scale_factor, 0);
+  // The surface hittest happens in device pixels, so we need to convert the
+  // |point| from DIPs to pixels before hittesting.

[aelias_OOO_until_Jul13, 2017/03/03 03:27:37]

Input events originate in device pixels, so this implies a round-trip from
device -> DIP -> physical, which results in off-by-one for some values.  Is
there a way we could plumb the device pixel values directly with no
transformation?  Other platforms are moving away from DIP in the browser process
over time anyway.

[kenrb, 2017/03/03 18:38:44]

This sounds like a good suggestion, but would really warrant a follow up bug
because it would have to be changed in all the RWHVs, and the callers would have
to be modified, as well as several years.

As it is, we try to avoid the off-by-one in the basic case by not modifying the
point if there is only one RenderWidgetHostView for the page (I.e. no OOPIFs
present).

+  gfx::Point point_in_pixels = gfx::ConvertPointToPixel(scale_factor, point);
+
+  cc::SurfaceId surface_id = delegated_frame_host_->SurfaceId();
+  if (surface_id.is_valid()) {
+    cc::SurfaceHittest hittest(delegate, GetSurfaceManager());
+    gfx::Transform target_transform;
+    surface_id = hittest.GetTargetSurfaceAtPoint(surface_id, point_in_pixels,
+                                                 &target_transform);
+    *transformed_point = point_in_pixels;
+    if (surface_id.is_valid())
+      target_transform.TransformPoint(transformed_point);
+    *transformed_point =
+        gfx::ConvertPointToDIP(scale_factor, *transformed_point);
+  }
+
+  // It is possible that the renderer has not yet produced a surface, in which
+  // case we return our current namespace.
+  if (!surface_id.is_valid())
+    return GetFrameSinkId();

[alexmos, 2017/03/06 07:22:03]

nit: you could move this if and return up before the if (surface_id.is_valid())
check, which would allow you to just remove the "if (surface_id.is_valid())"

[kenrb, 2017/03/07 19:20:19]

Having it here guards against the case of it becoming invalid during the hit
test on line 830 (which would mean the hit test failed for some reason).

[alexmos, 2017/03/08 19:50:03]

Acknowledged.

+  return surface_id.frame_sink_id();
+}
+
+void RenderWidgetHostViewAndroid::ProcessMouseEvent(
+    const blink::WebMouseEvent& event,
+    const ui::LatencyInfo& latency) {
+  host_->ForwardMouseEventWithLatencyInfo(event, latency);
+}
+
+void RenderWidgetHostViewAndroid::ProcessMouseWheelEvent(
+    const blink::WebMouseWheelEvent& event,
+    const ui::LatencyInfo& latency) {
+  host_->ForwardWheelEventWithLatencyInfo(event, latency);
+}
+
+void RenderWidgetHostViewAndroid::ProcessTouchEvent(
+    const blink::WebTouchEvent& event,
+    const ui::LatencyInfo& latency) {
+  host_->ForwardTouchEventWithLatencyInfo(event, latency);
+}
+
+void RenderWidgetHostViewAndroid::ProcessGestureEvent(
+    const blink::WebGestureEvent& event,
+    const ui::LatencyInfo& latency) {
+  host_->ForwardGestureEventWithLatencyInfo(event, latency);
+}
+
+bool RenderWidgetHostViewAndroid::TransformPointToLocalCoordSpace(
+    const gfx::Point& point,
+    const cc::SurfaceId& original_surface,
+    gfx::Point* transformed_point) {
+  float scale_factor =
+      display::Screen::GetScreen()->GetPrimaryDisplay().device_scale_factor();
+  DCHECK_GT(scale_factor, 0);
+  // Transformations use physical pixels rather than DIP, so conversion
+  // is necessary.
+  gfx::Point point_in_pixels = gfx::ConvertPointToPixel(scale_factor, point);
+
+  cc::SurfaceId surface_id = delegated_frame_host_->SurfaceId();
+  if (!surface_id.is_valid())
+    return false;
+
+  if (original_surface == surface_id)
+    return true;
+
+  *transformed_point = point_in_pixels;
+  cc::SurfaceHittest hittest(nullptr, GetSurfaceManager());
+  if (!hittest.TransformPointToTargetSurface(original_surface, surface_id,
+                                             transformed_point))
+    return false;
+
+  *transformed_point = gfx::ConvertPointToDIP(scale_factor, *transformed_point);
+  return true;
+}
+
+bool RenderWidgetHostViewAndroid::TransformPointToCoordSpaceForView(
+    const gfx::Point& point,
+    RenderWidgetHostViewBase* target_view,
+    gfx::Point* transformed_point) {

[alexmos, 2017/03/06 07:22:03]

In the equivalent RenderWidgetHostViewAura function, there is another check to
return true and set transformed_point to point if target_view == this.  Do we
need the same here? (I admit I don't fully understand why that check is there in
RWHVA)

[kenrb, 2017/03/07 19:20:19]

Oh yeah, that should be here. I think I added that check to RWHVA after I had
created this CL, and so neglected to propagate it. The reason is that we want to
avoid off-by-one errors from rounding if we are routing the event to its
original target.

[alexmos, 2017/03/08 19:50:03]

Do you still need to add it?  I don't see it in the latest PS.

[kenrb, 2017/03/08 21:15:28]

That's weird, but thanks for catching it. I am sure I added that,but maybe
forgot to save the file. Fixed now.

+  // In TransformPointToLocalCoordSpace() there is a Point-to-Pixel conversion,
+  // but it is not necessary here because the final target view is responsible
+  // for converting before computing the final transform.
+  cc::SurfaceId surface_id = delegated_frame_host_->SurfaceId();
+  if (!surface_id.is_valid())
+    return false;
+
+  return target_view->TransformPointToLocalCoordSpace(point, surface_id,
+                                                      transformed_point);
+}
+
 void RenderWidgetHostViewAndroid::OnStartContentIntent(
     const GURL& content_url, bool is_main_frame) {
   view_.StartContentIntent(content_url, is_main_frame);
@@ -803,7 +917,7 @@ void RenderWidgetHostViewAndroid::OnStartContentIntent(
 
 bool RenderWidgetHostViewAndroid::OnTouchEvent(
     const ui::MotionEvent& event) {
-  if (!host_)
+  if (!host_ || !host_->delegate())
     return false;
 
   ComputeEventLatencyOSTouchHistograms(event);
@@ -831,7 +945,13 @@ bool RenderWidgetHostViewAndroid::OnTouchEvent(
       event, result.moved_beyond_slop_region);
   ui::LatencyInfo latency_info(ui::SourceEventType::TOUCH);
   latency_info.AddLatencyNumber(ui::INPUT_EVENT_LATENCY_UI_COMPONENT, 0, 0);
-  host_->ForwardTouchEventWithLatencyInfo(web_event, latency_info);
+  if (host_->delegate()->GetInputEventRouter() &&
+      SiteIsolationPolicy::AreCrossProcessFramesPossible()) {

[alexmos, 2017/03/06 07:22:03]

There's a comment on RenderWidgetHostViewEventHandler::ShouldRouteEvent that
touch events should always be routed.  So is the AreCrossProcessFramesPossible
needed here? (And could this use ShouldRouteEvent?)

[kenrb, 2017/03/07 19:20:19]

I don't think that is strictly necessary because there is no BrowserPlugin on
Android, and ShouldRouteEvent() is specific to each RenderWidgetHostView
(RenderWidgetHostViewEventHandler, despite its generic name, is Aura-only).

That said, it is probably reasonable to remove the AreCrossProcessFramePossible
check and see how that goes.

[alexmos, 2017/03/08 19:50:03]

Ah, I see.  I think it's ok to keep this for now then, and try removing it in a
separate CL to make it easier to revert if something goes wrong.

+    host_->delegate()->GetInputEventRouter()->RouteTouchEvent(this, &web_event,
+                                                              latency_info);
+  } else {
+    host_->ForwardTouchEventWithLatencyInfo(web_event, latency_info);
+  }
 
   // Send a proactive BeginFrame for this vsync to reduce scroll latency for
   // scroll-inducing touch events. Note that Android's Choreographer ensures
@@ -1627,15 +1747,31 @@ void RenderWidgetHostViewAndroid::SendMouseEvent(
       action_button,
       motion_event.GetToolType(0));
 
-  if (host_)
+  if (!host_ || !host_->delegate())
+    return;
+
+  if (SiteIsolationPolicy::AreCrossProcessFramesPossible() &&
+      host_->delegate()->GetInputEventRouter()) {
+    host_->delegate()->GetInputEventRouter()->RouteMouseEvent(
+        this, &mouse_event, ui::LatencyInfo());
+  } else {
     host_->ForwardMouseEvent(mouse_event);
+  }
 }
 
 void RenderWidgetHostViewAndroid::SendMouseWheelEvent(
     const blink::WebMouseWheelEvent& event) {
-  if (host_) {
-    ui::LatencyInfo latency_info(ui::SourceEventType::WHEEL);
-    latency_info.AddLatencyNumber(ui::INPUT_EVENT_LATENCY_UI_COMPONENT, 0, 0);
+  if (!host_ || !host_->delegate())
+    return;
+
+  ui::LatencyInfo latency_info(ui::SourceEventType::WHEEL);
+  latency_info.AddLatencyNumber(ui::INPUT_EVENT_LATENCY_UI_COMPONENT, 0, 0);
+  if (SiteIsolationPolicy::AreCrossProcessFramesPossible() &&
+      host_->delegate()->GetInputEventRouter()) {
+    blink::WebMouseWheelEvent wheel_event(event);
+    host_->delegate()->GetInputEventRouter()->RouteMouseWheelEvent(
+        this, &wheel_event, latency_info);
+  } else {
     host_->ForwardWheelEventWithLatencyInfo(event, latency_info);
   }
 }
@@ -1646,9 +1782,17 @@ void RenderWidgetHostViewAndroid::SendGestureEvent(
   if (overscroll_controller_)
     overscroll_controller_->Enable();
 
-  if (host_) {
-    ui::LatencyInfo latency_info =
-        ui::WebInputEventTraits::CreateLatencyInfoForWebGestureEvent(event);
+  if (!host_ || !host_->delegate())
+    return;
+
+  ui::LatencyInfo latency_info =
+      ui::WebInputEventTraits::CreateLatencyInfoForWebGestureEvent(event);
+  if (SiteIsolationPolicy::AreCrossProcessFramesPossible() &&
+      host_->delegate()->GetInputEventRouter()) {
+    blink::WebGestureEvent gesture_event(event);
+    host_->delegate()->GetInputEventRouter()->RouteGestureEvent(
+        this, &gesture_event, latency_info);
+  } else {
     host_->ForwardGestureEventWithLatencyInfo(event, latency_info);
   }
 }