Consume entire touch sequence in touch selection controller

ui/touch_selection/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 "ui/touch_selection/touch_selection_controller.h"
 
 #include "base/auto_reset.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/user_metrics.h"
@@ skipping 50 matching lines @@
       start_orientation_(TouchHandleOrientation::UNDEFINED),
       end_orientation_(TouchHandleOrientation::UNDEFINED),
       active_status_(INACTIVE),
       activate_insertion_automatically_(false),
       activate_selection_automatically_(false),
       selection_empty_(false),
       selection_editable_(false),
       temporarily_hidden_(false),
       anchor_drag_to_selection_start_(false),
       longpress_drag_selector_(this),
-      selection_handle_dragged_(false) {
+      selection_handle_dragged_(false),
+      consume_touch_sequence_(false) {
   DCHECK(client_);
 }
 
 TouchSelectionController::~TouchSelectionController() {
 }
 
 void TouchSelectionController::OnSelectionBoundsChanged(
     const gfx::SelectionBound& start,
     const gfx::SelectionBound& end) {
   if (!force_next_update_ && start == start_ && end_ == end)
@@ skipping 89 matching lines @@
     DCHECK(end_selection_handle_);
     start_selection_handle_->SetViewportRect(viewport_rect);
     end_selection_handle_->SetViewportRect(viewport_rect);
   }
 
   // Update handle layout after setting the new Viewport size.
   UpdateHandleLayoutIfNecessary();
 }
 
 bool TouchSelectionController::WillHandleTouchEvent(const MotionEvent& event) {
-  if (config_.enable_longpress_drag_selection &&
-      longpress_drag_selector_.WillHandleTouchEvent(event)) {
-    return true;
+  bool handled = WillHandleTouchEventImpl(event) || consume_touch_sequence_;

[sadrul, 2016/09/22 01:55:51]

Reverse the order, so when |consume_touch_sequence_| is turned on, you don't
need to call WillHandleTouchEventImpl()

[mohsen, 2016/09/22 02:50:15]

I actually want to call WillHandleTouchEventImpl() in order for touch selection
to perform its duty! Maybe, I should rewrite it as:

  bool handled = WillHandleTouchEventImpl(event);
  handled = handled || consume_touch_sequence_;

or:

  bool handled = WillHandleTouchEventImpl(event);
  if (consume_touch_sequence_)
    handled = true;

or:

  bool handled = WillHandleTouchEventImpl(event);
  if (!handled && consume_touch_sequence_)
    handled = true;

none of which seems better to me than the original one. WDYT?

[sadrul, 2016/09/22 03:10:33]

Ah, I see. OK, in that case, either works. You could also do:

  bool handled = Will...Impl..;
  ...
  return handled || consume_touch_sequence_;

[mohsen, 2016/09/22 18:08:45]

That would not work as the value of |consume_touch_sequence_| might change
before returning unless we keep the value in a local variable. However, I'm
changing this code (see the next comment) and this would not be an issue
anymore.

+  switch (event.GetAction()) {
+    case MotionEvent::ACTION_DOWN:
+      DCHECK(!consume_touch_sequence_);
+      // If ACTION_DOWN is consumed, the rest of the touch sequence should be
+      // consumed, too.
+      if (handled)
+        consume_touch_sequence_ = true;
+      break;
+    case MotionEvent::ACTION_UP:
+    case MotionEvent::ACTION_CANCEL:
+      consume_touch_sequence_ = false;

[sadrul, 2016/09/22 01:55:51]

Should you check that this is the last pointer? (i.e. event.GetPointerCount()
should be 0/1?)

[mohsen, 2016/09/22 02:50:15]

I was under the impression that ACTION_UP is returned for the last pointer (vs.
ACTION_POINTER_UP for other ones) (e.g. see
MotionEventAura::UpdateCachedAction()). Is it not the case?

[sadrul, 2016/09/22 03:10:33]

For UP, that does seem to be the case. Probably not for cancel?

[mohsen, 2016/09/22 18:08:44]

I could not conclude whether there is just one CANCEL for all pointers or one
for each pointer! There are codes that suggest the former (lots of places that
handle CANCEL with UP rather than POINTER_UP, GestureProvider::ResetDetection(),
GetActionFrom() in motion_event_web.cc, etc.) and codes that suggest the latter
(MotionEventAura::UpdateCachedAction(),
GestureProviderTest.GestureBeginAndEndOnCancel, etc.).

Anyways, per tdresser@'s suggestion, instead of using CANCEL/UP to reset
|consume_touch_sequence_|, I'm switching to use the next ACTION_DOWN. Done.

+      break;
+    default:
+      break;
   }
-
-  if (active_status_ == INSERTION_ACTIVE) {
-    DCHECK(insertion_handle_);
-    return insertion_handle_->WillHandleTouchEvent(event);
-  }
-
-  if (active_status_ == SELECTION_ACTIVE) {
-    DCHECK(start_selection_handle_);
-    DCHECK(end_selection_handle_);
-    if (start_selection_handle_->IsActive())
-      return start_selection_handle_->WillHandleTouchEvent(event);
-
-    if (end_selection_handle_->IsActive())
-      return end_selection_handle_->WillHandleTouchEvent(event);
-
-    const gfx::PointF event_pos(event.GetX(), event.GetY());
-    if ((event_pos - GetStartPosition()).LengthSquared() <=
-        (event_pos - GetEndPosition()).LengthSquared()) {
-      return start_selection_handle_->WillHandleTouchEvent(event);
-    }
-    return end_selection_handle_->WillHandleTouchEvent(event);
-  }
-
-  return false;
+  return handled;
 }
 
 bool TouchSelectionController::WillHandleTapEvent(const gfx::PointF& location,
                                                   int tap_count) {
   if (WillHandleTapOrLongPress(location))
     return true;
 
   if (tap_count > 1) {
     response_pending_input_event_ = REPEATED_TAP;
     ShowSelectionHandlesAutomatically();
@@ skipping 131 matching lines @@
 }
 
 const gfx::PointF& TouchSelectionController::GetStartPosition() const {
   return start_.edge_bottom();
 }
 
 const gfx::PointF& TouchSelectionController::GetEndPosition() const {
   return end_.edge_bottom();
 }
 
+bool TouchSelectionController::WillHandleTouchEventImpl(
+    const MotionEvent& event) {
+  if (config_.enable_longpress_drag_selection &&
+      longpress_drag_selector_.WillHandleTouchEvent(event)) {
+    return true;
+  }
+
+  if (active_status_ == INSERTION_ACTIVE) {
+    DCHECK(insertion_handle_);
+    return insertion_handle_->WillHandleTouchEvent(event);
+  }
+
+  if (active_status_ == SELECTION_ACTIVE) {
+    DCHECK(start_selection_handle_);
+    DCHECK(end_selection_handle_);
+    if (start_selection_handle_->IsActive())
+      return start_selection_handle_->WillHandleTouchEvent(event);
+
+    if (end_selection_handle_->IsActive())
+      return end_selection_handle_->WillHandleTouchEvent(event);
+
+    const gfx::PointF event_pos(event.GetX(), event.GetY());
+    if ((event_pos - GetStartPosition()).LengthSquared() <=
+        (event_pos - GetEndPosition()).LengthSquared()) {
+      return start_selection_handle_->WillHandleTouchEvent(event);
+    }
+    return end_selection_handle_->WillHandleTouchEvent(event);
+  }
+
+  return false;
+}
+
 void TouchSelectionController::OnDragBegin(
     const TouchSelectionDraggable& draggable,
     const gfx::PointF& drag_position) {
   if (&draggable == insertion_handle_.get()) {
     DCHECK_EQ(active_status_, INSERTION_ACTIVE);
     client_->OnSelectionEvent(INSERTION_HANDLE_DRAG_STARTED);
     anchor_drag_to_selection_start_ = true;
     return;
   }
 
@@ skipping 324 matching lines @@
     base::TimeDelta duration = base::TimeTicks::Now() - selection_start_time_;
     UMA_HISTOGRAM_CUSTOM_TIMES("Event.TouchSelection.WasDraggedDuration",
                                duration,
                                base::TimeDelta::FromMilliseconds(500),
                                base::TimeDelta::FromSeconds(60),
                                60);
   }
 }
 
 }  // namespace ui