Implement Mac Pop-up Dictionary for OOPIF.

content/browser/renderer_host/render_widget_host_view_mac.mm

Index: content/browser/renderer_host/render_widget_host_view_mac.mm
diff --git a/content/browser/renderer_host/render_widget_host_view_mac.mm b/content/browser/renderer_host/render_widget_host_view_mac.mm
index f918d1320043865700e576de13764d527da4925b..9880d16d8a2e2cd90019c4b36252092c0db8837f 100644
--- a/content/browser/renderer_host/render_widget_host_view_mac.mm
+++ b/content/browser/renderer_host/render_widget_host_view_mac.mm
@@ -60,6 +60,7 @@
 #include "content/public/browser/browser_plugin_guest_manager.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/native_web_keyboard_event.h"
+#include "content/public/browser/render_widget_host.h"

[Charlie Reis, 2016/09/02 21:57:47]

Stale?  (Do we need the impl file?)

[EhsanK, 2016/09/08 17:10:40]

impl file is already included on line 46. I think we should keep this as IWYU
rule. But I am not using this in this patch. But generally this file makes
references to RenderWidgetHost* so I am not quite sure if we should remove it.
If so I will remove this along with all the forward decs in this file and the .h
file.

[Charlie Reis, 2016/09/09 22:23:32]

Ah, I missed the other uses of RenderWidgetHost in this file.  This can stay.

 #include "content/public/browser/render_widget_host_view_frame_subscriber.h"
 #import "content/public/browser/render_widget_host_view_mac_delegate.h"
 #include "content/public/browser/web_contents.h"
@@ -96,6 +97,7 @@ using content::NativeWebKeyboardEvent;
 using content::RenderFrameHost;
 using content::RenderViewHost;
 using content::RenderViewHostImpl;
+using content::RenderWidgetHost;

[Charlie Reis, 2016/09/02 21:57:47]

Stale?

[EhsanK, 2016/09/08 17:10:40]

Same discussion as the .h file. For the override
RenderWidgetHostViewMac::GetRenderWidgetHost() we are returning a
RenderWidgetHost*.

[Charlie Reis, 2016/09/09 22:23:32]

I don't think you need this.  This file is already in content/, so it's a little
weird that we have these "using content::" declarations at all.  I think they're
just for the anonymous namespaces at the top of the file, which don't appear to
include any RenderWidgetHost references.

[EhsanK, 2016/09/13 18:52:15]

Acknowledged.

 using content::RenderWidgetHostImpl;
 using content::RenderWidgetHostViewMac;
 using content::RenderWidgetHostViewMacEditCommandHelper;
@@ -553,6 +555,20 @@ RenderWidgetHostImpl* RenderWidgetHostViewMac::GetActiveWidget() {
                                : nullptr;
 }
 
+RenderWidgetHostImpl* RenderWidgetHostViewMac::GetRenderWidgetHostAtPoint(
+    const gfx::Point& point,
+    gfx::Point* transformed_point) {
+  if (!render_widget_host_)
+    return nullptr;
+
+  if (!render_widget_host_->delegate() ||
+      !render_widget_host_->delegate()->GetInputEventRouter())
+    return render_widget_host_;
+
+  return render_widget_host_->delegate()
+      ->GetInputEventRouter()
+      ->GetRenderWidgetHostAtPoint(this, point, transformed_point);
+}
 ///////////////////////////////////////////////////////////////////////////////
 // RenderWidgetHostViewMac, RenderWidgetHostView implementation:
 
