Delegate touch handler registration logic to the TouchEventQueue

Delegate touch handler registration logic to the TouchEventQueue

Teach the TouchEventQueue about touch handler registration, allowing it to make
more informed touch forwarding decisions.  This also prevents corner cases where
a partial touch sequence would be improperly forwarded to the renderer after a
touch handler is registered.

BUG=332418
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244072

[jdduke (slow), 2014-01-09 19:20:43]

tdresser@: PTAL.  Thanks.  This logic is somewhat redundant with what we have in
ContentViewGestureHandler, but I'd like for it to exist concretely somewhere
lower in the pipeline.

[tdresser, 2014-01-09 21:14:31]

A few nits, and a few questions about behavior I'm not clear on.

I'll another look once you've got the tests passing.

https://codereview.chromium.org/132083006/diff/30001/content/browser/renderer...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/132083006/diff/30001/content/browser/renderer...
content/browser/renderer_host/input/input_router_impl.cc:266:
DCHECK(touch_event_queue_->has_handlers() || touch_event_queue_->empty());
This DCHECK doesn't seem to make sense in conjunction with the comment above.
The comment describes how we handle the case where there are no touch handlers
but there are events in the queue, and the DCHECK states that this case will
never occur.

Does this case ever occur?

The comment is also kind of hard to parse due to sentence length.

https://codereview.chromium.org/132083006/diff/30001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/132083006/diff/30001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:314:
touch_ack_states_.clear();
Shouldn't these be cleared per finger, not per gesture?

https://codereview.chromium.org/132083006/diff/30001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:377: return;
This should be a DCHECK, shouldn't it? (Given the way we already do this check
in the input router)

https://codereview.chromium.org/132083006/diff/30001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:383: // if we cache the
last WebTouchEvent sent to the renderer.
Can you give me an example of when we'd want a TouchCancel sent here?

https://codereview.chromium.org/132083006/diff/30001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue.h (right):

https://codereview.chromium.org/132083006/diff/30001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.h:140: bool
scroll_in_progress_;
Good rename.

https://codereview.chromium.org/132083006/diff/30001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right):

