Suppress LongPress/Tap, and TwoFingerTap when TapDown cancels a fling.

Suppress LongPress/Tap, and TwoFingerTap when TapDown cancels a fling.

MUST_SUPPRESS_TAP_ENDS state is added to TapSuppressionController. When
a tap end event causes to drop the stashed tap down, the controller goes
to the new state. This state shows that all following tap ends should
get suppressed till the end of the current touch sequence.

BUG=656434
TEST=TapSuppressionControllerTest.*SufficientlyLateTapUp

Committed: https://crrev.com/3d1623599ed4bd5fc9aef5ac9a835c3cfc4d6d43
Cr-Commit-Position: refs/heads/master@{#437923}

[sahel, 2016-11-30 21:44:39]

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

[commit-bot: I haz the power, 2016-11-30 21:45:06]

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-11-30 22:30:38]

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

[commit-bot: I haz the power, 2016-11-30 22:30:39]

Dry run: This issue passed the CQ dry run.

[sahel, 2016-12-01 15:40:41]

sahel@chromium.org changed reviewers:
+ tdresser@chromium.org

[tdresser, 2016-12-01 18:57:59]

https://codereview.chromium.org/2542453003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/input/input_router_config_helper.cc (right):

https://codereview.chromium.org/2542453003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/input_router_config_helper.cc:55:
gesture_config->long_press_time_in_ms() * 1.1);
Maybe elaborate a bit here - why does this need to be greater than the long
press time?

Maybe also increment the timer by a fixed amount, instead of multiplying - the
impact is a bit more obvious that way.

https://codereview.chromium.org/2542453003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/input/tap_suppression_controller.cc (right):

https://codereview.chromium.org/2542453003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/tap_suppression_controller.cc:16:
max_tap_gap_time(base::TimeDelta::FromMilliseconds(550)) {
Why this change? This constant likely needs a comment.

https://codereview.chromium.org/2542453003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/tap_suppression_controller.cc:114: // state
to NOTHING when the stashed tap down is dropped.
This comment feels related to the history of the file, which is a bit confusing.

I'd say "to drop the latter events as well as the tap cancel, change the state
to MUST_SUPPRESS_TAP_ENDS" when the stashed tap down is dropped.

https://codereview.chromium.org/2542453003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/input/tap_suppression_controller.h (right):

https://codereview.chromium.org/2542453003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/tap_suppression_controller.h:73:
MUST_SUPPRESS_TAP_ENDS,
How about SUPPRESSING_TAPS?

The events you're suppressing are LongTap and TwoFingerTap right?
Might be worth adding a comment to this state as well, saying what evens are
suppressed.

https://codereview.chromium.org/2542453003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/input/tap_suppression_controller_unittest.cc
(right):

https://codereview.chromium.org/2542453003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/tap_suppression_controller_unittest.cc:494:
// LongTap should be suppressed when the previously suppressed TapDown is
Do we have a test where the LongTap is forwarded?

[sahel, 2016-12-05 22:04:57]

Patchset #2 (id:20001) has been deleted

[sahel, 2016-12-05 22:12:15]

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

[commit-bot: I haz the power, 2016-12-05 22:13:05]

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-12-05 23:06:45]

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

[commit-bot: I haz the power, 2016-12-05 23:06:46]

Dry run: This issue passed the CQ dry run.

[sahel, 2016-12-06 15:18:13]

https://codereview.chromium.org/2542453003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/input/input_router_config_helper.cc (right):

https://codereview.chromium.org/2542453003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/input_router_config_helper.cc:55:
gesture_config->long_press_time_in_ms() * 1.1);
On 2016/12/01 18:57:59, tdresser wrote:
> Maybe elaborate a bit here - why does this need to be greater than the long
> press time?
> 
> Maybe also increment the timer by a fixed amount, instead of multiplying - the
> impact is a bit more obvious that way.

Done.

https://codereview.chromium.org/2542453003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/input/tap_suppression_controller.cc (right):

https://codereview.chromium.org/2542453003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/tap_suppression_controller.cc:16:
max_tap_gap_time(base::TimeDelta::FromMilliseconds(550)) {
On 2016/12/01 18:57:59, tdresser wrote:
> Why this change? This constant likely needs a comment.

This is set to 500ms, because that's the long press timeout (the longest
timeout)
I changed it to 550 because it should be larger than the long press timeout to
suppress it.

https://codereview.chromium.org/2542453003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/tap_suppression_controller.cc:114: // state
to NOTHING when the stashed tap down is dropped.
On 2016/12/01 18:57:59, tdresser wrote:
> This comment feels related to the history of the file, which is a bit
confusing.
> 
> I'd say "to drop the latter events as well as the tap cancel, change the state
> to MUST_SUPPRESS_TAP_ENDS" when the stashed tap down is dropped.

