Events ignoring ack disposition receive synthetic acks.

Events ignoring ack disposition receive synthetic acks.

Logic to handle events which ignore ack disposition has moved from the
|GestureEventFilter| to the |ImmediateInputRouter|, and |WebInputEventTraits|.

This will allow us to make more events ignore their ack disposition, including
touch events.

Gesture event types which will eventually ignore their ack disposition include 
Tap, ScrollBegin, PinchBegin, PinchEnd, etc.

BUG=302852, 275611
TEST=ImmediateInputRouterTest.EventsIgnoringAckDispositionDontWaitForAcks,
ImmediateInputRouterTest.EventsIgnoringAckDispositionStayInOrder

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

[tdresser, 2013-10-25 19:12:13]

joi: Can you take a look at content/{port,public}?
jdduke: Can you take a look at the rest? Feel free to pull in an additional
reviewer if you want.

[jdduke (slow), 2013-10-25 20:37:11]

https://codereview.chromium.org/44983003/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/input/gesture_event_filter.cc (right):

https://codereview.chromium.org/44983003/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/gesture_event_filter.cc:201: if
(coalesced_gesture_events_.size() > 1) {
This whole section is broken with synchronous acks, and I'm amazed WebView
hasn't been burned here.

What we need to do instead is this: before we send any event, check if the
second event exists and can be forwarded together with the first (first is
GestureScrollUpdate and second is GesturePinchUpdate). If it can, great, set
"ignore_next_ack_ = true;" *before* forwarding them both.  Otherwise, just send
the first as usual.

Could you also add a test such that this is properly handled? You can modify
GEFTest so that |GEFClient::SendGestureEventImmediately| synchronously calls
into |GEF::ProcessGestureAck|.  I realize this won't be triggered by your patch
(GestureScrollUpdate ack will not be ignored), but it's a part of the
synchronous ack paradigm so might as well handle it now.

https://codereview.chromium.org/44983003/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/gesture_event_filter.cc:255:
client_->SendGestureEventImmediately(*it);

If a synchronous ack is triggered by this send, might that ever trigger an
additional event being added to the |debouncing_deferral_queue_|? It may not
happen, but we should be robust enough that if it does we don't explode.  We
could fix this with something like

while (!debouncing_deferral_queue_.empty()) { Pop, etc... }

https://codereview.chromium.org/44983003/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/input/immediate_input_router_unittest.cc
(right):

https://codereview.chromium.org/44983003/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/immediate_input_router_unittest.cc:566:
EXPECT_EQ(1U, GetSentMessageCountAndResetSink());

Please also check that 1) We get the ack immediately (GetAndResetAckCount()),
and 2) Sending the ack later has no effect.

https://codereview.chromium.org/44983003/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/immediate_input_router_unittest.cc:599:
EXPECT_EQ(2U, GetSentMessageCountAndResetSink());

Same as above, check that we get the immediate ack and that a later ack from the
renderer has no effect.

[jdduke (slow), 2013-10-25 20:53:34]

https://codereview.chromium.org/44983003/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/input/immediate_input_router.cc (right):

https://codereview.chromium.org/44983003/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/immediate_input_router.cc:348: CLIENT);
Please create a new enum entry for this ack source instead of repurposing
CLIENT.

[jdduke (slow), 2013-10-25 21:00:37]

https://codereview.chromium.org/44983003/diff/1/content/common/input/web_inpu...
File content/common/input/web_input_event_traits.cc (right):

https://codereview.chromium.org/44983003/diff/1/content/common/input/web_inpu...
content/common/input/web_input_event_traits.cc:290: type ==
WebInputEvent::GestureTapCancel ||

If adding GestureTap + GestureTapCancel are giving you trouble, you might just
stick with GestureTapDown + GestureShowPress and treat this patch as a
refactoring, adding the other gesture types in a followup patch.

[Jói, 2013-10-28 10:05:34]

//content/port and //content/public LGTM.

[tdresser, 2013-10-29 14:11:43]

I was just discussing this patch with rbyers@, and he was mentioning that it
might make more sense to send these synthetic acks from the browser
asynchronously, in order to avoid having to worry about the issues with
synchronous acks.

Does that sound reasonable? I can't imagine the performance impact being
significant.

Is there a good reason for WebView to use synchronous acks? If we can't make the
assumption that all acks will be asynchronous, it will be  more difficult to
reason about gesture handling.

