Update touch selection to only modify one selection point at a time.

content/browser/renderer_host/input/touch_selection_controller.cc

 // Copyright 2014 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/input/touch_selection_controller.h"
 
 #include "base/auto_reset.h"
 #include "base/logging.h"
 #include "third_party/WebKit/public/web/WebInputEvent.h"
 
@@ skipping 191 matching lines @@
 
   return false;
 }
 
 void TouchSelectionController::OnHandleDragBegin(const TouchHandle& handle) {
   if (&handle == insertion_handle_.get()) {
     client_->OnSelectionEvent(INSERTION_DRAG_STARTED, handle.position());
     return;
   }
 
+  gfx::PointF fixed_handle_position;
   if (&handle == start_selection_handle_.get()) {
-    fixed_handle_position_ =
+    fixed_handle_position =
         end_selection_handle_->position() + GetEndLineOffset();
   } else {
-    fixed_handle_position_ =
+    fixed_handle_position =
         start_selection_handle_->position() + GetStartLineOffset();
   }
+
+  // When moving the handle we want to move only the extent point. Before doing
+  // so we must make sure that the base point is set correctly.
+  gfx::Vector2dF line_offset = &handle == end_selection_handle_.get()

[jdduke (slow), 2014/10/15 15:46:07]

This could might be more simply folded into the above if statements, so you only
have a single branch point, e.g.,

gfx::PointF base, extent;
if (&handle == start_selection_handle_.get()) {
  base = end_selection_handle_->position() + GetEndLineOffset();
  extent = start_selection_handle_->position() + GetStartLineOffset();
} ...

[christiank, 2014/10/17 14:33:05]

Good idea!

+                                   ? GetStartLineOffset()
+                                   : GetEndLineOffset();
+  gfx::PointF handle_position = handle.position() + line_offset;
+  client_->SelectBetweenCoordinates(fixed_handle_position, handle_position);

[christiank, 2014/10/15 09:55:03]

Instead of adding a new code path for swapping the base and extent I thought I
could as well use SelectBetweenCoordinates to set the desired base and extent.
SelectBetweenCoordinates calls its parameters start and end, however they
correspond to base and extent when ultimately reaching
WebLocalFrameImpl::selectRange.

Not sure what you think about this the Jared?

I think setting both points in one message is more elegant than issuing a
"switch base and extent" message. An option could be to add a new code path that
makes it more explicit that we're setting the base and extent, not start and
end.

[jdduke (slow), 2014/10/15 15:46:07]

Ah, I think what you have is reasonable, maybe we just need to update the
start/end names in the SelectRange pipeline to read base/extent? Then I think
the intent is a bit more clear?

[christiank, 2014/10/17 14:33:05]

I agree, I'll update the names.

+
   client_->OnSelectionEvent(SELECTION_DRAG_STARTED, handle.position());
 }
 
 void TouchSelectionController::OnHandleDragUpdate(const TouchHandle& handle,
                                                   const gfx::PointF& position) {
   // As the position corresponds to the bottom left point of the selection
   // bound, offset it by half the corresponding line height.
   gfx::Vector2dF line_offset = &handle == end_selection_handle_.get()
                                    ? GetStartLineOffset()
                                    : GetEndLineOffset();
   gfx::PointF line_position = position + line_offset;
   if (&handle == insertion_handle_.get()) {
     client_->MoveCaret(line_position);
   } else {
-    client_->SelectBetweenCoordinates(fixed_handle_position_, line_position);
+    client_->MoveSelectionExtent(line_position);

[jdduke (slow), 2014/10/15 15:46:07]

Could add a test (or update an existing test) in
touch_selection_controller_unittest.cc to track the SelectRange +
MoveSelectionExtent flow?

[christiank, 2014/10/17 14:33:05]

Sure, will do!

   }
 }
 
 void TouchSelectionController::OnHandleDragEnd(const TouchHandle& handle) {
   if (&handle != insertion_handle_.get())
     client_->OnSelectionEvent(SELECTION_DRAG_STOPPED, handle.position());
 }
 
 void TouchSelectionController::OnHandleTapped(const TouchHandle& handle) {
   if (insertion_handle_ && &handle == insertion_handle_.get())
@@ skipping 164 matching lines @@
 }
 
 TouchHandle::AnimationStyle TouchSelectionController::GetAnimationStyle(
     bool was_active) const {
   return was_active && client_->SupportsAnimation()
              ? TouchHandle::ANIMATION_SMOOTH
              : TouchHandle::ANIMATION_NONE;
 }
 
 }  // namespace content