Done.

https://codereview.chromium.org/2542453003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/input/tap_suppression_controller.h (right):

https://codereview.chromium.org/2542453003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/tap_suppression_controller.h:73:
MUST_SUPPRESS_TAP_ENDS,
On 2016/12/01 18:57:59, tdresser wrote:
> How about SUPPRESSING_TAPS?
> 
> The events you're suppressing are LongTap and TwoFingerTap right?
> Might be worth adding a comment to this state as well, saying what evens are
> suppressed.

TapCancel as well. (Before LongTap and TwoFingerTap, TapCancel is generated.)

https://codereview.chromium.org/2542453003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/input/tap_suppression_controller_unittest.cc
(right):

https://codereview.chromium.org/2542453003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/tap_suppression_controller_unittest.cc:494:
// LongTap should be suppressed when the previously suppressed TapDown is
On 2016/12/01 18:57:59, tdresser wrote:
> Do we have a test where the LongTap is forwarded?

In the tests there is no distinction between long tap and a tap.
If we wait for the timeout and send the TapUp afterwards, it will
be considered as long tap. Otherwise it will be a normal tap.

[tdresser, 2016-12-07 19:53:22]

LGTM with nits!

https://codereview.chromium.org/2542453003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_config_helper.cc (right):

https://codereview.chromium.org/2542453003/diff/40001/content/browser/rendere...
content/browser/renderer_host/input/input_router_config_helper.cc:54: // press
to get suppressed before forwarding the stashed tapDown.
Explain why we need this delay - to protect against a late tap cancel ack.

https://codereview.chromium.org/2542453003/diff/40001/content/browser/rendere...
content/browser/renderer_host/input/input_router_config_helper.cc:57:
gesture_config->long_press_time_in_ms() + 50);
We should make sure to handle the case where a long press comes after this
timeout.

https://codereview.chromium.org/2542453003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/input/tap_suppression_controller.cc (right):

https://codereview.chromium.org/2542453003/diff/40001/content/browser/rendere...
content/browser/renderer_host/input/tap_suppression_controller.cc:16: //
suppressing the long press.
Clarify this comment - the long press could happen later, and this is to protect
against a late ta cancel ack.

https://codereview.chromium.org/2542453003/diff/40001/content/browser/rendere...
content/browser/renderer_host/input/tap_suppression_controller.cc:20:
max_tap_gap_time(base::TimeDelta::FromMilliseconds(550)) {}
Should the long press time be passed in as a parameter?

https://codereview.chromium.org/2542453003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/input/tap_suppression_controller.h (right):

https://codereview.chromium.org/2542453003/diff/40001/content/browser/rendere...
content/browser/renderer_host/input/tap_suppression_controller.h:74: // timer
expiration, the contorller enters the SUPPRESSING_TAPS state.
controller

https://codereview.chromium.org/2542453003/diff/40001/content/browser/rendere...
content/browser/renderer_host/input/tap_suppression_controller.h:76: //
TwoFingerTap, and TapCancel gesture events untill the next tapDown event
until

[sahel, 2016-12-09 16:48:54]

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

[sahel, 2016-12-09 16:49:12]

https://codereview.chromium.org/2542453003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/input/input_router_config_helper.cc (right):

https://codereview.chromium.org/2542453003/diff/40001/content/browser/rendere...
content/browser/renderer_host/input/input_router_config_helper.cc:54: // press
to get suppressed before forwarding the stashed tapDown.
On 2016/12/07 19:53:21, tdresser wrote:
> Explain why we need this delay - to protect against a late tap cancel ack.

Done.

https://codereview.chromium.org/2542453003/diff/40001/content/browser/rendere...
content/browser/renderer_host/input/input_router_config_helper.cc:57:
gesture_config->long_press_time_in_ms() + 50);
On 2016/12/07 19:53:21, tdresser wrote:
> We should make sure to handle the case where a long press comes after this
> timeout.

Done.

https://codereview.chromium.org/2542453003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/input/tap_suppression_controller.cc (right):

https://codereview.chromium.org/2542453003/diff/40001/content/browser/rendere...
content/browser/renderer_host/input/tap_suppression_controller.cc:16: //
suppressing the long press.
On 2016/12/07 19:53:21, tdresser wrote:
> Clarify this comment - the long press could happen later, and this is to
protect
> against a late ta cancel ack.

Done.

