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 ecfcc9ca472deff504ac837d80ceea708b2370b3..33be14ca4c27111530d78c9a49a893de5245ead9 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"
 #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;
 using content::RenderWidgetHostImpl;
 using content::RenderWidgetHostViewMac;
 using content::RenderWidgetHostViewMacEditCommandHelper;
@@ -553,6 +555,20 @@ RenderWidgetHostImpl* RenderWidgetHostViewMac::GetActiveWidget() {
                                : nullptr;
 }
 
+RenderWidgetHost* RenderWidgetHostViewMac::GetRenderWidgetHostAtPoint(

[Charlie Reis, 2016/08/31 22:50:03]

Why not RenderWidgetHostImpl, as above?  We're within content, so might as well
avoid a future need for static casts.

[EhsanK, 2016/09/01 21:58:36]

I guess I just put what I needed. But I agree with the future use argument.
Thanks! Done!

+    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()
+      ->GetRenderWidgetHostAtPos(this, point, transformed_point);
+}
 ///////////////////////////////////////////////////////////////////////////////
 // RenderWidgetHostViewMac, RenderWidgetHostView implementation:
 
@@ -2282,15 +2298,26 @@ void RenderWidgetHostViewMac::OnDisplayMetricsChanged(
 }
 
 - (void)showLookUpDictionaryOverlayAtPoint:(NSPoint)point {
+  gfx::Point rootPoint(point.x, NSHeight([self frame]) - point.y);
+  gfx::Point transformedPoint;
+  RenderWidgetHost* widgetHost =
+      renderWidgetHostView_->GetRenderWidgetHostAtPoint(rootPoint,
+                                                        &transformedPoint);
+  if (!widgetHost)
+    return;
+
+  gfx::Rect root_box = renderWidgetHostView_->GetViewBounds();
+  gfx::Rect view_box = widgetHost->GetView()->GetViewBounds();
   TextInputClientMac::GetInstance()->GetStringAtPoint(

[erikchen, 2016/09/01 00:04:29]

It looks like if this method is called twice in quick succession (which looks
possible, since this is triggered by a cocoa event), then TextInputClientMac
will use the second callback with the first response from the renderer. Seems
like a bug? [I know that it was already present, and not related to your
changes. Even if you don't fix it, could you add a comment and file a bug?]

[EhsanK, 2016/09/01 21:58:37]

We actually fire a DCHECK in that scenario. I added some comments below but I
think it might be wise to have a mechanism of tracking callbacks per RWH (i.e.,
map<process_id, map<routing_id, callback>> or somehing).

I am leaving a TODO in the TextInputClientMac class for this purpose (also filed
a bug).

-      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();
         [self showLookUpDictionaryOverlayInternal:string

[erikchen, 2016/09/01 00:04:28]

Is it ever possible for this callback to not be invoked (e.g. renderer process
dies at inopportune time?) If so, it looks like this block will be leaked, which
will leak a strong reference to "self", which could cause a retain cycle.

Perhaps this should be a weak reference using __weak
(RenderWidgetHostViewCocoa*) weakSelf = self?

[EhsanK, 2016/09/01 21:58:36]

Yes it is - as far as I can tell - possible to have this callback not invoked.
We only clear the call back on a new request (which we also DCHECK if there is
an existing one), or when we receive the IPC back. There is also no 
logic what so ever to clear the callback in TextInputClientMac when a RWHVCocoa
is going away or something. 

But I am not sure if there will be a retain cycle as the RWHVCocoa is not
keeping this callback object (TextInputClientMac is which is a singleton). 

But that being said, the DCHECK inside TextInputClientMac for an existing
handler when we make a new request (check fail if there is an existing one). I
am not quite convinced with the design and I think it might need to changes. To
crash the DCHECK all you need to do is to search for a word in a janky page and
then switch tabs and search for another word. This issue is too easily
reproducible IMO for a DCHECK.

Maybe we need a map<RWH, callback> and clear those callbacks when deemed
necessary? 

[erikchen, 2016/09/02 01:10:39]

That would be good. Doesn't have to be in this CL, since the problem was already
present. Note that a DCHECK should never trigger. If it triggers, it means
something is wrong with the code (or dcheck). See
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

                                     baselinePoint:baselinePoint
                                        targetView:self];
-      }
-  );
+      });
 }
 
 // This is invoked only on 10.8 or newer when the user taps a word using
@@ -2806,10 +2833,13 @@ extern NSString *NSTextInputReplacementRangeAttributeName;
   thePoint = [self convertPoint:thePoint fromView:nil];
   thePoint.y = NSHeight([self frame]) - thePoint.y;
 
+  gfx::Point transformedPoint;
+  RenderWidgetHost* widgetHost =
+      renderWidgetHostView_->GetRenderWidgetHostAtPoint(
+          gfx::Point(thePoint.x, thePoint.y), &transformedPoint);

[erikchen, 2016/09/01 00:04:29]

Doesn't this need a nullptr check?

[EhsanK, 2016/09/01 21:58:36]

There is two cases that we might dereference a null here and inside the method
GetRenderWidgetHostAtPoint().

1) If RenderWidgetHostInputEventRouter returns null for the WidgetAtPoint, which
I think is unlikely. wjmaclean@ might have a better answer to that.

2) If we destroy the RenderWidgetHost for a child frame, the
RenderWidgetHostViewChildFrame is not destroyed immediately, which will still be
registered as a viable view inside RenderWidgetHostInputEventRouter. So we might
endup with a valid pointer to RWHVCF when we call
RenderWidgetHostInputEventRouter::FindEventTarget but then the view will have a
null RenderWidgetHost.

Separate note: while investigating this I noticed NSTextInput is deprecated past
10.6. (I guess bug 479010?). There are several methods including this one here
to remove. Just curious if there is any progress/plans towards that.

[erikchen, 2016/09/02 01:10:39]

Thanks for looking into this. With regards to NSTextInput
deprecation...eventually, hopefully? Unfortunately it's not a high priority
right now.

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