[jdduke (slow), 2013-10-29 14:58:50]

On 2013/10/29 14:11:43, tdresser wrote:
> I was just discussing this patch with rbyers@, and he was mentioning that it
> might make more sense to send these synthetic acks from the browser
> asynchronously, in order to avoid having to worry about the issues with
> synchronous acks.
> 
> Does that sound reasonable? I can't imagine the performance impact being
> significant.
> 
> Is there a good reason for WebView to use synchronous acks? If we can't make
the
> assumption that all acks will be asynchronous, it will be  more difficult to
> reason about gesture handling.

I certainly agree that async acks are the better model.

However, WebView API puts us in an awkward position, as the user expects certain
results from a given API call, and it expects them to be synchronous with
respect to that API call.  So we need to flush out all the gestures from a a
given TouchEvent synchronously (assuming they're handled by the impl/UI thread).

[Rick Byers, 2013-10-29 15:19:00]

On 2013/10/29 14:58:50, jdduke wrote:
> On 2013/10/29 14:11:43, tdresser wrote:
> > I was just discussing this patch with rbyers@, and he was mentioning that it
> > might make more sense to send these synthetic acks from the browser
> > asynchronously, in order to avoid having to worry about the issues with
> > synchronous acks.
> > 
> > Does that sound reasonable? I can't imagine the performance impact being
> > significant.
> > 
> > Is there a good reason for WebView to use synchronous acks? If we can't make
> the
> > assumption that all acks will be asynchronous, it will be  more difficult to
> > reason about gesture handling.
> 
> I certainly agree that async acks are the better model.
> 
> However, WebView API puts us in an awkward position, as the user expects
certain
> results from a given API call, and it expects them to be synchronous with
> respect to that API call.  So we need to flush out all the gestures from a a
> given TouchEvent synchronously (assuming they're handled by the impl/UI
thread).

Ah, in that case we should use sync ACKs wherever possible (to help flush out
the issues outside of just webview scenarios).

[tdresser, 2013-11-04 15:57:30]

https://codereview.chromium.org/44983003/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/input/immediate_input_router.cc (right):

https://codereview.chromium.org/44983003/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/immediate_input_router.cc:348: CLIENT);
On 2013/10/25 20:53:34, jdduke wrote:
> Please create a new enum entry for this ack source instead of repurposing
> CLIENT.

Done.

https://codereview.chromium.org/44983003/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/input/immediate_input_router_unittest.cc
(right):

https://codereview.chromium.org/44983003/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/immediate_input_router_unittest.cc:566:
EXPECT_EQ(1U, GetSentMessageCountAndResetSink());
On 2013/10/25 20:37:12, jdduke wrote:
> 
> Please also check that 1) We get the ack immediately (GetAndResetAckCount()),
> and 2) Sending the ack later has no effect.

Done.

https://codereview.chromium.org/44983003/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/immediate_input_router_unittest.cc:599:
EXPECT_EQ(2U, GetSentMessageCountAndResetSink());
On 2013/10/25 20:37:12, jdduke wrote:
> 
> Same as above, check that we get the immediate ack and that a later ack from
the
> renderer has no effect.

Done.

https://codereview.chromium.org/44983003/diff/1/content/common/input/web_inpu...
File content/common/input/web_input_event_traits.cc (right):

https://codereview.chromium.org/44983003/diff/1/content/common/input/web_inpu...
content/common/input/web_input_event_traits.cc:290: type ==
WebInputEvent::GestureTapCancel ||
On 2013/10/25 21:00:38, jdduke wrote:
> 
> If adding GestureTap + GestureTapCancel are giving you trouble, you might just
> stick with GestureTapDown + GestureShowPress and treat this patch as a
> refactoring, adding the other gesture types in a followup patch.

Done.

[jdduke (slow), 2013-11-05 05:57:17]

https://codereview.chromium.org/44983003/diff/210001/content/browser/renderer...
File content/browser/renderer_host/input/gesture_event_filter_unittest.cc
(left):

https://codereview.chromium.org/44983003/diff/210001/content/browser/renderer...
content/browser/renderer_host/input/gesture_event_filter_unittest.cc:729: //
wait for ACKs.
I'd feel a bit more comfortable if we had some synchronous ack test coverage for
the following scenarios:

