Defer synthetic gesture completions until events have been flushed

Defer synthetic gesture completions until events have been flushed

Previously, synthetic gesture completion callbacks were triggered as soon as
the last generated event was *sent*.  A truer indicator of completion, however,
is when the last generated event was *ack'ed*.  For now, this is accomplished
by blocking the completion notification until all events have been flushed
through the pipeline.

In the future, a better solution will be inserting event
fences with an appropriate callback to indicate completion.  Note that this
approach fails to properly wait until all timeout events have been flushed
(e.g., GestureTap events on desktop sites on Android).

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

[jdduke (slow), 2014-03-28 23:10:06]

dominikg@: PTAL.  I don't feel great about this solution.  I had another version
that hooked into the |InputRouterClient::DidFlush()| callback to trigger a
pending synthetic gesture completed notification.  However, in that case the
async trace event end was still being fired early, and it would be nice to have
both the trace end and the IPC be sent around the same time.

It will require a good deal more work to use event markers to accurately
determine when the touches/gestures for the specific synthetic gesture have been
fully flushed.  I'm thinking we could hook into the trace id's for this... sigh,
this would all be simpler if we'd adopted content::InputEvent already and had
unique id's per event.  Maybe we push this fix, then a longer term fix will
force us to clean up the entire pipeline.

[Dominik Grewe, 2014-04-01 10:09:27]

On 2014/03/28 23:10:06, jdduke wrote:
> dominikg@: PTAL.  I don't feel great about this solution.  I had another
version
> that hooked into the |InputRouterClient::DidFlush()| callback to trigger a
> pending synthetic gesture completed notification.  However, in that case the
> async trace event end was still being fired early, and it would be nice to
have
> both the trace end and the IPC be sent around the same time.

How about we add a SyntheticGestureController::OnDidFlushGesture() (or similar)
callback that's being called by InputRouterClient::DidFlush()?
When the controller has sent the last input event, it request a callback (via
SyntheticGestureTarget I guess) and stops processing gestures until the callback
has been called. In the callback, it stops the trace event and calls the gesture
complete callback.

Btw, is DidFlush hooked up at all? Couldn't find any code that calls it (but
maybe codesearch is broken).

> 
> It will require a good deal more work to use event markers to accurately
> determine when the touches/gestures for the specific synthetic gesture have
been
> fully flushed.  I'm thinking we could hook into the trace id's for this...
sigh,
> this would all be simpler if we'd adopted content::InputEvent already and had
> unique id's per event.  Maybe we push this fix, then a longer term fix will
> force us to clean up the entire pipeline.

[jdduke (slow), 2014-04-01 23:05:47]

OK, this is definitely an improvement.  I'll add a test or two to
InputRouterImpl if you think this is the way to go.

The RWHVFoo::DidFlushInput was originally plumbed for Android, but I don't think
we ever got around to using it.  It can probably be removed until we decide it's
necessary.

[Dominik Grewe, 2014-04-02 09:10:31]

I'm happy with this. Maybe it can be improved once we have unique event IDs as
you suggested, but for now this seems fine to me.

LGTM if you add a test for InputRouterImpl.

https://codereview.chromium.org/217163006/diff/20001/content/browser/renderer...
File
content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc
(right):

https://codereview.chromium.org/217163006/diff/20001/content/browser/renderer...
content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:489:
TEST_F(SyntheticGestureControllerTest, GestureCompledOnDidFlushInput) {
typo: GestureCompledOnDidFlushInput -> GestureCompletedOnDidFlushInput

[jdduke (slow), 2014-04-02 20:15:06]

sadrul@: Owner review for render_widget_host_{impl,unittest}.cc? Thanks.

[sadrul, 2014-04-02 20:39:22]

lgtm

[jdduke (slow), 2014-04-03 01:01:36]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-04-03 01:11:22]

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

[commit-bot: I haz the power, 2014-04-03 08:33:00]

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

[commit-bot: I haz the power, 2014-04-03 08:33:00]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[jdduke (slow), 2014-04-03 13:24:51]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-04-03 13:24:54]

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

[commit-bot: I haz the power, 2014-04-03 14:41:45]

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

[commit-bot: I haz the power, 2014-04-03 14:41:46]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[jdduke (slow), 2014-04-03 15:04:43]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-04-03 15:06:07]

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

[commit-bot: I haz the power, 2014-04-03 16:05:04]

Change committed as 261429