Consume entire touch sequence in touch selection controller

Consume entire touch sequence in touch selection controller

If touch selection gets deactivated while the user is dragging a handle,
the rest of the touch sequence should be consumed by the touch selection
controller; otherwise, there would be an incomplete touch sequence which
confuses gesture recognizer. In other words, if the touch selection
controller consumes a touch press, it should consume the rest of events
in the touch sequence up until the touch release.

BUG=642820
TEST=See bug details

Committed: https://crrev.com/4a024a3bfb952b02c0d0db8e52a9e7ed66df0e82
Cr-Commit-Position: refs/heads/master@{#424513}

[mohsen, 2016-09-21 18:21:06]

The CQ bit was checked by mohsen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 18:21:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 19:10:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-21 19:10:05]

Dry run: This issue passed the CQ dry run.

[mohsen, 2016-09-21 19:14:33]

mohsen@chromium.org changed reviewers:
+ sadrul@chromium.org

[mohsen, 2016-09-21 19:14:33]

Please take a look...

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

https://codereview.chromium.org/2355403002/diff/1/ui/touch_selection/touch_se...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/2355403002/diff/1/ui/touch_selection/touch_se...
ui/touch_selection/touch_selection_controller.cc:182: bool handled =
WillHandleTouchEventImpl(event) || consume_touch_sequence_;
Reverse the order, so when |consume_touch_sequence_| is turned on, you don't
need to call WillHandleTouchEventImpl()

https://codereview.chromium.org/2355403002/diff/1/ui/touch_selection/touch_se...
ui/touch_selection/touch_selection_controller.cc:193: consume_touch_sequence_ =
false;
Should you check that this is the last pointer? (i.e. event.GetPointerCount()
should be 0/1?)

[mohsen, 2016-09-22 02:50:16]

https://codereview.chromium.org/2355403002/diff/1/ui/touch_selection/touch_se...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/2355403002/diff/1/ui/touch_selection/touch_se...
ui/touch_selection/touch_selection_controller.cc:182: bool handled =
WillHandleTouchEventImpl(event) || consume_touch_sequence_;
On 2016/09/22 at 01:55:51, sadrul wrote:
> Reverse the order, so when |consume_touch_sequence_| is turned on, you don't
need to call WillHandleTouchEventImpl()

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?

https://codereview.chromium.org/2355403002/diff/1/ui/touch_selection/touch_se...
ui/touch_selection/touch_selection_controller.cc:193: consume_touch_sequence_ =
false;
On 2016/09/22 at 01:55:51, sadrul wrote:
> Should you check that this is the last pointer? (i.e. event.GetPointerCount()
should be 0/1?)

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]

lgtm (maybe double check with tdresser@ re ACTION_CANCEL?)

https://codereview.chromium.org/2355403002/diff/1/ui/touch_selection/touch_se...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/2355403002/diff/1/ui/touch_selection/touch_se...
ui/touch_selection/touch_selection_controller.cc:182: bool handled =
WillHandleTouchEventImpl(event) || consume_touch_sequence_;
On 2016/09/22 02:50:15, mohsen wrote:
> On 2016/09/22 at 01:55:51, sadrul wrote:
> > Reverse the order, so when |consume_touch_sequence_| is turned on, you don't
> need to call WillHandleTouchEventImpl()
> 
> 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?

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

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

https://codereview.chromium.org/2355403002/diff/1/ui/touch_selection/touch_se...
ui/touch_selection/touch_selection_controller.cc:193: consume_touch_sequence_ =
false;
On 2016/09/22 02:50:15, mohsen wrote:
> On 2016/09/22 at 01:55:51, sadrul wrote:
> > Should you check that this is the last pointer? (i.e.
event.GetPointerCount()
> should be 0/1?)
> 
> 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?

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

[mohsen, 2016-09-22 18:07:23]

The CQ bit was checked by mohsen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 18:07:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2355403002/diff/1/ui/touch_selection/touch_se...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/2355403002/diff/1/ui/touch_selection/touch_se...
ui/touch_selection/touch_selection_controller.cc:182: bool handled =
WillHandleTouchEventImpl(event) || consume_touch_sequence_;
On 2016/09/22 at 03:10:33, sadrul wrote:
> On 2016/09/22 02:50:15, mohsen wrote:
> > On 2016/09/22 at 01:55:51, sadrul wrote:
> > > Reverse the order, so when |consume_touch_sequence_| is turned on, you
don't
> > need to call WillHandleTouchEventImpl()
> > 
> > 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?
> 
> Ah, I see. OK, in that case, either works. You could also do:
> 
>   bool handled = Will...Impl..;
>   ...
>   return handled || consume_touch_sequence_;

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.

https://codereview.chromium.org/2355403002/diff/1/ui/touch_selection/touch_se...
ui/touch_selection/touch_selection_controller.cc:193: consume_touch_sequence_ =
false;
On 2016/09/22 at 03:10:33, sadrul wrote:
> On 2016/09/22 02:50:15, mohsen wrote:
> > On 2016/09/22 at 01:55:51, sadrul wrote:
> > > Should you check that this is the last pointer? (i.e.
event.GetPointerCount()
> > should be 0/1?)
> > 
> > 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?
> 
> For UP, that does seem to be the case. Probably not for cancel?

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.

[commit-bot: I haz the power, 2016-09-22 18:40:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-22 18:40:05]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[mohsen, 2016-09-22 20:29:42]

The CQ bit was checked by mohsen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 20:30:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 22:08:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-22 22:08:43]

Dry run: This issue passed the CQ dry run.

[mohsen, 2016-09-27 19:45:11]

sadrul@: I changed the code in the last patch set to consume events from
action-down to the following action-down rather than the following
action-up/action-cancel. Can you please take another look...

[mohsen, 2016-10-11 17:34:30]

The CQ bit was checked by mohsen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-11 17:34:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-11 18:26:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-11 18:26:19]

Dry run: This issue passed the CQ dry run.

[mohsen, 2016-10-11 19:30:20]

The CQ bit was checked by mohsen@chromium.org

[mohsen, 2016-10-11 19:30:20]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2355403002/#ps60001
(title: "Added a TODO comment")

[commit-bot: I haz the power, 2016-10-11 19:30:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-11 19:37:13]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-11 19:40:44]

Description was changed from

==========
Consume entire touch sequence in touch selection controller

If touch selection gets deactivated while the user is dragging a handle,
the rest of the touch sequence should be consumed by the touch selection
controller; otherwise, there would be an incomplete touch sequence which
confuses gesture recognizer. In other words, if the touch selection
controller consumes a touch press, it should consume the rest of events
in the touch sequence up until the touch release.

BUG=642820
TEST=See bug details
==========

to

==========
Consume entire touch sequence in touch selection controller

If touch selection gets deactivated while the user is dragging a handle,
the rest of the touch sequence should be consumed by the touch selection
controller; otherwise, there would be an incomplete touch sequence which
confuses gesture recognizer. In other words, if the touch selection
controller consumes a touch press, it should consume the rest of events
in the touch sequence up until the touch release.

BUG=642820
TEST=See bug details

Committed: https://crrev.com/4a024a3bfb952b02c0d0db8e52a9e7ed66df0e82
Cr-Commit-Position: refs/heads/master@{#424513}
==========

[commit-bot: I haz the power, 2016-10-11 19:40:45]

Patchset 4 (id:??) landed as
https://crrev.com/4a024a3bfb952b02c0d0db8e52a9e7ed66df0e82
Cr-Commit-Position: refs/heads/master@{#424513}