(Queue) -> (Ack Sequence)
1) Event -> Sync Ack
2) Event + Event -> Async Ack + Sync Ack
3) Event + Event + Event -> Async Ack + Sync Ack + Sync Ack

and/or whatever other sequences you think might be useful.  Some of that may be
covered by InputRouter tests, but we should have a simple version here as well.

https://codereview.chromium.org/44983003/diff/210001/content/browser/renderer...
File content/browser/renderer_host/input/immediate_input_router.cc (right):

https://codereview.chromium.org/44983003/diff/210001/content/browser/renderer...
content/browser/renderer_host/input/immediate_input_router.cc:336: }

This code should only be called if the event was sent to the renderer.  For the
synchronous handling cases (OverscrollController + Client) we may get valid
acks, in which case we should use those instead of the synthetic IGNORED ones. 
Please move to |OfferToRenderer()|.

[tdresser, 2013-11-05 18:57:02]

https://codereview.chromium.org/44983003/diff/210001/content/browser/renderer...
File content/browser/renderer_host/input/gesture_event_filter_unittest.cc
(left):

https://codereview.chromium.org/44983003/diff/210001/content/browser/renderer...
content/browser/renderer_host/input/gesture_event_filter_unittest.cc:729: //
wait for ACKs.
On 2013/11/05 05:57:18, jdduke wrote:
> I'd feel a bit more comfortable if we had some synchronous ack test coverage
for
> the following scenarios:
> 
> (Queue) -> (Ack Sequence)
> 1) Event -> Sync Ack

I'm not sure how to test this sequence. The first event bypasses
|SendGestureEventImmediately|, which is the only reasonable place to fire the
synchronous ack, if I understand correctly.

> 2) Event + Event -> Async Ack + Sync Ack
Done
> 3) Event + Event + Event -> Async Ack + Sync Ack + Sync Ack
Done
> 
> and/or whatever other sequences you think might be useful.  Some of that may
be
> covered by InputRouter tests, but we should have a simple version here as
well.
>

https://codereview.chromium.org/44983003/diff/210001/content/browser/renderer...
File content/browser/renderer_host/input/immediate_input_router.cc (right):

https://codereview.chromium.org/44983003/diff/210001/content/browser/renderer...
content/browser/renderer_host/input/immediate_input_router.cc:336: }
On 2013/11/05 05:57:18, jdduke wrote:
> 
> This code should only be called if the event was sent to the renderer.  For
the
> synchronous handling cases (OverscrollController + Client) we may get valid
> acks, in which case we should use those instead of the synthetic IGNORED ones.

> Please move to |OfferToRenderer()|.

Done.

[jdduke (slow), 2013-11-05 19:12:18]

https://codereview.chromium.org/44983003/diff/210001/content/browser/renderer...
File content/browser/renderer_host/input/gesture_event_filter_unittest.cc
(left):

https://codereview.chromium.org/44983003/diff/210001/content/browser/renderer...
content/browser/renderer_host/input/gesture_event_filter_unittest.cc:729: //
wait for ACKs.
On 2013/11/05 18:57:02, tdresser wrote:
> On 2013/11/05 05:57:18, jdduke wrote:
> > I'd feel a bit more comfortable if we had some synchronous ack test coverage
> for
> > the following scenarios:
> > 
> > (Queue) -> (Ack Sequence)
> > 1) Event -> Sync Ack
> 
> I'm not sure how to test this sequence. The first event bypasses
> |SendGestureEventImmediately|, which is the only reasonable place to fire the
> synchronous ack, if I understand correctly.
> 
> > 2) Event + Event -> Async Ack + Sync Ack
> Done
> > 3) Event + Event + Event -> Async Ack + Sync Ack + Sync Ack
> Done
> > 
> > and/or whatever other sequences you think might be useful.  Some of that may
> be
> > covered by InputRouter tests, but we should have a simple version here as
> well.
> > 
> 

Hmm, right, then could you change |SimulateGestureEvent()| so that it calls
|SendGestureEventImmediately()|:

if (filter()->ShouldForward(gesture_with_latency)) {
  SendGestureEventImmediately(gesture_with_latency);
  return true;
}

[tdresser, 2013-11-05 20:40:12]

Added a test for a single event with a synchronous ack.

[tdresser, 2013-11-07 13:03:30]

