Consume entire touch sequence in touch selection controller

ui/touch_selection/touch_selection_controller.cc

Index: ui/touch_selection/touch_selection_controller.cc
diff --git a/ui/touch_selection/touch_selection_controller.cc b/ui/touch_selection/touch_selection_controller.cc
index 09f0e4b020bf7ba7398cbd727b405a0015ade221..97498f33085078b6fd22bbe360b6035fa8b2c60e 100644
--- a/ui/touch_selection/touch_selection_controller.cc
+++ b/ui/touch_selection/touch_selection_controller.cc
@@ -68,7 +68,8 @@ TouchSelectionController::TouchSelectionController(
       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_);
 }
 
@@ -178,34 +179,23 @@ void TouchSelectionController::OnViewportChanged(
 }
 
 bool TouchSelectionController::WillHandleTouchEvent(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);
+  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_ == 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,
@@ -357,6 +347,38 @@ 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) {