https://codereview.chromium.org/2542453003/diff/40001/content/browser/rendere...
content/browser/renderer_host/input/tap_suppression_controller.cc:20:
max_tap_gap_time(base::TimeDelta::FromMilliseconds(550)) {}
On 2016/12/07 19:53:21, tdresser wrote:
> Should the long press time be passed in as a parameter?

Done.

https://codereview.chromium.org/2542453003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/input/tap_suppression_controller.h (right):

https://codereview.chromium.org/2542453003/diff/40001/content/browser/rendere...
content/browser/renderer_host/input/tap_suppression_controller.h:74: // timer
expiration, the contorller enters the SUPPRESSING_TAPS state.
On 2016/12/07 19:53:21, tdresser wrote:
> controller

Done.

https://codereview.chromium.org/2542453003/diff/40001/content/browser/rendere...
content/browser/renderer_host/input/tap_suppression_controller.h:76: //
TwoFingerTap, and TapCancel gesture events untill the next tapDown event
On 2016/12/07 19:53:21, tdresser wrote:
> until

Done.

[commit-bot: I haz the power, 2016-12-09 16:49:19]

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-12-09 18:47:42]

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

[commit-bot: I haz the power, 2016-12-09 18:47:42]

Dry run: This issue passed the CQ dry run.

[sahel, 2016-12-09 18:55:18]

Patchset #3 (id:60001) has been deleted

[sahel, 2016-12-09 19:44:23]

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

[commit-bot: I haz the power, 2016-12-09 19:45: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-12-09 22:38:42]

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

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

Dry run: This issue passed the CQ dry run.

[sahel, 2016-12-12 15:24:58]

The CQ bit was checked by sahel@chromium.org

[sahel, 2016-12-12 15:24:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/2542453003/#ps80001
(title: "clarification comments added.")

[commit-bot: I haz the power, 2016-12-12 15:25:16]

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-12-12 17:09:58]

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

[commit-bot: I haz the power, 2016-12-12 17:10:00]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sahel, 2016-12-12 21:04:43]

The CQ bit was checked by sahel@chromium.org

[commit-bot: I haz the power, 2016-12-12 21:06:11]

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-12-12 21:42:35]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481576683014060,
"parent_rev": "d736836f0d86d6080bd104deacc3122c85251a29", "commit_rev":
"57adccfc6e7d537e437e5a52ab4ada2fc4b44282"}

[commit-bot: I haz the power, 2016-12-12 21:43:05]

Description was changed from

==========
Suppress LongPress/Tap, and TwoFingerTap when TapDown cancels a fling.

MUST_SUPPRESS_TAP_ENDS state is added to TapSuppressionController. When
a tap end event causes to drop the stashed tap down, the controller goes
to the new state. This state shows that all following tap ends should
get suppressed till the end of the current touch sequence.

BUG=656434
TEST=TapSuppressionControllerTest.*SufficientlyLateTapUp
==========

to

==========
Suppress LongPress/Tap, and TwoFingerTap when TapDown cancels a fling.

MUST_SUPPRESS_TAP_ENDS state is added to TapSuppressionController. When
a tap end event causes to drop the stashed tap down, the controller goes
to the new state. This state shows that all following tap ends should
get suppressed till the end of the current touch sequence.

BUG=656434
TEST=TapSuppressionControllerTest.*SufficientlyLateTapUp

Review-Url: https://codereview.chromium.org/2542453003
==========

[commit-bot: I haz the power, 2016-12-12 21:43:06]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-12-12 21:46:50]

Description was changed from

==========
Suppress LongPress/Tap, and TwoFingerTap when TapDown cancels a fling.

MUST_SUPPRESS_TAP_ENDS state is added to TapSuppressionController. When
a tap end event causes to drop the stashed tap down, the controller goes
to the new state. This state shows that all following tap ends should
get suppressed till the end of the current touch sequence.

BUG=656434
TEST=TapSuppressionControllerTest.*SufficientlyLateTapUp

Review-Url: https://codereview.chromium.org/2542453003
==========

to

==========
Suppress LongPress/Tap, and TwoFingerTap when TapDown cancels a fling.

MUST_SUPPRESS_TAP_ENDS state is added to TapSuppressionController. When
a tap end event causes to drop the stashed tap down, the controller goes
to the new state. This state shows that all following tap ends should
get suppressed till the end of the current touch sequence.

BUG=656434
TEST=TapSuppressionControllerTest.*SufficientlyLateTapUp

Committed: https://crrev.com/3d1623599ed4bd5fc9aef5ac9a835c3cfc4d6d43
Cr-Commit-Position: refs/heads/master@{#437923}
==========

[commit-bot: I haz the power, 2016-12-12 21:46:51]

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