Don't pass touches to gesture recognizer for async acks.

Don't pass touches to gesture recognizer for async acks.

Previously, in RenderWidgetHostViewAura, upon receipt of a touch ack,
we would convert the WebTouchEvent associated with the ack into
ui::TouchEvents.

With the eager GR, this is no longer necessary.

BUG=407645
TEST=GestureRecognizerTest.*

Committed: https://crrev.com/b1663498ac1d59fc3baa085c155f84a1726ca87d
Cr-Commit-Position: refs/heads/master@{#317072}

[tdresser, 2014-08-26 18:15:16]

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

[tdresser, 2014-08-26 18:15:16]

Sadrul, can you take a look?

[sadrul, 2014-08-26 20:03:29]

> Previously, in WindowEventDispatcher, upon receipt of a touch ack,
> we would convert the WebTouchEvent associated with the ack into
> ui::TouchEvents.

Do you mean in RenderWidgetHostViewAura?

[tdresser, 2014-08-26 20:04:25]

On 2014/08/26 20:03:29, sadrul wrote:
> > Previously, in WindowEventDispatcher, upon receipt of a touch ack,
> > we would convert the WebTouchEvent associated with the ack into
> > ui::TouchEvents.
> 
> Do you mean in RenderWidgetHostViewAura?

Sorry, fixed.

[sadrul, 2014-08-26 20:09:07]

https://codereview.chromium.org/503883004/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/503883004/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:1151:
host->dispatcher()->ProcessedTouchEvent(window_, result);
With this change, we are going to be ack-ing more times than we do currently.
For example, if there are multiple active touch-points, and we sent an update
for a single touch-point, then upon the ACK, we would call ProcessedTouchEvent()
just once. With this change, we would call this for each touch-point (including
the stationary ones). Would that not cause issues?

[tdresser, 2014-08-27 17:40:24]

https://codereview.chromium.org/503883004/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/503883004/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:1151:
host->dispatcher()->ProcessedTouchEvent(window_, result);
On 2014/08/26 20:09:07, sadrul wrote:
> With this change, we are going to be ack-ing more times than we do currently.
> For example, if there are multiple active touch-points, and we sent an update
> for a single touch-point, then upon the ACK, we would call
ProcessedTouchEvent()
> just once. With this change, we would call this for each touch-point
(including
> the stationary ones). Would that not cause issues?

Easy fix, hard test...

https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/gesture_...
makes this fail silently.

I suppose I could disable that early out for a browsertest, and test it that
way... Any other ideas?

[sadrul, 2014-08-28 13:31:23]

On 2014/08/27 17:40:24, tdresser wrote:
>
https://codereview.chromium.org/503883004/diff/20001/content/browser/renderer...
> File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> 
>
https://codereview.chromium.org/503883004/diff/20001/content/browser/renderer...
> content/browser/renderer_host/render_widget_host_view_aura.cc:1151:
> host->dispatcher()->ProcessedTouchEvent(window_, result);
> On 2014/08/26 20:09:07, sadrul wrote:
> > With this change, we are going to be ack-ing more times than we do
currently.
> > For example, if there are multiple active touch-points, and we sent an
update
> > for a single touch-point, then upon the ACK, we would call
> ProcessedTouchEvent()
> > just once. With this change, we would call this for each touch-point
> (including
> > the stationary ones). Would that not cause issues?
> 
> Easy fix, hard test...
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/gesture_...
> makes this fail silently.
> 
> I suppose I could disable that early out for a browsertest, and test it that
> way... Any other ideas?

You could try installing a mock-dispatcher for the RWHVA and count the number of
times ProcessedTouchEvent() is called.

[sadrul, 2014-08-28 13:40:39]

On 2014/08/27 17:40:24, tdresser wrote:
...
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/gesture_...

Btw, you normally wouldn't refer to 'renderer' from ui/events code, because
'renderer' is a content-concept, and events shouldn't know about it. An
alternate comment could be "Spurious asynchronous touch acks should not trigger
a crash."

[tdresser, 2014-12-03 19:49:52]

https://codereview.chromium.org/503883004/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/503883004/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:1168:
DCHECK(false);
Should be NOTREACHED().

[tdresser, 2015-02-06 17:43:49]

https://codereview.chromium.org/503883004/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/503883004/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:1168:
DCHECK(false);
On 2014/12/03 19:49:52, tdresser wrote:
> Should be NOTREACHED().

Done.

[tdresser, 2015-02-06 17:44:50]

On 2015/02/06 17:43:49, tdresser wrote:
>
https://codereview.chromium.org/503883004/diff/60001/content/browser/renderer...
> File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> 
>
https://codereview.chromium.org/503883004/diff/60001/content/browser/renderer...
> content/browser/renderer_host/render_widget_host_view_aura.cc:1168:
> DCHECK(false);
> On 2014/12/03 19:49:52, tdresser wrote:
> > Should be NOTREACHED().
> 
> Done.

Sadrul, can you take a look?

[sadrul, 2015-02-07 17:25:34]

https://codereview.chromium.org/503883004/diff/120001/ui/aura/test/aura_test_...
File ui/aura/test/aura_test_utils.h (right):

https://codereview.chromium.org/503883004/diff/120001/ui/aura/test/aura_test_...
ui/aura/test/aura_test_utils.h:11: 
nit: maybe use forward declarations instead (at least for consistency with
WindowTreeHost below)?
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Forward_Decla...)

