Update TextSelection for non-user initiated events

content/renderer/render_frame_impl.cc

 // Copyright 2013 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/renderer/render_frame_impl.h"
 
 #include <map>
 #include <string>
 #include <utility>
 #include <vector>
@@ skipping 1363 matching lines @@
 #if defined(OS_MACOSX) || defined(USE_AURA)
   GetRenderWidget()->UpdateCompositionInfo(
       false /* not an immediate request */);
 #endif
 }
 
 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(
     PepperPluginInstanceImpl* plugin) {
   GURL active_url;
   if (render_view()->webview())
     active_url = render_view()->GetURLForGraphicsContext3D();
 
   // Synchronous IPC to obtain a routing id for the fullscreen widget.
   int32_t fullscreen_widget_routing_id = MSG_ROUTING_NONE;
@@ skipping 569 matching lines @@
   base::AutoReset<bool> handling_select_range(&handling_select_range_, true);
   frame_->MoveRangeSelectionExtent(
       render_view_->ConvertWindowPointToViewport(point));
 }
 
 void RenderFrameImpl::OnReplace(const base::string16& text) {
   if (!frame_->HasSelection())
     frame_->SelectWordAroundCaret();
 
   frame_->ReplaceSelection(WebString::FromUTF16(text));
-  SyncSelectionIfRequired();
+  SyncSelectionIfRequired(true);
 }
 
 void RenderFrameImpl::OnReplaceMisspelling(const base::string16& text) {
   if (!frame_->HasSelection())
     return;
 
   frame_->ReplaceMisspelledRange(WebString::FromUTF16(text));
 }
 
 void RenderFrameImpl::OnCopyImageAt(int x, int y) {
@@ skipping 660 matching lines @@
 void RenderFrameImpl::AttachGuest(int element_instance_id) {
   BrowserPluginManager::Get()->Attach(element_instance_id);
 }
 
 void RenderFrameImpl::DetachGuest(int element_instance_id) {
   BrowserPluginManager::Get()->Detach(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(
     v8::Isolate* isolate,
     v8::Local<v8::Context> context) {
   gin::ModuleRegistry* registry = gin::ModuleRegistry::From(context);
   if (registry->available_modules().count(mojo::edk::js::Core::kModuleName))
     return;
 
   v8::HandleScope handle_scope(isolate);
@@ skipping 1504 matching lines @@
 blink::WebEffectiveConnectionType
 RenderFrameImpl::GetEffectiveConnectionType() {
   return effective_connection_type_;
 }
 
 void RenderFrameImpl::AbortClientNavigation() {
   Send(new FrameHostMsg_AbortNavigation(routing_id_));
 }
 
 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();
 
   // UpdateTextInputState should be called before SyncSelectionIfRequired.
   // UpdateTextInputState may send TextInputStateChanged to notify the focus
   // was changed, and SyncSelectionIfRequired may send SelectionChanged
   // to notify the selection was changed.  Focus change should be notified
   // before selection change.
   GetRenderWidget()->UpdateTextInputState();
-  SyncSelectionIfRequired();
+  SyncSelectionIfRequired(user_initiated);
 }
 
 bool RenderFrameImpl::HandleCurrentKeyboardEvent() {
   bool did_execute_command = false;
   for (auto command : GetRenderWidget()->edit_commands()) {
     // In gtk and cocoa, it's possible to bind multiple edit commands to one
     // key (but it's the exception). Once one edit command is not executed, it
     // seems safest to not execute the rest.
     if (!frame_->ExecuteCommand(blink::WebString::FromUTF8(command.name),
                                 blink::WebString::FromUTF8(command.value)))
@@ skipping 2078 matching lines @@
   }
 }
 
 void RenderFrameImpl::UpdateEncoding(WebFrame* frame,
                                      const std::string& encoding_name) {
   // Only update main frame's encoding_name.
   if (!frame->Parent())
     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);
     offset = 0;  // Pepper API does not support offset reporting.
     // TODO(kinaba): cut as needed.
   } else
 #endif
   {
     WebRange selection =
         GetRenderWidget()->GetWebWidget()->CaretOrSelectionRange();
-    if (selection.IsNull())
-      return;
+    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());
 
-    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 (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());
+      }
     }
   }
 
   // TODO(dglazkov): Investigate if and why this would be happening,
   // and resolve this. We shouldn't be carrying selection text here.
   // http://crbug.com/632920.
   // Sometimes we get repeated didChangeSelection calls from webkit when
   // the selection hasn't actually changed. We don't want to report these
   // because it will cause us to continually claim the X clipboard.
   if (selection_text_offset_ != offset ||
       selection_range_ != range ||
       selection_text_ != text) {
     selection_text_ = text;
     selection_text_offset_ = offset;
     selection_range_ = range;
-    SetSelectedText(text, offset, range);
+    SetSelectedText(text, offset, range, user_initiated);
   }
   GetRenderWidget()->UpdateSelectionBounds();
 }
 
 void RenderFrameImpl::InitializeUserMediaClient() {
   RenderThreadImpl* render_thread = RenderThreadImpl::current();
   if (!render_thread)  // Will be NULL during unit tests.
     return;
 
 #if BUILDFLAG(ENABLE_WEBRTC)
@@ skipping 709 matching lines @@
       policy(info.default_policy),
       replaces_current_history_item(info.replaces_current_history_item),
       history_navigation_in_new_child_frame(
           info.is_history_navigation_in_new_child_frame),
       client_redirect(info.is_client_redirect),
       cache_disabled(info.is_cache_disabled),
       form(info.form),
       source_location(info.source_location) {}
 
 }  // namespace content