Track text selection on the browser side (Mac)

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 9ed2ca7aa7dc5d3a31afe4ab72d0e02bb451f421..813ca0b68b58c071483e8b835fc9bb79c8d18b12 100644
--- a/content/browser/renderer_host/render_widget_host_view_mac.mm
+++ b/content/browser/renderer_host/render_widget_host_view_mac.mm
@@ -920,6 +920,52 @@ void RenderWidgetHostViewMac::OnImeCancelComposition(
   [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) {
@@ -1036,33 +1082,6 @@ void RenderWidgetHostViewMac::StopSpeaking() {
 // 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) {