https://codereview.chromium.org/132083006/diff/30001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue_unittest.cc:231:
TEST_F(TouchEventQueueTest, HasNoTouchHandlers) {
Test name isn't very descriptive. Perhaps QueueFlushedWhenHandlersRemoved?

https://codereview.chromium.org/132083006/diff/30001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue_unittest.cc:268: // the
rest of the sequence will be ignored until a new touch sequence.
until a new touch sequence. -> until the next touch sequence.

[jdduke (slow), 2014-01-09 22:05:03]

Thanks for review!  Tests fixed.  There's a failure on linux_chromeos,
accessibility_unittests_br, but it looks like it might be a flake?

https://codereview.chromium.org/132083006/diff/30001/content/browser/renderer...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/132083006/diff/30001/content/browser/renderer...
content/browser/renderer_host/input/input_router_impl.cc:266:
DCHECK(touch_event_queue_->has_handlers() || touch_event_queue_->empty());
On 2014/01/09 21:14:32, tdresser wrote:
> This DCHECK doesn't seem to make sense in conjunction with the comment above.
> The comment describes how we handle the case where there are no touch handlers
> but there are events in the queue, and the DCHECK states that this case will
> never occur.
> 
> Does this case ever occur?
> 
> The comment is also kind of hard to parse due to sentence length.

The comment wording was poorly modified on my part.  It was supposed to mean
that we may get a "no touch handler" message while there are touch events in the
queue, which can definitely happen.  However, if that does happen, the queue
should be flushed and remain empty.

 I've shortened the comment.  Also, the DCHECK's in TouchEventQueue should be
sufficient to make the guarantee that the queue is empty if there's no touch
handler.

https://codereview.chromium.org/132083006/diff/30001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/132083006/diff/30001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:314:
touch_ack_states_.clear();
On 2014/01/09 21:14:32, tdresser wrote:
> Shouldn't these be cleared per finger, not per gesture?

I thought about adding a DCHECK(touch_ack_states_.empty()), but that is safe
only if the TEQ gets ALL the touch events.  For instance, on Android, if the
first touch down responds with NO_CONSUMER_EXISTS, we cease forwarding any touch
events (for the current sequence).  In that case, |touch_ack_states_| may very
well be non-empty upon the next touch sequence.

As we transition to a unified GR, we'll rely more heavily on the TEQ for
verifying a consistent stream of touches, and we can harden this kind of
assumption.  Good catch.

https://codereview.chromium.org/132083006/diff/30001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:383: // if we cache the
last WebTouchEvent sent to the renderer.
On 2014/01/09 21:14:32, tdresser wrote:
> Can you give me an example of when we'd want a TouchCancel sent here?

I was talking with rbyers@ about this. He seemed to think that interrupting a
touch sequence mid-stream might have some unintended consequences in the
renderer (blink apparently keeps around some touch state tracking).  I don't
think we have any outstanding bugs related to this, and at the least this change
should not be regressing in any way.  Your call whether you think the comment
worthwhile.

https://codereview.chromium.org/132083006/diff/30001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right):

https://codereview.chromium.org/132083006/diff/30001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue_unittest.cc:231:
TEST_F(TouchEventQueueTest, HasNoTouchHandlers) {
On 2014/01/09 21:14:32, tdresser wrote:
> Test name isn't very descriptive. Perhaps QueueFlushedWhenHandlersRemoved?

Done.

https://codereview.chromium.org/132083006/diff/30001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue_unittest.cc:268: // the
rest of the sequence will be ignored until a new touch sequence.
On 2014/01/09 21:14:32, tdresser wrote:
> until a new touch sequence. -> until the next touch sequence.

Done.

[tdresser, 2014-01-09 22:19:52]

On 2014/01/09 22:05:03, jdduke wrote:
> Thanks for review!  Tests fixed.  There's a failure on linux_chromeos,
> accessibility_unittests_br, but it looks like it might be a flake?
> 
>
https://codereview.chromium.org/132083006/diff/30001/content/browser/renderer...
> File content/browser/renderer_host/input/input_router_impl.cc (right):
> 
>
https://codereview.chromium.org/132083006/diff/30001/content/browser/renderer...
> content/browser/renderer_host/input/input_router_impl.cc:266:
> DCHECK(touch_event_queue_->has_handlers() || touch_event_queue_->empty());
> On 2014/01/09 21:14:32, tdresser wrote:
> > This DCHECK doesn't seem to make sense in conjunction with the comment
above.
> > The comment describes how we handle the case where there are no touch
handlers
> > but there are events in the queue, and the DCHECK states that this case will
> > never occur.
> > 
> > Does this case ever occur?
> > 
> > The comment is also kind of hard to parse due to sentence length.
> 
> The comment wording was poorly modified on my part.  It was supposed to mean
> that we may get a "no touch handler" message while there are touch events in
the
> queue, which can definitely happen.  However, if that does happen, the queue
> should be flushed and remain empty.
> 
>  I've shortened the comment.  Also, the DCHECK's in TouchEventQueue should be
> sufficient to make the guarantee that the queue is empty if there's no touch
> handler.
> 
>
https://codereview.chromium.org/132083006/diff/30001/content/browser/renderer...
> File content/browser/renderer_host/input/touch_event_queue.cc (right):
> 
>
https://codereview.chromium.org/132083006/diff/30001/content/browser/renderer...
> content/browser/renderer_host/input/touch_event_queue.cc:314:
> touch_ack_states_.clear();
> On 2014/01/09 21:14:32, tdresser wrote:
> > Shouldn't these be cleared per finger, not per gesture?
> 
> I thought about adding a DCHECK(touch_ack_states_.empty()), but that is safe
> only if the TEQ gets ALL the touch events.  For instance, on Android, if the
> first touch down responds with NO_CONSUMER_EXISTS, we cease forwarding any
touch
> events (for the current sequence).  In that case, |touch_ack_states_| may very
> well be non-empty upon the next touch sequence.
> 
> As we transition to a unified GR, we'll rely more heavily on the TEQ for
> verifying a consistent stream of touches, and we can harden this kind of
> assumption.  Good catch.
> 
>
https://codereview.chromium.org/132083006/diff/30001/content/browser/renderer...
> content/browser/renderer_host/input/touch_event_queue.cc:383: // if we cache
the
> last WebTouchEvent sent to the renderer.
> On 2014/01/09 21:14:32, tdresser wrote:
> > Can you give me an example of when we'd want a TouchCancel sent here?
> 
> I was talking with rbyers@ about this. He seemed to think that interrupting a
> touch sequence mid-stream might have some unintended consequences in the
> renderer (blink apparently keeps around some touch state tracking).  I don't
> think we have any outstanding bugs related to this, and at the least this
change
> should not be regressing in any way.  Your call whether you think the comment
> worthwhile.
> 
>
https://codereview.chromium.org/132083006/diff/30001/content/browser/renderer...
> File content/browser/renderer_host/input/touch_event_queue_unittest.cc
(right):
> 
>
https://codereview.chromium.org/132083006/diff/30001/content/browser/renderer...
> content/browser/renderer_host/input/touch_event_queue_unittest.cc:231:
> TEST_F(TouchEventQueueTest, HasNoTouchHandlers) {
> On 2014/01/09 21:14:32, tdresser wrote:
> > Test name isn't very descriptive. Perhaps QueueFlushedWhenHandlersRemoved?
> 
> Done.
> 
>
https://codereview.chromium.org/132083006/diff/30001/content/browser/renderer...
> content/browser/renderer_host/input/touch_event_queue_unittest.cc:268: // the
> rest of the sequence will be ignored until a new touch sequence.
> On 2014/01/09 21:14:32, tdresser wrote:
> > until a new touch sequence. -> until the next touch sequence.
> 
> Done.

LGTM, thanks.

[tdresser, 2014-01-09 22:20:41]

https://codereview.chromium.org/132083006/diff/30001/content/browser/renderer...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/132083006/diff/30001/content/browser/renderer...
content/browser/renderer_host/input/touch_event_queue.cc:383: // if we cache the
last WebTouchEvent sent to the renderer.
On 2014/01/09 22:05:04, jdduke wrote:
> On 2014/01/09 21:14:32, tdresser wrote:
> > Can you give me an example of when we'd want a TouchCancel sent here?
> 
> I was talking with rbyers@ about this. He seemed to think that interrupting a
> touch sequence mid-stream might have some unintended consequences in the
> renderer (blink apparently keeps around some touch state tracking).  I don't
> think we have any outstanding bugs related to this, and at the least this
change
> should not be regressing in any way.  Your call whether you think the comment
> worthwhile.

Perhaps "Synthesize a TouchCancel if necessary to update blink touch state
tracking"...
I think having the comment there is reasonable.

[commit-bot: I haz the power, 2014-01-09 22:41:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdduke@chromium.org/132083006/260001

[commit-bot: I haz the power, 2014-01-10 00:35:39]

Retried try job too often on linux_aura for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2014-01-10 01:47:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdduke@chromium.org/132083006/260001

[commit-bot: I haz the power, 2014-01-10 03:37:05]

Change committed as 244072