Track text selection on the browser side (Mac)

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 902 matching lines @@
 #endif
   }
 }
 
 void RenderWidgetHostViewMac::OnImeCancelComposition(
     TextInputManager* text_input_manager,
     RenderWidgetHostViewBase* updated_view) {
   [cocoa_view_ cancelComposition];
 }
 
+void RenderWidgetHostViewMac::OnTextSelectionChanged(
+    TextInputManager* text_input_manager,
+    RenderWidgetHostViewBase* updated_view) {
+  if (!GetTextInputManager())

[erikchen, 2016/08/12 19:33:43]

Can you describe a situation where this conditional can evaluate to true? Should
this be DCHECK_EQ(GetTextInputManager(), text_input_manager)?

[EhsanK, 2016/08/16 13:36:45]

Good question. The reason we have this is for future usage with <guest>-views
and nested WebContents. It is not 100% determined how they will integrate into
this model (i.e., TextInputManager per WebContents as opposed to one per
outermost WebContents and how to transition from inner to outer WebContents when
we attach WebContents). Eventually, |text_input_manager| argument might get
removed, or might still stay.

Having said all this, I think DCHECK_EQ(GetTextInputManager(),
text_input_manager) should be better since we do expect this observer call from
none other than the one belonging to |render_widget_host_->delegate()|. Also
this is an automatic check agains null-ness.

+    return;
+
+  RenderWidgetHostViewBase* focused_view = nullptr;
+  if (is_guest_view_hack_) {

[erikchen, 2016/08/12 19:33:43]

This code looks really similar to the logic in
RenderWidgetHostViewAura::OnTextSelectionChanged. Would it make sense to pull
out a common method?

[EhsanK, 2016/08/16 13:36:45]

I added this logic to obtain the focused view into the base class. Since this
relies on RenderWidgetHostView::GetRenderWidgetHost() I implemented that method
for the base class which returns nullptr. The only justification for it is that
we might head into pure virtual calls if the method is used somewhere else.

+    // We obtain the TextSelection from focused RWH which is obtained from the
+    // frame tree. BrowserPlugin-based guests' RWH is not part of the frame tree
+    // and the focused RWH will be that of the embedder which is incorrect. In
+    // this case we should use TextSelection for |this| since RWHV for guest
+    // forwards text selection information to its platform view.
+    focused_view = this;
+  } else if (render_widget_host_ && render_widget_host_->delegate()) {
+    RenderWidgetHostImpl* focused_widget =
+        render_widget_host_->delegate()->GetFocusedRenderWidgetHost(
+            render_widget_host_);
+    if (focused_widget)
+      focused_view = focused_widget->GetView();
+  }
+
+  if (!focused_view)
+    return;
+
+  base::string16 text;
+  if (GetTextInputManager()->GetSelectedText(focused_view, &text))
+    selected_text_ = base::UTF16ToUTF8(text);
+
+  const TextInputManager::TextSelection* selection =
+      GetTextInputManager()->GetTextSelection(focused_view);

[erikchen, 2016/08/12 19:33:43]

The logic for GetTextInputManager()->GetSelectedText() also requires calling
GetTextSelection(). Should GetSelectedText() be a method on
TextInputManager::TextSelection instead?

[EhsanK, 2016/08/16 13:36:45]

Acknowledged.

+  [cocoa_view_ setSelectedRange:selection->range.ToNSRange()];
+  // Updates markedRange when there is no marked text so that retrieving
+  // markedRange immediately after calling setMarkdText: returns the current
+  // caret position.
+  if (![cocoa_view_ hasMarkedText]) {
+    [cocoa_view_ setMarkedRange:selection->range.ToNSRange()];
+  }
+
+  // TODO(ekaramad): The following values are tracked by TextInputManager and
+  // should be cleaned up from this class (https://crbug.com/602427).
+  selection_text_ = selection->text;

[EhsanK, 2016/08/12 05:07:54]

These are also still used by android, so I might as well keep them here until
they are fully removed from the base class.

[erikchen, 2016/08/12 19:33:43]

Why aren't we updating the methods in render_widget_host_view_base.cc to use the
AURA path for mac instead? 

[EhsanK, 2016/08/16 13:36:45]

I am not quite clear on this comment, so I will explain what I understand wait
for your feedback.

As I understand, with this patch we are using AURA path for Mac. The call to
RenderWidgetHostViewBase::SelectionChanged will lead to a call to
TextInputManager::SelectionChanged for both Mac and Aura. Then the TIM will call
its observer (either RenderWidgetHostViewMac) to notify about a change in text
selection; which happens similarly in aura.

The only remaining issue is these member variables in the base class which we
are still using in Mac. I figured it might be better to remove them all at once
after implementing the same approach for Android.

[erikchen, 2016/08/16 16:36:22]

That's fine.

+  selection_range_ = selection->range;
+  selection_text_offset_ = selection->offset;
+}
+
 void RenderWidgetHostViewMac::ImeCompositionRangeChanged(
     const gfx::Range& range,
     const std::vector<gfx::Rect>& character_bounds) {
   // The RangeChanged message is only sent with valid values. The current
   // caret position (start == end) will be sent if there is no IME range.
   [cocoa_view_ setMarkedRange:range.ToNSRange()];
   composition_range_ = range;
   composition_bounds_ = character_bounds;
 }
 
@@ skipping 96 matching lines @@
 
 void RenderWidgetHostViewMac::StopSpeaking() {
   if ([NSApp respondsToSelector:@selector(stopSpeaking:)])
     [NSApp stopSpeaking:cocoa_view_];
 }
 
 //
 // RenderWidgetHostViewCocoa uses the stored selection text,
 // which implements NSServicesRequests protocol.
 //
-void RenderWidgetHostViewMac::SelectionChanged(const base::string16& text,
-                                               size_t offset,
-                                               const gfx::Range& range) {
-  if (range.is_empty() || text.empty()) {
-    selected_text_.clear();
-  } else {
-    size_t pos = range.GetMin() - offset;
-    size_t n = range.length();
-
-    DCHECK(pos + n <= text.length()) << "The text can not fully cover range.";
-    if (pos >= text.length()) {
-      DCHECK(false) << "The text can not cover range.";
-      return;
-    }
-    selected_text_ = base::UTF16ToUTF8(text.substr(pos, n));
-  }
-
-  [cocoa_view_ setSelectedRange:range.ToNSRange()];
-  // Updates markedRange when there is no marked text so that retrieving
-  // markedRange immediately after calling setMarkdText: returns the current
-  // caret position.
-  if (![cocoa_view_ hasMarkedText]) {
-    [cocoa_view_ setMarkedRange:range.ToNSRange()];
-  }
-
-  RenderWidgetHostViewBase::SelectionChanged(text, offset, range);
-}
 
 void RenderWidgetHostViewMac::SelectionBoundsChanged(
     const ViewHostMsg_SelectionBounds_Params& params) {
   if (params.anchor_rect == params.focus_rect)
     caret_rect_ = params.anchor_rect;
   first_selection_rect_ = params.anchor_rect;
 }
 
 void RenderWidgetHostViewMac::SetShowingContextMenu(bool showing) {
   RenderWidgetHostViewBase::SetShowingContextMenu(showing);
@@ skipping 2179 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