Adjust cancelabilty of touchmoves that are queued when scroll start occurs

Adjust cancelabilty of touchmoves that are queued when scroll start occurs.
   
Generalize the coalescing callback to be a visit of the queue. Treat
TouchScrollStart as a separate path in here.

BUG=700407
Review-Url: https://codereview.chromium.org/2788203002
Cr-Commit-Position: refs/heads/master@{#461262}
Committed: https://chromium.googlesource.com/chromium/src/+/887c78b39f27eccd42ec308d1b899efc2a8f7a49

[dtapuska, 2017-03-31 18:55:40]

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

[dtapuska, 2017-03-31 18:55:45]

Description was changed from

==========
Cancel out touchmoves that are pending queued when a scroll start occurs.

Generalize the coalescing callback to be a visit of the queue. Treat
TouchScrollStart as a separate path in here.

BUG=700407
==========

to

==========
Cancel out touchmoves that are pending queued when a scroll start occurs.

Generalize the coalescing callback to be a visit of the queue. Treat
TouchScrollStart as a separate path in here.

BUG=700407
==========

[dtapuska, 2017-03-31 18:55:46]

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

[dtapuska, 2017-03-31 18:56:09]

PTAL

[commit-bot: I haz the power, 2017-03-31 18:56:15]

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

[tdresser, 2017-03-31 19:31:02]

"Cancel out touchmoves that are pending queued when a scroll start occurs."

Cancel out implies that you're removing the events, which we aren't. We should
make it clear that these are just being marked uncancellable.

https://codereview.chromium.org/2788203002/diff/1/content/renderer/input/main...
File content/renderer/input/main_thread_event_queue_task.h (right):

https://codereview.chromium.org/2788203002/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue_task.h:31: // event with this
event, stop iterating or keep iterating.
This naming is confusing. We aren't notifying an event will be queued, cause we
might not queue it, we might coalesce it. It also isn't really clear from the
comment what it means to keep iterating here.

https://codereview.chromium.org/2788203002/diff/1/content/renderer/input/main...
File content/renderer/input/main_thread_event_queue_task_list.cc (right):

https://codereview.chromium.org/2788203002/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue_task_list.cc:20: case
MainThreadEventQueueTask::NotifyResult::Stop:
I found "CannotCoalesce" a bit clearer here. Why the change?

[commit-bot: I haz the power, 2017-03-31 19:51:00]

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

[commit-bot: I haz the power, 2017-03-31 19:51:01]

Dry run: This issue passed the CQ dry run.

[dtapuska, 2017-03-31 20:17:26]

Description was changed from

==========
Cancel out touchmoves that are pending queued when a scroll start occurs.

Generalize the coalescing callback to be a visit of the queue. Treat
TouchScrollStart as a separate path in here.

BUG=700407
==========

to

==========
Adjust cancelabilty of touchmoves that are queued when scroll start occurs.
   
Generalize the coalescing callback to be a visit of the queue. Treat
TouchScrollStart as a separate path in here.

BUG=700407
==========

[dtapuska, 2017-03-31 20:18:18]

PTAL

https://codereview.chromium.org/2788203002/diff/1/content/renderer/input/main...
File content/renderer/input/main_thread_event_queue_task.h (right):

https://codereview.chromium.org/2788203002/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue_task.h:31: // event with this
event, stop iterating or keep iterating.
On 2017/03/31 19:31:02, tdresser wrote:
> This naming is confusing. We aren't notifying an event will be queued, cause
we
> might not queue it, we might coalesce it. It also isn't really clear from the
> comment what it means to keep iterating here.

I adjusted the names a bit

https://codereview.chromium.org/2788203002/diff/1/content/renderer/input/main...
File content/renderer/input/main_thread_event_queue_task_list.cc (right):

https://codereview.chromium.org/2788203002/diff/1/content/renderer/input/main...
content/renderer/input/main_thread_event_queue_task_list.cc:20: case
MainThreadEventQueueTask::NotifyResult::Stop:
On 2017/03/31 19:31:02, tdresser wrote:
> I found "CannotCoalesce" a bit clearer here. Why the change?

As discussed in person it covers a few scenarios.

[tdresser, 2017-03-31 20:50:20]

LGTM with nits.

https://codereview.chromium.org/2788203002/diff/20001/content/renderer/input/...
File content/renderer/input/main_thread_event_queue.cc (right):

https://codereview.chromium.org/2788203002/diff/20001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:97: return
FilterResult::PreventQueuingEvent;
It seems a bit weird to me that we consider coalescing to not be queueing. Could
this be called "CoalescedEvent"?

https://codereview.chromium.org/2788203002/diff/20001/content/renderer/input/...
File content/renderer/input/main_thread_event_queue_task.h (right):

https://codereview.chromium.org/2788203002/diff/20001/content/renderer/input/...
content/renderer/input/main_thread_event_queue_task.h:31: // are to prevent
queuing the event, stop iterating or keep iterating.
Add more detail around what it means to keep iterating. How is this intended to
be used?

https://codereview.chromium.org/2788203002/diff/20001/content/renderer/input/...
File content/renderer/input/main_thread_event_queue_unittest.cc (right):

https://codereview.chromium.org/2788203002/diff/20001/content/renderer/input/...
content/renderer/input/main_thread_event_queue_unittest.cc:1072:
TEST_P(MainThreadEventQueueTest, BlockingTouchMoveWithTouchEnd) {
There is no extra touch end here. The name seems wrong?

[dtapuska, 2017-03-31 21:06:11]

https://codereview.chromium.org/2788203002/diff/20001/content/renderer/input/...
File content/renderer/input/main_thread_event_queue.cc (right):

https://codereview.chromium.org/2788203002/diff/20001/content/renderer/input/...
content/renderer/input/main_thread_event_queue.cc:97: return
FilterResult::PreventQueuingEvent;
On 2017/03/31 20:50:20, tdresser wrote:
> It seems a bit weird to me that we consider coalescing to not be queueing.
Could
> this be called "CoalescedEvent"?

Done.

https://codereview.chromium.org/2788203002/diff/20001/content/renderer/input/...
File content/renderer/input/main_thread_event_queue_task.h (right):

https://codereview.chromium.org/2788203002/diff/20001/content/renderer/input/...
content/renderer/input/main_thread_event_queue_task.h:31: // are to prevent
queuing the event, stop iterating or keep iterating.
On 2017/03/31 20:50:20, tdresser wrote:
> Add more detail around what it means to keep iterating. How is this intended
to
> be used?

Done.

https://codereview.chromium.org/2788203002/diff/20001/content/renderer/input/...
File content/renderer/input/main_thread_event_queue_unittest.cc (right):

https://codereview.chromium.org/2788203002/diff/20001/content/renderer/input/...
content/renderer/input/main_thread_event_queue_unittest.cc:1072:
TEST_P(MainThreadEventQueueTest, BlockingTouchMoveWithTouchEnd) {
On 2017/03/31 20:50:20, tdresser wrote:
> There is no extra touch end here. The name seems wrong?

Yes there is kEvents[1].ReleasePoint(0) turns kEvents[1] into a TouchEnd. The
touchscroll end is appended after the TouchEnd ensuring that the propagation
doesn't happen.

[dtapuska, 2017-03-31 21:09:11]

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

[commit-bot: I haz the power, 2017-03-31 21:10:03]

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, 2017-03-31 22:28:42]

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

[commit-bot: I haz the power, 2017-03-31 22:28:43]

Dry run: This issue passed the CQ dry run.

[dtapuska, 2017-03-31 22:48:32]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2017-03-31 22:48:33]

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/2788203002/#ps40001
(title: "Fix nits")

[commit-bot: I haz the power, 2017-03-31 22:49:10]

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, 2017-03-31 22:55:30]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491000512681370,
"parent_rev": "dc1a0f2c42b5de2840c22e0a3e5c5edc57dd4715", "commit_rev":
"887c78b39f27eccd42ec308d1b899efc2a8f7a49"}

[commit-bot: I haz the power, 2017-03-31 22:56:13]

Description was changed from

==========
Adjust cancelabilty of touchmoves that are queued when scroll start occurs.
   
Generalize the coalescing callback to be a visit of the queue. Treat
TouchScrollStart as a separate path in here.

BUG=700407
==========

to

==========
Adjust cancelabilty of touchmoves that are queued when scroll start occurs.
   
Generalize the coalescing callback to be a visit of the queue. Treat
TouchScrollStart as a separate path in here.

BUG=700407

Review-Url: https://codereview.chromium.org/2788203002
Cr-Commit-Position: refs/heads/master@{#461262}
Committed:
https://chromium.googlesource.com/chromium/src/+/887c78b39f27eccd42ec308d1b89...
==========

[commit-bot: I haz the power, 2017-03-31 22:56:16]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/887c78b39f27eccd42ec308d1b89...