Update TextSelection for non-user initiated events

content/renderer/render_frame_impl.cc

Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index f2a6efea6851fb9834d3a4fe6cac839b08571845..c14636c1c275bac2a93980ac8ffbc5e949de380b 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -1381,7 +1381,7 @@ void RenderFrameImpl::PepperSelectionChanged(
     PepperPluginInstanceImpl* instance) {
   if (instance != focused_pepper_plugin_)
     return;
-  SyncSelectionIfRequired();
+  SyncSelectionIfRequired(true);

[nasko, 2017/05/04 16:10:24]

Can we put some small comments in cases where we pass in a hardcoded value as to
why that's correct?

[Peter Varga, 2017/05/04 16:33:45]

I can't say this is correct. The only reason that I have for these hard coded
values is that this was the original behavior.

 }
 
 RenderWidgetFullscreenPepper* RenderFrameImpl::CreatePepperFullscreenContainer(
@@ -1971,7 +1971,7 @@ void RenderFrameImpl::OnReplace(const base::string16& text) {
     frame_->SelectWordAroundCaret();
 
   frame_->ReplaceSelection(WebString::FromUTF16(text));
-  SyncSelectionIfRequired();
+  SyncSelectionIfRequired(true);
 }
 
 void RenderFrameImpl::OnReplaceMisspelling(const base::string16& text) {
@@ -2652,9 +2652,11 @@ void RenderFrameImpl::DetachGuest(int element_instance_id) {
 
 void RenderFrameImpl::SetSelectedText(const base::string16& selection_text,
                                       size_t offset,
-                                      const gfx::Range& range) {
+                                      const gfx::Range& range,
+                                      bool user_initiated) {
   Send(new FrameHostMsg_SelectionChanged(routing_id_, selection_text,
-                                         static_cast<uint32_t>(offset), range));
+                                         static_cast<uint32_t>(offset), range,
+                                         user_initiated));
 }
 
 void RenderFrameImpl::EnsureMojoBuiltinsAreAvailable(
@@ -4179,9 +4181,9 @@ void RenderFrameImpl::AbortClientNavigation() {
 }
 
 void RenderFrameImpl::DidChangeSelection(bool is_empty_selection) {
-  if (!GetRenderWidget()->input_handler().handling_input_event() &&
-      !handling_select_range_)
-    return;
+  bool user_initiated =
+      GetRenderWidget()->input_handler().handling_input_event() ||
+      handling_select_range_;
 
   if (is_empty_selection)
     selection_text_.clear();
@@ -4192,7 +4194,7 @@ void RenderFrameImpl::DidChangeSelection(bool is_empty_selection) {
   // to notify the selection was changed.  Focus change should be notified
   // before selection change.
   GetRenderWidget()->UpdateTextInputState();
-  SyncSelectionIfRequired();
+  SyncSelectionIfRequired(user_initiated);
 }
 
 bool RenderFrameImpl::HandleCurrentKeyboardEvent() {
@@ -6291,10 +6293,10 @@ void RenderFrameImpl::UpdateEncoding(WebFrame* frame,
     Send(new FrameHostMsg_UpdateEncoding(routing_id_, encoding_name));
 }
 
-void RenderFrameImpl::SyncSelectionIfRequired() {
+void RenderFrameImpl::SyncSelectionIfRequired(bool user_initiated) {
   base::string16 text;
-  size_t offset;
-  gfx::Range range;
+  size_t offset = 0;
+  gfx::Range range = gfx::Range::InvalidRange();
 #if BUILDFLAG(ENABLE_PLUGINS)
   if (focused_pepper_plugin_) {
     focused_pepper_plugin_->GetSurroundingText(&text, &range);
@@ -6305,32 +6307,31 @@ void RenderFrameImpl::SyncSelectionIfRequired() {
   {
     WebRange selection =
         GetRenderWidget()->GetWebWidget()->CaretOrSelectionRange();
-    if (selection.IsNull())
-      return;
-
-    range = gfx::Range(selection.StartOffset(), selection.EndOffset());
-
-    if (frame_->GetInputMethodController()->TextInputType() !=
-        blink::kWebTextInputTypeNone) {
-      // If current focused element is editable, we will send 100 more chars
-      // before and after selection. It is for input method surrounding text
-      // feature.
-      if (selection.StartOffset() > kExtraCharsBeforeAndAfterSelection)
-        offset = selection.StartOffset() - kExtraCharsBeforeAndAfterSelection;
-      else
-        offset = 0;
-      size_t length =
-          selection.EndOffset() - offset + kExtraCharsBeforeAndAfterSelection;
-      text = frame_->RangeAsText(WebRange(offset, length)).Utf16();
-    } else {
-      offset = selection.StartOffset();
-      text = frame_->SelectionAsText().Utf16();
-      // http://crbug.com/101435
-      // In some case, frame->selectionAsText() returned text's length is not
-      // equal to the length returned from
-      // GetWebWidget()->caretOrSelectionRange().
-      // So we have to set the range according to text.length().
-      range.set_end(range.start() + text.length());
+    if (!selection.IsNull()) {

[nasko, 2017/05/04 16:10:24]

This looks like a change in behavior as the block of code following the current
scope will now be executed if selection is null. Is this intentional? The CL
description says that it only exposes the user_initiated boolean.

[Peter Varga, 2017/05/04 16:33:45]

Yes it is intentional. I haven't found any test related to this. I've found that
clearing the selection from JavaScript doesn't update the selection text. In the
new test I tried to cover this for both the user and non-user initiated
selection clear. I haven't noticed duplicate SelectionChanged call so I don't
know why the SetSelectedText was guarded.

[Peter Varga, 2017/05/05 13:09:11]

While working on the new test I've found a case when this produce an unnecessary
SetSelectedText call. It seems I have to keep the early return.

+      range = gfx::Range(selection.StartOffset(), selection.EndOffset());
+
+      if (frame_->GetInputMethodController()->TextInputType() !=
+          blink::kWebTextInputTypeNone) {
+        // If current focused element is editable, we will send 100 more chars
+        // before and after selection. It is for input method surrounding text
+        // feature.
+        if (selection.StartOffset() > kExtraCharsBeforeAndAfterSelection)
+          offset = selection.StartOffset() - kExtraCharsBeforeAndAfterSelection;
+        else
+          offset = 0;
+        size_t length =
+            selection.EndOffset() - offset + kExtraCharsBeforeAndAfterSelection;
+        text = frame_->RangeAsText(WebRange(offset, length)).Utf16();
+      } else {
+        offset = selection.StartOffset();
+        text = frame_->SelectionAsText().Utf16();
+        // http://crbug.com/101435
+        // In some case, frame->selectionAsText() returned text's length is not
+        // equal to the length returned from
+        // GetWebWidget()->caretOrSelectionRange().
+        // So we have to set the range according to text.length().
+        range.set_end(range.start() + text.length());
+      }
     }
   }
 
@@ -6346,7 +6347,7 @@ void RenderFrameImpl::SyncSelectionIfRequired() {
     selection_text_ = text;
     selection_text_offset_ = offset;
     selection_range_ = range;
-    SetSelectedText(text, offset, range);
+    SetSelectedText(text, offset, range, user_initiated);
   }
   GetRenderWidget()->UpdateSelectionBounds();
 }