@@ -2286,15 +2302,27 @@ void RenderWidgetHostViewMac::OnDisplayMetricsChanged(
 }
 
 - (void)showLookUpDictionaryOverlayAtPoint:(NSPoint)point {
+  gfx::Point rootPoint(point.x, NSHeight([self frame]) - point.y);
+  gfx::Point transformedPoint;
+  RenderWidgetHost* widgetHost =

[Charlie Reis, 2016/09/02 21:57:47]

RenderWidgetHostImpl

[EhsanK, 2016/09/08 17:10:40]

Done.

+      renderWidgetHostView_->GetRenderWidgetHostAtPoint(rootPoint,
+                                                        &transformedPoint);
+  if (!widgetHost)
+    return;
+
+  gfx::Rect root_box = renderWidgetHostView_->GetViewBounds();
+  gfx::Rect view_box = widgetHost->GetView()->GetViewBounds();
+
   TextInputClientMac::GetInstance()->GetStringAtPoint(
-      renderWidgetHostView_->render_widget_host_,
-      gfx::Point(point.x, NSHeight([self frame]) - point.y),
+      widgetHost, transformedPoint,
       ^(NSAttributedString* string, NSPoint baselinePoint) {
+        baselinePoint.x += view_box.origin().x() - root_box.origin().x();
+        baselinePoint.y +=
+            root_box.bottom_left().y() - view_box.bottom_left().y();

[kenrb, 2016/09/06 18:14:48]

I don't really understand why this point gets shifted, and in particular why
does the code shift a point headed to the top-level frame, even though
previously it didn't do that?

[EhsanK, 2016/09/08 17:10:40]

The return coordinate is:
1) With respect to topmost local frame (I don't think it is always local root?)
2) inverted y-axis.

So when it comes back for a child frame we have to account for these things.
Because of the inverted y-axis
issue we might not be able to use RWHV::TransformPointToRootCoordSpace.
Furthermore, the callback here
is not on UI (I assume IO). So if we want to use that method we have to thread
hop.
As I understand, we get the RWH at a given point on line 2309 and at the same
time transform the point in the
root view to the RWH coordinates (right?). Then we technically transform the
point before sending the IPC and after we get the response.

[kenrb, 2016/09/12 17:28:13]

I am surprised to hear about a threading problem, but the point should really be
transformed here. On line 2843 you are transforming the original input point to
the view's coordinate space, and then when Blink returns a point you are doing a
simple translation to return it to the root's space, which isn't necessarily the
inverse (though usually it is). The compositor should internally handle the
inverted y-axis.

If coordinates being received are in the space of the top-most local frame that
is not the local frame root, then that would be a bug. I'll comment on that in
the other file.
The part here that I don't understand is that it looks like you are even
shifting a point that went to a root frame, which wouldn't have had any
transformation done to it before being sent to the renderer process. This isn't
important if you change this to use TransformPointToRootCoordSpace, which is
becomes a nop for the root frame.

[EhsanK, 2016/09/13 18:52:15]

Following our offline conversation, I am leaving a comment here explaining why
we manually subtract the offset in view bounds rather than using
TransformPointToRootCoordSpace which is due to the unfortunate fact that right
now the IPCs from renderer for this task are handled on IO and the callback is
also on IO.

         [self showLookUpDictionaryOverlayInternal:string
                                     baselinePoint:baselinePoint
                                        targetView:self];
-      }
-  );
+      });
 }
 
 // This is invoked only on 10.8 or newer when the user taps a word using
@@ -2810,10 +2838,16 @@ extern NSString *NSTextInputReplacementRangeAttributeName;
   thePoint = [self convertPoint:thePoint fromView:nil];
   thePoint.y = NSHeight([self frame]) - thePoint.y;
 
+  gfx::Point transformedPoint;
+  RenderWidgetHost* widgetHost =

[Charlie Reis, 2016/09/02 21:57:47]

RenderWidgetHostImpl

[EhsanK, 2016/09/08 17:10:40]

Done.

+      renderWidgetHostView_->GetRenderWidgetHostAtPoint(
+          gfx::Point(thePoint.x, thePoint.y), &transformedPoint);
+  if (!widgetHost)
+    return NSNotFound;
+
   NSUInteger index =
       TextInputClientMac::GetInstance()->GetCharacterIndexAtPoint(
-          renderWidgetHostView_->render_widget_host_,
-          gfx::Point(thePoint.x, thePoint.y));
+          widgetHost, transformedPoint);
   return index;
 }