Implement Mac Pop-up Dictionary for OOPIF.

content/browser/renderer_host/render_widget_host_view_mac.mm

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/renderer_host/render_widget_host_view_mac.h"
 
 #import <objc/runtime.h>
 #include <OpenGL/gl.h>
 #include <QuartzCore/QuartzCore.h>
 #include <stdint.h>
@@ skipping 42 matching lines @@
 #include "content/common/accessibility_messages.h"
 #include "content/common/edit_command.h"
 #include "content/common/input_messages.h"
 #include "content/common/site_isolation_policy.h"
 #include "content/common/text_input_state.h"
 #include "content/common/view_messages.h"
 #include "content/public/browser/browser_context.h"
 #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"
 #include "gpu/ipc/common/gpu_messages.h"
 #include "skia/ext/platform_canvas.h"
 #include "skia/ext/skia_utils_mac.h"
 #include "third_party/WebKit/public/platform/WebScreenInfo.h"
 #include "third_party/WebKit/public/web/WebInputEvent.h"
 #import "ui/base/clipboard/clipboard_util_mac.h"
 #include "ui/base/cocoa/animation_utils.h"
@@ skipping 16 matching lines @@
 #include "ui/gl/gl_switches.h"
 
 using content::BrowserAccessibility;
 using content::BrowserAccessibilityManager;
 using content::EditCommand;
 using content::FrameTreeNode;
 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;
 using content::TextInputClientMac;
 using content::WebContents;
 using content::WebGestureEventBuilder;
 using content::WebMouseEventBuilder;
 using content::WebMouseWheelEventBuilder;
 using blink::WebInputEvent;
 using blink::WebMouseEvent;
@@ skipping 437 matching lines @@
   if (!GetActiveWidget())
     return ui::TEXT_INPUT_TYPE_NONE;
   return GetTextInputManager()->GetTextInputState()->type;
 }
 
 RenderWidgetHostImpl* RenderWidgetHostViewMac::GetActiveWidget() {
   return GetTextInputManager() ? GetTextInputManager()->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:
 
 bool RenderWidgetHostViewMac::OnMessageReceived(const IPC::Message& message) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(RenderWidgetHostViewMac, message)
     IPC_MESSAGE_HANDLER(ViewMsg_GetRenderedTextCompleted,
         OnGetRenderedTextCompleted)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
@@ skipping 1713 matching lines @@
       renderWidgetHostView_->render_widget_host_, range,
       ^(NSAttributedString* string, NSPoint baselinePoint) {
         [self showLookUpDictionaryOverlayInternal:string
                                     baselinePoint:baselinePoint
                                        targetView:targetView];
       }
   );
 }
 
 - (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
 // three fingers.
 - (void)quickLookWithEvent:(NSEvent*)event {
   NSPoint point = [self convertPoint:[event locationInWindow] fromView:nil];
   [self showLookUpDictionaryOverlayAtPoint:point];
 }
 
 // This method handles 2 different types of hardware events.
@@ skipping 495 matching lines @@
 
 - (NSUInteger)characterIndexForPoint:(NSPoint)thePoint {
   DCHECK([self window]);
   // |thePoint| is in screen coordinates, but needs to be converted to WebKit
   // coordinates (upper left origin). Scroll offsets will be taken care of in
   // the renderer.
   thePoint = ui::ConvertPointFromScreenToWindow([self window], thePoint);
   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;
 }
 
 - (NSRect)firstViewRectForCharacterRange:(NSRange)theRange
                              actualRange:(NSRangePointer)actualRange {
   NSRect rect;
   if (!renderWidgetHostView_->GetCachedFirstRectForCharacterRange(
           theRange,
           &rect,
           actualRange)) {
@@ skipping 438 matching lines @@
 
 // "-webkit-app-region: drag | no-drag" is implemented on Mac by excluding
 // regions that are not draggable. (See ControlRegionView in
 // native_app_window_cocoa.mm). This requires the render host view to be
 // draggable by default.
 - (BOOL)mouseDownCanMoveWindow {
   return YES;
 }
 
 @end