On 2013/11/05 20:40:12, tdresser wrote:
> Added a test for a single event with a synchronous ack.

jdduke@, how does this look?

[jdduke (slow), 2013-11-07 15:57:42]

https://codereview.chromium.org/44983003/diff/510002/content/browser/renderer...
File content/browser/renderer_host/input/gesture_event_filter_unittest.cc
(right):

https://codereview.chromium.org/44983003/diff/510002/content/browser/renderer...
content/browser/renderer_host/input/gesture_event_filter_unittest.cc:55:
SendInputEventACK(event.event.type, *ack_result);

This is a bit odd, why would sending a gesture trigger multiple synchronous
acks? To make this simpler, just keep the scoped_ptr |sync_ack_result_|, but
don't reset it when you |SendInputEventAck()|.  So the sync_ack stays until it's
reset manually by the test.  That should cover the cases we care about here.

https://codereview.chromium.org/44983003/diff/510002/content/browser/renderer...
content/browser/renderer_host/input/gesture_event_filter_unittest.cc:594:
EXPECT_EQ(0U, GestureEventQueueSize());

Please make sure we get the ack, "GetAndResetAckedGestureEventCount()", also in
tests below.

https://codereview.chromium.org/44983003/diff/510002/content/browser/renderer...
content/browser/renderer_host/input/gesture_event_filter_unittest.cc:595: }

Could we have a test similar to "SimpleSyncAck" where the synchronous ack
triggers an additionally queued event in the call to |OnGestureEventAck()|? So
you'd have a scoped_ptr<WebGestureEvent> sync_followup_event_, and you'll need
to Pass()/reset() it before queueing to avoid an infinite loop.

https://codereview.chromium.org/44983003/diff/510002/content/browser/renderer...
File content/browser/renderer_host/input/immediate_input_router_unittest.cc
(right):

https://codereview.chromium.org/44983003/diff/510002/content/browser/renderer...
content/browser/renderer_host/input/immediate_input_router_unittest.cc:132:
unsigned GestureEventQueueSize() {

Nit: size_t

[tdresser, 2013-11-07 22:12:00]

https://codereview.chromium.org/44983003/diff/510002/content/browser/renderer...
File content/browser/renderer_host/input/gesture_event_filter_unittest.cc
(right):

https://codereview.chromium.org/44983003/diff/510002/content/browser/renderer...
content/browser/renderer_host/input/gesture_event_filter_unittest.cc:55:
SendInputEventACK(event.event.type, *ack_result);
On 2013/11/07 15:57:42, jdduke wrote:
> 
> This is a bit odd, why would sending a gesture trigger multiple synchronous
> acks? To make this simpler, just keep the scoped_ptr |sync_ack_result_|, but
> don't reset it when you |SendInputEventAck()|.  So the sync_ack stays until
it's
> reset manually by the test.  That should cover the cases we care about here.

I think it's clearer to explicitly set |sync_ack_result_| each time.
I've fixed the tests to only deal with one sync ack at a time.

https://codereview.chromium.org/44983003/diff/510002/content/browser/renderer...
content/browser/renderer_host/input/gesture_event_filter_unittest.cc:594:
EXPECT_EQ(0U, GestureEventQueueSize());
On 2013/11/07 15:57:42, jdduke wrote:
> 
> Please make sure we get the ack, "GetAndResetAckedGestureEventCount()", also
in
> tests below.

Done.

https://codereview.chromium.org/44983003/diff/510002/content/browser/renderer...
content/browser/renderer_host/input/gesture_event_filter_unittest.cc:595: }
On 2013/11/07 15:57:42, jdduke wrote:
> 
> Could we have a test similar to "SimpleSyncAck" where the synchronous ack
> triggers an additionally queued event in the call to |OnGestureEventAck()|? So
> you'd have a scoped_ptr<WebGestureEvent> sync_followup_event_, and you'll need
> to Pass()/reset() it before queueing to avoid an infinite loop.

Can you clarify what you mean by "triggers an additionally queued event"?
Do you mean that the sync ack queues an additional event, which will then need
to be acked?

That's what I've implemented, but I'm not sure I've interpreted you correctly.

https://codereview.chromium.org/44983003/diff/510002/content/browser/renderer...
File content/browser/renderer_host/input/immediate_input_router_unittest.cc
(right):

