Make tapDown async.

Make tapDown async.

The GestureEventFilter now ignores ACKs for GestureTapDown
events. Whenever a GestureTapDown event reaches the front of the
queue, it is immediately fired and removed from the queue.

BUG=288078
TEST=ImmediateInputRouterTest.GestureTapDownIsAsynch, ImmediateInputRouterTest.GestureTapDownIsInOrder

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

[jochen (gone - plz use gerrit), 2013-09-30 15:19:03]

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

https://codereview.chromium.org/25268002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/gesture_event_filter.cc:453: if
(GestureEventFilter::IsGestureEventTypeAsync(
nit. can you invert the condition so you can return early? i.e.

gesture_event = coalesced_gesture_events_.front();
if (!GestureEventFilter::IsGestureEventTypeAsync(gesture_event))
  return;

[tdresser, 2013-09-30 15:34:47]

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

https://codereview.chromium.org/25268002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/gesture_event_filter.cc:453: if
(GestureEventFilter::IsGestureEventTypeAsync(
On 2013/09/30 15:19:03, jochen wrote:
> nit. can you invert the condition so you can return early? i.e.
> 
> gesture_event = coalesced_gesture_events_.front();
> if (!GestureEventFilter::IsGestureEventTypeAsync(gesture_event))
>   return;
> 

Good call, thanks. Done.

[jdduke (slow), 2013-09-30 15:42:55]

I think this will be a nice addition, though I am a little concerned if the
client/view might require/desire an ack for an async event type. 

That's the main reason I had originally thought a flag on the event might be
sufficient (in which case, the renderer could simply never send the ack), but we
could always adopt that model if the need arises.

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

https://codereview.chromium.org/25268002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/gesture_event_filter.cc:251: return;
As I mention below, we might be able to use a return value here and have the GEF
take be the sole checker of this condition.

https://codereview.chromium.org/25268002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/gesture_event_filter.cc:453: if
(GestureEventFilter::IsGestureEventTypeAsync(
On 2013/09/30 15:19:03, jochen wrote:
> nit. can you invert the condition so you can return early? i.e.
> 
> gesture_event = coalesced_gesture_events_.front();
> if (!GestureEventFilter::IsGestureEventTypeAsync(gesture_event))
>   return;
> 

+1

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

https://codereview.chromium.org/25268002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/gesture_event_filter.h:80: static bool
IsGestureEventTypeAsync(int type);
Please use the enum type directly, WebInputEvent::Type.

https://codereview.chromium.org/25268002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/gesture_event_filter.h:151: void
SendAsynchEvents();
nit: SendAsyncEvents();

Hmm, actually, there are a few "asynch" additions in the patch, could you change
to "async"?

https://codereview.chromium.org/25268002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/gesture_event_filter.h:192: // is awaiting
an ACK. All other events have yet to be sent.
I don't know that this is always true, e.g., we sometimes send a
ScrollBy/PinchBy gesture pair, ignoring one of the returned acks.  Might be
worth a mention (something about |ignore_next_ack_|)?

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

https://codereview.chromium.org/25268002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/immediate_input_router.cc:547: return;
Are we sure that the |ack_handler_| will never be interested in the ack for an
async event type?

I also wonder if we could shift this logic into the GEF... perhaps reverse the
order of the two calls below, so we start with something like

if (gesture_event_filter_->ProcessGestureAck(processed, type)) {
  ack_handler_->OnGestureEventAck(
      gesture_event_filter_->GetGestureEventAwaitingAck(), ack_result);
}

It needn't be a bool return value, perhaps an enum like 

enum GestureAckResult {
  FORWARD,
  DROP
}

Either way, we'd need to make sure the order of the ack processing (between
filter and client) isn't important here, but it would be nice to have the logic
self-contained in the GEF if possible.

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

https://codereview.chromium.org/25268002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/immediate_input_router_unittest.cc:958: //
Test that GestureTapDown events don't wait for ACKs.
Could you file a bug that GestureEventFilter tests should be factored out from
ImmediateInputRouterTest? And link to that bug in a comment above the first GEF
test here? Thanks.

https://codereview.chromium.org/25268002/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/overscroll_controller.cc (right):

https://codereview.chromium.org/25268002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/overscroll_controller.cc:108: if
(GestureEventFilter::IsGestureEventTypeAsync(event.type))
Make this a DCHECK(!GestureEventFilter::IsGestureEventTypeAsync(event.type)); if
the ack has made it to the OverscrollController, we have a problem.

[tdresser, 2013-09-30 18:05:02]

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

https://codereview.chromium.org/25268002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/gesture_event_filter.h:80: static bool
IsGestureEventTypeAsync(int type);
On 2013/09/30 15:42:56, jdduke wrote:
> Please use the enum type directly, WebInputEvent::Type.

Done.

https://codereview.chromium.org/25268002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/gesture_event_filter.h:151: void
SendAsynchEvents();
On 2013/09/30 15:42:56, jdduke wrote:
> nit: SendAsyncEvents();
> 
> Hmm, actually, there are a few "asynch" additions in the patch, could you
change
> to "async"?

Done.

https://codereview.chromium.org/25268002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/gesture_event_filter.h:192: // is awaiting
an ACK. All other events have yet to be sent.
On 2013/09/30 15:42:56, jdduke wrote:
> I don't know that this is always true, e.g., we sometimes send a
> ScrollBy/PinchBy gesture pair, ignoring one of the returned acks.  Might be
> worth a mention (something about |ignore_next_ack_|)?

Done. Can you double check that the new comment is correct?

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

https://codereview.chromium.org/25268002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/immediate_input_router.cc:547: return;
On 2013/09/30 15:42:56, jdduke wrote:
> Are we sure that the |ack_handler_| will never be interested in the ack for an
> async event type?

I believe this is true. Acks for async event types could come back completely
out of order, so depending on them seems dangerous. I think eventually we should
stop the renderer from even firing acks for async events. Can you think of any
instance where we'd want to pay attention to an ack from an async event?

> 
> I also wonder if we could shift this logic into the GEF... perhaps reverse the
> order of the two calls below, so we start with something like
> 
> if (gesture_event_filter_->ProcessGestureAck(processed, type)) {
>   ack_handler_->OnGestureEventAck(
>       gesture_event_filter_->GetGestureEventAwaitingAck(), ack_result);
> }
> 
> It needn't be a bool return value, perhaps an enum like 
> 
> enum GestureAckResult {
>   FORWARD,
>   DROP
> }
> 
> Either way, we'd need to make sure the order of the ack processing (between
> filter and client) isn't important here, but it would be nice to have the
logic
> self-contained in the GEF if possible. 

The order of the ack processing does matter, so unfortunately I don't think we
can centralize that logic.

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

https://codereview.chromium.org/25268002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/immediate_input_router_unittest.cc:958: //
Test that GestureTapDown events don't wait for ACKs.
On 2013/09/30 15:42:56, jdduke wrote:
> Could you file a bug that GestureEventFilter tests should be factored out from
> ImmediateInputRouterTest? And link to that bug in a comment above the first
GEF
> test here? Thanks.

Done.

https://codereview.chromium.org/25268002/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/overscroll_controller.cc (right):

https://codereview.chromium.org/25268002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/overscroll_controller.cc:108: if
(GestureEventFilter::IsGestureEventTypeAsync(event.type))
On 2013/09/30 15:42:56, jdduke wrote:
> Make this a DCHECK(!GestureEventFilter::IsGestureEventTypeAsync(event.type));
if
> the ack has made it to the OverscrollController, we have a problem.

Done.

[tdresser, 2013-10-01 15:31:34]

Rob, could you take a quick look at this to ensure the GestureEventFilter
changes make sense?

[rjkroege, 2013-10-01 16:50:52]

lgtm

[jdduke (slow), 2013-10-01 17:16:14]

lgtm with a couple nits.

https://codereview.chromium.org/25268002/diff/18001/content/browser/renderer_...
File content/browser/renderer_host/input/gesture_event_filter.h (right):

https://codereview.chromium.org/25268002/diff/18001/content/browser/renderer_...
content/browser/renderer_host/input/gesture_event_filter.h:151: void
SendAsyncEvents();
Could you put  a short note here indicating what this does, e.g. "Pops and sends
async events from the head of the queue until a non-async event is reached.", or
even just rename to "SendAsyncEventsFromQueueHead()".

https://codereview.chromium.org/25268002/diff/18001/content/browser/renderer_...
File content/browser/renderer_host/input/immediate_input_router.cc (right):

https://codereview.chromium.org/25268002/diff/18001/content/browser/renderer_...
content/browser/renderer_host/input/immediate_input_router.cc:547: return;
As we discussed, please add a comment here indicating the importance of order
for ack dispatch to |ack_handler_| and |gesture_event_filter_|, so somebody
doesn't fiddle with this condition.

[tdresser, 2013-10-01 17:54:36]

https://codereview.chromium.org/25268002/diff/18001/content/browser/renderer_...
File content/browser/renderer_host/input/gesture_event_filter.h (right):

https://codereview.chromium.org/25268002/diff/18001/content/browser/renderer_...
content/browser/renderer_host/input/gesture_event_filter.h:151: void
SendAsyncEvents();
On 2013/10/01 17:16:15, jdduke wrote:
> Could you put  a short note here indicating what this does, e.g. "Pops and
sends
> async events from the head of the queue until a non-async event is reached.",
or
> even just rename to "SendAsyncEventsFromQueueHead()".

Done.

https://codereview.chromium.org/25268002/diff/18001/content/browser/renderer_...
File content/browser/renderer_host/input/immediate_input_router.cc (right):

https://codereview.chromium.org/25268002/diff/18001/content/browser/renderer_...
content/browser/renderer_host/input/immediate_input_router.cc:547: return;
On 2013/10/01 17:16:15, jdduke wrote:
> As we discussed, please add a comment here indicating the importance of order
> for ack dispatch to |ack_handler_| and |gesture_event_filter_|, so somebody
> doesn't fiddle with this condition.

Done.

[jochen (gone - plz use gerrit), 2013-10-02 13:27:14]

lgtm

[commit-bot: I haz the power, 2013-10-02 13:29:02]

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

[commit-bot: I haz the power, 2013-10-02 15:30:03]

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

[commit-bot: I haz the power, 2013-10-02 15:32:45]

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

[commit-bot: I haz the power, 2013-10-02 17:26:32]

Change committed as 226490