https://codereview.chromium.org/503883004/diff/120001/ui/aura/window_tree_host.h
File ui/aura/window_tree_host.h (right):

https://codereview.chromium.org/503883004/diff/120001/ui/aura/window_tree_hos...
ui/aura/window_tree_host.h:170: 
This should be private

https://codereview.chromium.org/503883004/diff/120001/ui/events/gestures/gest...
File ui/events/gestures/gesture_recognizer.h (right):

https://codereview.chromium.org/503883004/diff/120001/ui/events/gestures/gest...
ui/events/gestures/gesture_recognizer.h:42: virtual Gestures*
AckSyncTouchEvent(const uint64 unique_event_id,
I feel like verifying that we are acking the correct event is more important for
async acks than it is for sync acks, and so I would have thought we would still
want to use the unique-event-id for async acks.

Of course, I also still think that having different interface for sync/async
acks is just weird.

[tdresser, 2015-02-09 15:23:39]

https://codereview.chromium.org/503883004/diff/120001/ui/aura/test/aura_test_...
File ui/aura/test/aura_test_utils.h (right):

https://codereview.chromium.org/503883004/diff/120001/ui/aura/test/aura_test_...
ui/aura/test/aura_test_utils.h:11: 
On 2015/02/07 17:25:34, sadrul wrote:
> nit: maybe use forward declarations instead (at least for consistency with
> WindowTreeHost below)?
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Forward_Decla...)

Done.

https://codereview.chromium.org/503883004/diff/120001/ui/aura/window_tree_host.h
File ui/aura/window_tree_host.h (right):

https://codereview.chromium.org/503883004/diff/120001/ui/aura/window_tree_hos...
ui/aura/window_tree_host.h:170: 
On 2015/02/07 17:25:34, sadrul wrote:
> This should be private

Done.

https://codereview.chromium.org/503883004/diff/120001/ui/events/gestures/gest...
File ui/events/gestures/gesture_recognizer.h (right):