https://codereview.chromium.org/44983003/diff/510002/content/browser/renderer...
content/browser/renderer_host/input/immediate_input_router_unittest.cc:132:
unsigned GestureEventQueueSize() {
On 2013/11/07 15:57:42, jdduke wrote:
> 
> Nit: size_t

Done.

[jdduke (slow), 2013-11-07 23:12:52]

https://codereview.chromium.org/44983003/diff/710001/content/browser/renderer...
File content/browser/renderer_host/input/gesture_event_filter.h (right):

https://codereview.chromium.org/44983003/diff/710001/content/browser/renderer...
content/browser/renderer_host/input/gesture_event_filter.h:95: friend class
ImmediateInputRouterTest;

I think we can test sync ack behavior in the ImmediateInputRouterTest without
needing to know the internals of the GEF.  Just make sure we get the right
number of sent events and acked events.

https://codereview.chromium.org/44983003/diff/710001/content/browser/renderer...
File content/browser/renderer_host/input/gesture_event_filter_unittest.cc
(right):

https://codereview.chromium.org/44983003/diff/710001/content/browser/renderer...
content/browser/renderer_host/input/gesture_event_filter_unittest.cc:677:
SimulateGestureEvent(WebInputEvent::GestureScrollUpdate,

There's no reason for this test if the second sync ack isn't triggered directly
by the first sync ack.  As it stands this is just a combination of the two
previous tests.  You can either remove it or allow for "SyncAck->Send->SyncAck".

https://codereview.chromium.org/44983003/diff/710001/content/browser/renderer...
File content/browser/renderer_host/input/immediate_input_router_unittest.cc
(right):

https://codereview.chromium.org/44983003/diff/710001/content/browser/renderer...
content/browser/renderer_host/input/immediate_input_router_unittest.cc:574:
EXPECT_EQ(0U, GestureEventQueueSize());

You can just delete all the "GestureEventQueueSize()" queries here, we have
enough coverage between Sent and Ack'ed counts to check for the correct
behavior.

[jdduke (slow), 2013-11-07 23:52:31]

lgtm with the nits from above.  Could you also edit the description to wrap at
80 characters?

[tdresser, 2013-11-08 16:20:34]

Description wrapped at 80 chars.

https://codereview.chromium.org/44983003/diff/710001/content/browser/renderer...
File content/browser/renderer_host/input/gesture_event_filter.h (right):

https://codereview.chromium.org/44983003/diff/710001/content/browser/renderer...
content/browser/renderer_host/input/gesture_event_filter.h:95: friend class
ImmediateInputRouterTest;
On 2013/11/07 23:12:52, jdduke wrote:
> 
> I think we can test sync ack behavior in the ImmediateInputRouterTest without
> needing to know the internals of the GEF.  Just make sure we get the right
> number of sent events and acked events.

Done.

https://codereview.chromium.org/44983003/diff/710001/content/browser/renderer...
File content/browser/renderer_host/input/gesture_event_filter_unittest.cc
(right):

https://codereview.chromium.org/44983003/diff/710001/content/browser/renderer...
content/browser/renderer_host/input/gesture_event_filter_unittest.cc:677:
SimulateGestureEvent(WebInputEvent::GestureScrollUpdate,
On 2013/11/07 23:12:52, jdduke wrote:
> 
> There's no reason for this test if the second sync ack isn't triggered
directly
> by the first sync ack.  As it stands this is just a combination of the two
> previous tests.  You can either remove it or allow for
"SyncAck->Send->SyncAck".
> 

Removed.

https://codereview.chromium.org/44983003/diff/710001/content/browser/renderer...
File content/browser/renderer_host/input/immediate_input_router_unittest.cc
(right):

https://codereview.chromium.org/44983003/diff/710001/content/browser/renderer...
content/browser/renderer_host/input/immediate_input_router_unittest.cc:574:
EXPECT_EQ(0U, GestureEventQueueSize());
On 2013/11/07 23:12:52, jdduke wrote:
> 
> You can just delete all the "GestureEventQueueSize()" queries here, we have
> enough coverage between Sent and Ack'ed counts to check for the correct
> behavior.

Done.

[commit-bot: I haz the power, 2013-11-08 16:22:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdresser@chromium.org/44983003/980001

[commit-bot: I haz the power, 2013-11-08 18:43:17]

Change committed as 233946