https://codereview.chromium.org/503883004/diff/120001/ui/events/gestures/gest...
ui/events/gestures/gesture_recognizer.h:42: virtual Gestures*
AckSyncTouchEvent(const uint64 unique_event_id,
On 2015/02/07 17:25:34, sadrul wrote:
> I feel like verifying that we are acking the correct event is more important
for
> async acks than it is for sync acks, and so I would have thought we would
still
> want to use the unique-event-id for async acks.
> 
> Of course, I also still think that having different interface for sync/async
> acks is just weird.

Yeah, it may make sense to use unique_event_id for async acks. It'll require
plumbing some extra information through blink, and probably belongs in a
separate patch (which we could land before this one, if you'd prefer).

Just making sure, it wouldn't make you any happier to have 
AckTouchEvent(result, consumer, SYNC_ACK) and 
AckTouchEvent(result, consumer, ASYNC_ACK),
would it?

[sadrul, 2015-02-10 19:10:15]

https://codereview.chromium.org/503883004/diff/120001/ui/events/gestures/gest...
File ui/events/gestures/gesture_recognizer.h (right):

https://codereview.chromium.org/503883004/diff/120001/ui/events/gestures/gest...
ui/events/gestures/gesture_recognizer.h:42: virtual Gestures*
AckSyncTouchEvent(const uint64 unique_event_id,
On 2015/02/09 15:23:39, tdresser wrote:
> On 2015/02/07 17:25:34, sadrul wrote:
> > I feel like verifying that we are acking the correct event is more important
> for
> > async acks than it is for sync acks, and so I would have thought we would
> still
> > want to use the unique-event-id for async acks.
> > 
> > Of course, I also still think that having different interface for sync/async
> > acks is just weird.
> 
> Yeah, it may make sense to use unique_event_id for async acks. It'll require
> plumbing some extra information through blink, and probably belongs in a
> separate patch (which we could land before this one, if you'd prefer).

That should be a separate patch, yeah. I am not sure if you need blink-side
changes (e.g. you could maybe maintain the queue of the unique-ids in RWHVA or
InputRouterImpl).

> 
> Just making sure, it wouldn't make you any happier to have 
> AckTouchEvent(result, consumer, SYNC_ACK) and 
> AckTouchEvent(result, consumer, ASYNC_ACK),
> would it?

Heh, not really :) The caller should tell you what event it received an ack for,
and what the ack was, so 'AckTouchEvent(event-id, consumed)'. The GR has enough
info at hand to verify that the correct touch-event is acked, and return the set
of gesture-events to dispatch. Whether the ack happened sync or async does not
need to be taken into account either by the caller or the GR.

https://codereview.chromium.org/503883004/diff/140001/ui/aura/test/aura_test_...
File ui/aura/test/aura_test_utils.h (right):

https://codereview.chromium.org/503883004/diff/140001/ui/aura/test/aura_test_...
ui/aura/test/aura_test_utils.h:21: void SetHostDispatcher(WindowTreeHost* host,
WindowEventDispatcher* dispatcher);
Document ownership of |dispatcher| (or use scoped_ptr<> to make that more
obvious).

https://codereview.chromium.org/503883004/diff/140001/ui/aura/window_tree_host.h
File ui/aura/window_tree_host.h (right):

https://codereview.chromium.org/503883004/diff/140001/ui/aura/window_tree_hos...
ui/aura/window_tree_host.h:192: scoped_ptr<WindowEventDispatcher> dispatcher_;
Is this change in order necessary? if not, remove. (avoid unnecessary
code-churn)

[tdresser, 2015-02-11 16:10:25]

https://codereview.chromium.org/503883004/diff/120001/ui/events/gestures/gest...
File ui/events/gestures/gesture_recognizer.h (right):

https://codereview.chromium.org/503883004/diff/120001/ui/events/gestures/gest...
ui/events/gestures/gesture_recognizer.h:42: virtual Gestures*
AckSyncTouchEvent(const uint64 unique_event_id,
On 2015/02/10 19:10:15, sadrul wrote:
> On 2015/02/09 15:23:39, tdresser wrote:
> > On 2015/02/07 17:25:34, sadrul wrote:
> > > I feel like verifying that we are acking the correct event is more
important
> > for
> > > async acks than it is for sync acks, and so I would have thought we would
> > still
> > > want to use the unique-event-id for async acks.
> > > 
> > > Of course, I also still think that having different interface for
sync/async
> > > acks is just weird.
> > 
> > Yeah, it may make sense to use unique_event_id for async acks. It'll require
> > plumbing some extra information through blink, and probably belongs in a
> > separate patch (which we could land before this one, if you'd prefer).
> 
> That should be a separate patch, yeah. I am not sure if you need blink-side
> changes (e.g. you could maybe maintain the queue of the unique-ids in RWHVA or
> InputRouterImpl).

True, we could avoid making any blink side changes. There is a theoretical class
of bugs that this would miss (somehow we get two events in the Renderer at the
same time, and ack them out of order) but that's unlikely enough that it
probably makes sense to go for the simpler option. I've filed crbug.com/457669
to keep track of making this change.
> 
> > 
> > Just making sure, it wouldn't make you any happier to have 
> > AckTouchEvent(result, consumer, SYNC_ACK) and 
> > AckTouchEvent(result, consumer, ASYNC_ACK),
> > would it?
> 
> Heh, not really :) The caller should tell you what event it received an ack
for,
> and what the ack was, so 'AckTouchEvent(event-id, consumed)'. The GR has
enough
> info at hand to verify that the correct touch-event is acked, and return the
set
> of gesture-events to dispatch. Whether the ack happened sync or async does not
> need to be taken into account either by the caller or the GR.

Personally I find it somewhat easier to think of synchronous acks and
asynchronous acks as fairly distinct, so this implementation maps pretty well to
my mental model. We definitely can transition to the model you've proposed
though. We'd need a slightly better way of generating unique event ids, but that
would be pretty easy.

Let me know if you think it's worth the time, and I'll file a bug for this. It
should follow pretty smoothly from crbug.com/457669.

https://codereview.chromium.org/503883004/diff/140001/ui/aura/test/aura_test_...
File ui/aura/test/aura_test_utils.h (right):

https://codereview.chromium.org/503883004/diff/140001/ui/aura/test/aura_test_...
ui/aura/test/aura_test_utils.h:21: void SetHostDispatcher(WindowTreeHost* host,
WindowEventDispatcher* dispatcher);
On 2015/02/10 19:10:15, sadrul wrote:
> Document ownership of |dispatcher| (or use scoped_ptr<> to make that more
> obvious).

Done.

https://codereview.chromium.org/503883004/diff/140001/ui/aura/window_tree_host.h
File ui/aura/window_tree_host.h (right):

https://codereview.chromium.org/503883004/diff/140001/ui/aura/window_tree_hos...
ui/aura/window_tree_host.h:192: scoped_ptr<WindowEventDispatcher> dispatcher_;
On 2015/02/10 19:10:15, sadrul wrote:
> Is this change in order necessary? if not, remove. (avoid unnecessary
> code-churn)

Done.

[sadrul, 2015-02-13 01:53:12]

The try failures look relevant. Mind taking a look?

> > The caller should tell you what event it received an ack
> for,
> > and what the ack was, so 'AckTouchEvent(event-id, consumed)'. The GR has
> enough
> > info at hand to verify that the correct touch-event is acked, and return the
> set
> > of gesture-events to dispatch. Whether the ack happened sync or async does
not
> > need to be taken into account either by the caller or the GR.
> 
> Personally I find it somewhat easier to think of synchronous acks and
> asynchronous acks as fairly distinct, so this implementation maps pretty well
to
> my mental model. We definitely can transition to the model you've proposed
> though. We'd need a slightly better way of generating unique event ids, but
that
> would be pretty easy.
> 
> Let me know if you think it's worth the time, and I'll file a bug for this. It
> should follow pretty smoothly from crbug.com/457669.

It may make sense to think of sync/async as distinct when you are working on the
GR internals. But for a GR user, this is an extra decision they need to make
that they could easily get wrong (see e.g.
https://codereview.chromium.org/680413006/diff/280001/ui/chromeos/touch_explo...).

https://codereview.chromium.org/503883004/diff/160001/ui/aura/window_tree_host.h
File ui/aura/window_tree_host.h (right):

https://codereview.chromium.org/503883004/diff/160001/ui/aura/window_tree_hos...
ui/aura/window_tree_host.h:200: #endif  // UI_AURA_WINDOW_TREE_HOST_H_
Are any of the changes in this file necessary?

[tdresser, 2015-02-18 20:05:37]

I've filed crbug.com/459339: "Unify GestureRecognizer::AckAsyncTouchEvent and
AckSyncTouchEvent"

https://codereview.chromium.org/503883004/diff/160001/ui/aura/window_tree_host.h
File ui/aura/window_tree_host.h (right):

https://codereview.chromium.org/503883004/diff/160001/ui/aura/window_tree_hos...
ui/aura/window_tree_host.h:200: #endif  // UI_AURA_WINDOW_TREE_HOST_H_
On 2015/02/13 01:53:12, sadrul wrote:
> Are any of the changes in this file necessary?

Done.

[sadrul, 2015-02-19 17:22:15]

lgtm

[tdresser, 2015-02-19 17:48:02]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2015-02-19 17:48:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/503883004/180001

[commit-bot: I haz the power, 2015-02-19 17:52:52]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-02-19 17:53:57]

Patchset 10 (id:??) landed as
https://crrev.com/b1663498ac1d59fc3baa085c155f84a1726ca87d
Cr-Commit-Position: refs/heads/master@{#317072}