Re-enable Eager Gesture Recognition on Aura

Re-enable Eager Gesture Recognition on Aura

This enables eager gesture recognition on Aura for the unified gesture
detector.

The previous attempt at enabling eager gesture recognition
(crrev.com/393953012) failed.
See crbug.com/408594 for details.

See https://codereview.chromium.org/551373006/ for some additional history.

BUG=332418
TEST=GestureRecognizerTest.{*,EagerGestureDetection}
Also manually tested on Cros and Windows.

Committed: https://crrev.com/8879cfca3b19c304004e34e6dec9629752cd3068
Cr-Commit-Position: refs/heads/master@{#307548}

[tdresser, 2014-11-11 21:16:48]

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

[tdresser, 2014-11-11 21:16:49]

I've tested this pretty thoroughly manually on Windows and Chrome OS.

GestureRecognizerTest.IgnoredEventsDontBreakGestureRecognition tests the way in
which Windows was broken in PS#2- I had modified that test incorrectly for PS#2.

How does this look to you?

[jdduke (slow), 2014-11-12 00:04:52]

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

https://codereview.chromium.org/680413006/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:2010: // event
does not get processed by the gesture recognizer before the events in
Could you split this sentence apart and perhaps tighten up the phrasing?

Perhaps "...even if no point has been updated. This ensures that..."

Then maybe "...before events currently awaiting dispatch in the touch queue"?

https://codereview.chromium.org/680413006/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:2019:
window_->GetHost()->dispatcher()->ProcessedTouchEvent(event, window_,
I've been getting this arg packing style recently as well... Is this a genuine
style change or is our collective clang-format borked?

[tdresser, 2014-11-12 14:51:25]

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

https://codereview.chromium.org/680413006/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:2010: // event
does not get processed by the gesture recognizer before the events in
On 2014/11/12 00:04:52, jdduke wrote:
> Could you split this sentence apart and perhaps tighten up the phrasing?
> 
> Perhaps "...even if no point has been updated. This ensures that..."
> 
> Then maybe "...before events currently awaiting dispatch in the touch queue"?

Done.

https://codereview.chromium.org/680413006/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:2019:
window_->GetHost()->dispatcher()->ProcessedTouchEvent(event, window_,
On 2014/11/12 00:04:52, jdduke wrote:
> I've been getting this arg packing style recently as well... Is this a genuine
> style change or is our collective clang-format borked?

It doesn't appear to be against the style guide.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Calls

[tdresser, 2014-11-17 13:32:39]

On 2014/11/12 14:51:25, tdresser wrote:
>
https://codereview.chromium.org/680413006/diff/40001/content/browser/renderer...
> File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> 
>
https://codereview.chromium.org/680413006/diff/40001/content/browser/renderer...
> content/browser/renderer_host/render_widget_host_view_aura.cc:2010: // event
> does not get processed by the gesture recognizer before the events in
> On 2014/11/12 00:04:52, jdduke wrote:
> > Could you split this sentence apart and perhaps tighten up the phrasing?
> > 
> > Perhaps "...even if no point has been updated. This ensures that..."
> > 
> > Then maybe "...before events currently awaiting dispatch in the touch
queue"?
> 
> Done.
> 
>
https://codereview.chromium.org/680413006/diff/40001/content/browser/renderer...
> content/browser/renderer_host/render_widget_host_view_aura.cc:2019:
> window_->GetHost()->dispatcher()->ProcessedTouchEvent(event, window_,
> On 2014/11/12 00:04:52, jdduke wrote:
> > I've been getting this arg packing style recently as well... Is this a
genuine
> > style change or is our collective clang-format borked?
> 
> It doesn't appear to be against the style guide.
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Calls

Ping.

[jdduke (slow), 2014-11-17 16:57:14]

I'm not an owner for any of these files =/

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

https://codereview.chromium.org/680413006/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:2016: // Forward
the touch event only if a touch point was updated, and
Would it make any sense to split out the point is null case here?

if (!point) {
  Do whatever.
  return;
}

https://codereview.chromium.org/680413006/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:2031:
window_->GetHost()->dispatcher()->ProcessedTouchEvent(event, window_,
It seems like this could cause us to get some out-of-order acks if |point| is
null. Is that intentional?

https://codereview.chromium.org/680413006/diff/60001/ui/events/event_constants.h
File ui/events/event_constants.h (right):

https://codereview.chromium.org/680413006/diff/60001/ui/events/event_constant...
ui/events/event_constants.h:129: 1 << 2,  // The event shouldn't be handled
synchronously.
This comment doesn't really add anything, maybe flesh it out to state when/why
this can happen?

[tdresser, 2014-11-17 22:00:36]

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

https://codereview.chromium.org/680413006/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:2016: // Forward
the touch event only if a touch point was updated, and
On 2014/11/17 16:57:13, jdduke wrote:
> Would it make any sense to split out the point is null case here?
> 
> if (!point) {
>   Do whatever.
>   return;
> }

Done.
Feels about 1% worse this way to me - what do you think?

https://codereview.chromium.org/680413006/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:2031:
window_->GetHost()->dispatcher()->ProcessedTouchEvent(event, window_,
On 2014/11/17 16:57:13, jdduke wrote:
> It seems like this could cause us to get some out-of-order acks if |point| is
> null. Is that intentional?

Gah, you're right, that could potentially break things.

I'm having a hard time coming up with any solution to this that isn't
horrendously complicated - for instance, telling the GR "The touch event that
you received most recently should be marked consumed. When it gets to the top of
the queue, just act as though you received an ack indicating that the event was
consumed."

Do you have any thoughts?

https://codereview.chromium.org/680413006/diff/60001/ui/events/event_constants.h
File ui/events/event_constants.h (right):

https://codereview.chromium.org/680413006/diff/60001/ui/events/event_constant...
ui/events/event_constants.h:129: 1 << 2,  // The event shouldn't be handled
synchronously.
On 2014/11/17 16:57:13, jdduke wrote:
> This comment doesn't really add anything, maybe flesh it out to state when/why
> this can happen?

Done.

[jdduke (slow), 2014-11-17 22:23:15]

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

https://codereview.chromium.org/680413006/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:2016: // Forward
the touch event only if a touch point was updated, and
On 2014/11/17 22:00:36, tdresser wrote:
> On 2014/11/17 16:57:13, jdduke wrote:
> > Would it make any sense to split out the point is null case here?
> > 
> > if (!point) {
> >   Do whatever.
> >   return;
> > }
> 
> Done.
> Feels about 1% worse this way to me - what do you think?

I was hoping we could get rid of the |DisableSynchronousHandling();| if point is
null, so maybe the old way is better...

https://codereview.chromium.org/680413006/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:2031:
window_->GetHost()->dispatcher()->ProcessedTouchEvent(event, window_,
On 2014/11/17 22:00:36, tdresser wrote:
> On 2014/11/17 16:57:13, jdduke wrote:
> > It seems like this could cause us to get some out-of-order acks if |point|
is
> > null. Is that intentional?
> 
> Gah, you're right, that could potentially break things.
> 
> I'm having a hard time coming up with any solution to this that isn't
> horrendously complicated - for instance, telling the GR "The touch event that
> you received most recently should be marked consumed. When it gets to the top
of
> the queue, just act as though you received an ack indicating that the event
was
> consumed."
> 
> Do you have any thoughts?
> 
> 

Well, ideally this method would be: |OnTouchEvent(const ui::MotionEvent)|, then
we don't have to keep any intermediate state and we can just convert the
MotionEvent to a WebTouchEvent directly.  How difficult would it be to wire that
up?

Short of that, shouldn't these ui::TouchEvents already be validated by the eager
pipeline? So shouldn't we be DCHECK'ing? Or are there cases where
extra/redundant events might be received (like an extra cancel)?

[tdresser, 2014-11-18 13:20:28]

On 2014/11/17 22:23:15, jdduke wrote:
>
https://codereview.chromium.org/680413006/diff/60001/content/browser/renderer...
> File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> 
>
https://codereview.chromium.org/680413006/diff/60001/content/browser/renderer...
> content/browser/renderer_host/render_widget_host_view_aura.cc:2016: // Forward
> the touch event only if a touch point was updated, and
> On 2014/11/17 22:00:36, tdresser wrote:
> > On 2014/11/17 16:57:13, jdduke wrote:
> > > Would it make any sense to split out the point is null case here?
> > > 
> > > if (!point) {
> > >   Do whatever.
> > >   return;
> > > }
> > 
> > Done.
> > Feels about 1% worse this way to me - what do you think?
> 
> I was hoping we could get rid of the |DisableSynchronousHandling();| if point
is
> null, so maybe the old way is better...
> 
>
https://codereview.chromium.org/680413006/diff/60001/content/browser/renderer...
> content/browser/renderer_host/render_widget_host_view_aura.cc:2031:
> window_->GetHost()->dispatcher()->ProcessedTouchEvent(event, window_,
> On 2014/11/17 22:00:36, tdresser wrote:
> > On 2014/11/17 16:57:13, jdduke wrote:
> > > It seems like this could cause us to get some out-of-order acks if |point|
> is
> > > null. Is that intentional?
> > 
> > Gah, you're right, that could potentially break things.
> > 
> > I'm having a hard time coming up with any solution to this that isn't
> > horrendously complicated - for instance, telling the GR "The touch event
that
> > you received most recently should be marked consumed. When it gets to the
top
> of
> > the queue, just act as though you received an ack indicating that the event
> was
> > consumed."
> > 
> > Do you have any thoughts?
> > 
> > 
> 
> Well, ideally this method would be: |OnTouchEvent(const ui::MotionEvent)|,
then
> we don't have to keep any intermediate state and we can just convert the
> MotionEvent to a WebTouchEvent directly.  How difficult would it be to wire
that
> up?
> 
> Short of that, shouldn't these ui::TouchEvents already be validated by the
eager
> pipeline? So shouldn't we be DCHECK'ing? Or are there cases where
> extra/redundant events might be received (like an extra cancel)?

The problem is that between the Window level validation and the web contents
level validation, there are all kinds of things that can interfere and
invalidate the event stream. We could try to find every case where this occurs,
but it would be difficult to ensure that we got all of them. Perhaps we can
eventually accomplish this with the event stream validator.

For example (I may have the details wrong here, but the general idea is
correct), on ChromeOS when the system menu is open and you swipe on web
contents, the touchstart is filtered out by the menu, but the rest of the touch
sequence is still targeted to the web contents, resulting in an event stream
which is valid at the window level (PreDispatch), but invalid once it reaches
the RWHVA. I think this behavior is broken, but I'm not certain how many more
things are broken in this way.

I'm starting to think that my first idea may not actually be that complicated. I
might just try adding a method that indicates that the last touch event should
be marked as consumed, and then store that information in the
GestureEventDataPacket. Whenever a new packet reaches the front of the queue,
we'll check immediately if it has already been marked as consumed.

Does that sound reasonable?

[tdresser, 2014-11-20 19:32:54]

If folks think this is a reasonable approach, I'll add some more granular
testing (in touch_disposition_gesture_filter_unittest.cc).

What do you think?

https://codereview.chromium.org/680413006/diff/100001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_event_data_packet.cc (left):

https://codereview.chromium.org/680413006/diff/100001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_event_data_packet.cc:48:
DCHECK_NE(gesture_source_, UNDEFINED);
I'm not sure if this is an improvement, or if I just _really_ wanted to use
delegated constructors. I did initially miss initializing |marked_to_ignore_| in
one of these constructors, which makes me think it might be a good idea, but
it's also a shame to have to get rid of this DCHECK.

Thoughts?

[jdduke (slow), 2014-11-20 19:49:17]

Hmm, but what about the events that are synchronously handled by some
intermediate consumer between the GR and RWHVAura? Don't we need to ignore them
too? If that's already in this patch I must have missed it.

https://codereview.chromium.org/680413006/diff/100001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/680413006/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2047:
window_->GetHost()->dispatcher()->IgnoreLastTouchEvent(window_);
Is this better than calling
|ui::GestureRecognizer::Get()->IgnoreLastTouchEvent(window_);| directly?

https://codereview.chromium.org/680413006/diff/100001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_event_data_packet.cc (left):

https://codereview.chromium.org/680413006/diff/100001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_event_data_packet.cc:48:
DCHECK_NE(gesture_source_, UNDEFINED);
On 2014/11/20 19:32:54, tdresser wrote:
> I'm not sure if this is an improvement, or if I just _really_ wanted to use
> delegated constructors. I did initially miss initializing |marked_to_ignore_|
in
> one of these constructors, which makes me think it might be a good idea, but
> it's also a shame to have to get rid of this DCHECK.
> 
> Thoughts?

You can keep the DCHECK for the constructor that takes an explicit
GestureSource.

https://codereview.chromium.org/680413006/diff/100001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_event_data_packet.cc (right):

https://codereview.chromium.org/680413006/diff/100001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_event_data_packet.cc:50: gfx::PointF(0, 0),
You can just do gfx::PointF().

https://codereview.chromium.org/680413006/diff/100001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_event_data_packet.cc:60: gestures_ =
other.gestures_;
Why not keep these in the initialize list?

https://codereview.chromium.org/680413006/diff/100001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_event_data_packet.h (right):

https://codereview.chromium.org/680413006/diff/100001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_event_data_packet.h:54: // Check if this
packet should be ignored.
No need for this getter comment.

[tdresser, 2014-11-20 21:02:30]

On 2014/11/20 19:49:17, jdduke wrote:
> Hmm, but what about the events that are synchronously handled by some
> intermediate consumer between the GR and RWHVAura? Don't we need to ignore
them
> too? If that's already in this patch I must have missed it.
> 
>
https://codereview.chromium.org/680413006/diff/100001/content/browser/rendere...
> File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> 
>
https://codereview.chromium.org/680413006/diff/100001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_view_aura.cc:2047:
> window_->GetHost()->dispatcher()->IgnoreLastTouchEvent(window_);
> Is this better than calling
> |ui::GestureRecognizer::Get()->IgnoreLastTouchEvent(window_);| directly?
> 
>
https://codereview.chromium.org/680413006/diff/100001/ui/events/gesture_detec...
> File ui/events/gesture_detection/gesture_event_data_packet.cc (left):
> 
>
https://codereview.chromium.org/680413006/diff/100001/ui/events/gesture_detec...
> ui/events/gesture_detection/gesture_event_data_packet.cc:48:
> DCHECK_NE(gesture_source_, UNDEFINED);
> On 2014/11/20 19:32:54, tdresser wrote:
> > I'm not sure if this is an improvement, or if I just _really_ wanted to use
> > delegated constructors. I did initially miss initializing
|marked_to_ignore_|
> in
> > one of these constructors, which makes me think it might be a good idea, but
> > it's also a shame to have to get rid of this DCHECK.
> > 
> > Thoughts?
> 
> You can keep the DCHECK for the constructor that takes an explicit
> GestureSource.
> 
>
https://codereview.chromium.org/680413006/diff/100001/ui/events/gesture_detec...
> File ui/events/gesture_detection/gesture_event_data_packet.cc (right):
> 
>
https://codereview.chromium.org/680413006/diff/100001/ui/events/gesture_detec...
> ui/events/gesture_detection/gesture_event_data_packet.cc:50: gfx::PointF(0,
0),
> You can just do gfx::PointF().
> 
>
https://codereview.chromium.org/680413006/diff/100001/ui/events/gesture_detec...
> ui/events/gesture_detection/gesture_event_data_packet.cc:60: gestures_ =
> other.gestures_;
> Why not keep these in the initialize list?
> 
>
https://codereview.chromium.org/680413006/diff/100001/ui/events/gesture_detec...
> File ui/events/gesture_detection/gesture_event_data_packet.h (right):
> 
>
https://codereview.chromium.org/680413006/diff/100001/ui/events/gesture_detec...
> ui/events/gesture_detection/gesture_event_data_packet.h:54: // Check if this
> packet should be ignored.
> No need for this getter comment.

Events which are synchronously handled by some intermediate consumer between the
GR and RWHVAura will be marked consumed, so they'll be acked as handled by
WindowEventDispatcher::PostDispatchEvent.

[jdduke (slow), 2014-11-20 21:09:50]

On 2014/11/20 21:02:30, tdresser wrote:
> Events which are synchronously handled by some intermediate consumer between
the
> GR and RWHVAura will be marked consumed, so they'll be acked as handled by
> WindowEventDispatcher::PostDispatchEvent.

Right, but that ack will be out-of-order if there is an async touch sequence in
the queue.

[jdduke (slow), 2014-11-20 21:12:13]

On 2014/11/20 21:09:50, jdduke wrote:
> On 2014/11/20 21:02:30, tdresser wrote:
> > Events which are synchronously handled by some intermediate consumer between
> the
> > GR and RWHVAura will be marked consumed, so they'll be acked as handled by
> > WindowEventDispatcher::PostDispatchEvent.
> 
> Right, but that ack will be out-of-order if there is an async touch sequence
in
> the queue.

I guess I'm just wondering if these gymnastics are better than hosting the GR in
RWHVAura (or thereabouts) for content-targetted touches, with a separate GR for
the synchronous View-handled case (that instance begin non-eager). This is
essentially what Android does; sadrul@, you mentioned you had concerns about
this approach?

[tdresser, 2014-11-20 21:36:52]

On 2014/11/20 21:12:13, jdduke wrote:
> On 2014/11/20 21:09:50, jdduke wrote:
> > On 2014/11/20 21:02:30, tdresser wrote:
> > > Events which are synchronously handled by some intermediate consumer
between
> > the
> > > GR and RWHVAura will be marked consumed, so they'll be acked as handled by
> > > WindowEventDispatcher::PostDispatchEvent.
> > 
> > Right, but that ack will be out-of-order if there is an async touch sequence
> in
> > the queue.
> 
> I guess I'm just wondering if these gymnastics are better than hosting the GR
in
> RWHVAura (or thereabouts) for content-targetted touches, with a separate GR
for
> the synchronous View-handled case (that instance begin non-eager). This is
> essentially what Android does; sadrul@, you mentioned you had concerns about
> this approach?

Oh geez...
Yeah, I think that's an issue with any of the methods I've been looking at. I
was previously playing with only passing touches to the GR once they've hit the
RWHVAura, but I was always using the same GR between web contents and views, so
I would still run into the same problem.

Currently you can perform a system level gesture with some fingers over web
contents and some fingers over views.
For example, you can perform a three finger swipe with one finger in web
contents, and the other two over views.

If we used a separate GR for web contents, we might need to lock each gesture
sequence (first touchstart to last touchrelease) to a single GR.

[tdresser, 2014-11-20 21:57:55]

On 2014/11/20 21:36:52, tdresser wrote:
> On 2014/11/20 21:12:13, jdduke wrote:
> > On 2014/11/20 21:09:50, jdduke wrote:
> > > On 2014/11/20 21:02:30, tdresser wrote:
> > > > Events which are synchronously handled by some intermediate consumer
> between
> > > the
> > > > GR and RWHVAura will be marked consumed, so they'll be acked as handled
by
> > > > WindowEventDispatcher::PostDispatchEvent.
> > > 
> > > Right, but that ack will be out-of-order if there is an async touch
sequence
> > in
> > > the queue.
> > 
> > I guess I'm just wondering if these gymnastics are better than hosting the
GR
> in
> > RWHVAura (or thereabouts) for content-targetted touches, with a separate GR
> for
> > the synchronous View-handled case (that instance begin non-eager). This is
> > essentially what Android does; sadrul@, you mentioned you had concerns about
> > this approach?
> 
> Oh geez...
> Yeah, I think that's an issue with any of the methods I've been looking at. I
> was previously playing with only passing touches to the GR once they've hit
the
> RWHVAura, but I was always using the same GR between web contents and views,
so
> I would still run into the same problem.
> 
> Currently you can perform a system level gesture with some fingers over web
> contents and some fingers over views.
> For example, you can perform a three finger swipe with one finger in web
> contents, and the other two over views.
> 
> If we used a separate GR for web contents, we might need to lock each gesture
> sequence (first touchstart to last touchrelease) to a single GR.

Another option is to give every touch event a unique ID (or package it with the
gestures it's going to create). That would less us associate the acks with the
gestures more consistently.

[tdresser, 2014-11-21 13:12:13]

On 2014/11/20 21:57:55, tdresser wrote:
> On 2014/11/20 21:36:52, tdresser wrote:
> > On 2014/11/20 21:12:13, jdduke wrote:
> > > On 2014/11/20 21:09:50, jdduke wrote:
> > > > On 2014/11/20 21:02:30, tdresser wrote:
> > > > > Events which are synchronously handled by some intermediate consumer
> > between
> > > > the
> > > > > GR and RWHVAura will be marked consumed, so they'll be acked as
handled
> by
> > > > > WindowEventDispatcher::PostDispatchEvent.
> > > > 
> > > > Right, but that ack will be out-of-order if there is an async touch
> sequence
> > > in
> > > > the queue.
> > > 
> > > I guess I'm just wondering if these gymnastics are better than hosting the
> GR
> > in
> > > RWHVAura (or thereabouts) for content-targetted touches, with a separate
GR
> > for
> > > the synchronous View-handled case (that instance begin non-eager). This is
> > > essentially what Android does; sadrul@, you mentioned you had concerns
about
> > > this approach?
> > 
> > Oh geez...
> > Yeah, I think that's an issue with any of the methods I've been looking at.
I
> > was previously playing with only passing touches to the GR once they've hit
> the
> > RWHVAura, but I was always using the same GR between web contents and views,
> so
> > I would still run into the same problem.
> > 
> > Currently you can perform a system level gesture with some fingers over web
> > contents and some fingers over views.
> > For example, you can perform a three finger swipe with one finger in web
> > contents, and the other two over views.
> > 
> > If we used a separate GR for web contents, we might need to lock each
gesture
> > sequence (first touchstart to last touchrelease) to a single GR.
> 
> Another option is to give every touch event a unique ID (or package it with
the
> gestures it's going to create). That would less us associate the acks with the
> gestures more consistently.

On second thought, we already put the web contents in a separate aura::Window.

That means that in the normal case, the GRs handling sync acks and the GRs
handling async acks are already separate.

The question is, when can we get a sync ack over web contents. It's possible
that some system level event handler could intercept the touches between them
reaching the aura::Window and the RWHVAura and give them a synchronous ack. Are
we aware of any cases where that can happen today?

[tdresser, 2014-11-21 13:13:13]

Clarification - the question is: when can we get a sync ack over web contents
while there are touches pending acks in the touch event queue.

[tdresser, 2014-11-21 13:25:22]

This patch is likely irrelevant, but I'm uploading it in case it is usable in
some form.

https://codereview.chromium.org/680413006/diff/100001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/680413006/diff/100001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2047:
window_->GetHost()->dispatcher()->IgnoreLastTouchEvent(window_);
On 2014/11/20 19:49:16, jdduke wrote:
> Is this better than calling
> |ui::GestureRecognizer::Get()->IgnoreLastTouchEvent(window_);| directly?

Done.

I was thinking it was better from a layering perspective, but I've changed my
mind.

https://codereview.chromium.org/680413006/diff/100001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_event_data_packet.cc (left):

https://codereview.chromium.org/680413006/diff/100001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_event_data_packet.cc:48:
DCHECK_NE(gesture_source_, UNDEFINED);
On 2014/11/20 19:49:16, jdduke wrote:
> On 2014/11/20 19:32:54, tdresser wrote:
> > I'm not sure if this is an improvement, or if I just _really_ wanted to use
> > delegated constructors. I did initially miss initializing
|marked_to_ignore_|
> in
> > one of these constructors, which makes me think it might be a good idea, but
> > it's also a shame to have to get rid of this DCHECK.
> > 
> > Thoughts?
> 
> You can keep the DCHECK for the constructor that takes an explicit
> GestureSource.

Not if the other constructors delegate to the constructor that takes an explicit
GestureSource.

https://codereview.chromium.org/680413006/diff/100001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_event_data_packet.cc (right):

https://codereview.chromium.org/680413006/diff/100001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_event_data_packet.cc:50: gfx::PointF(0, 0),
On 2014/11/20 19:49:16, jdduke wrote:
> You can just do gfx::PointF().

Done.

https://codereview.chromium.org/680413006/diff/100001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_event_data_packet.cc:60: gestures_ =
other.gestures_;
On 2014/11/20 19:49:16, jdduke wrote:
> Why not keep these in the initialize list?

Because, quoth the clang:
"error: an initializer for a delegating constructor must appear alone"

https://codereview.chromium.org/680413006/diff/100001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_event_data_packet.h (right):

https://codereview.chromium.org/680413006/diff/100001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_event_data_packet.h:54: // Check if this
packet should be ignored.
On 2014/11/20 19:49:16, jdduke wrote:
> No need for this getter comment.

Done.

[sadrul, 2014-11-21 17:50:12]

On 2014/11/20 21:12:13, jdduke wrote:
> On 2014/11/20 21:09:50, jdduke wrote:
> > On 2014/11/20 21:02:30, tdresser wrote:
> > > Events which are synchronously handled by some intermediate consumer
between
> > the
> > > GR and RWHVAura will be marked consumed, so they'll be acked as handled by
> > > WindowEventDispatcher::PostDispatchEvent.
> > 
> > Right, but that ack will be out-of-order if there is an async touch sequence
> in
> > the queue.
> 
> I guess I'm just wondering if these gymnastics are better than hosting the GR
in
> RWHVAura (or thereabouts) for content-targetted touches, with a separate GR
for
> the synchronous View-handled case (that instance begin non-eager). This is
> essentially what Android does; sadrul@, you mentioned you had concerns about
> this approach?

We should either do eager GR, or non-eager GR. Is there anything in eager GR
that doesn't work correctly with sync acks?

We currently have a GR in RWHVGuest for each <webview> tag. We may do something
similar for touch/gesture events in out-of-process iframes. So the idea of
hosting a GR in content isn't too bad (ideally in a platform-independent
manner). However, this will need a bit of work to do right for aura, since we
would need to make sure the pre/post-target handlers on the RWHVA::window_ etc.
keep working correctly. We will probably need some work to also make sure
drag-and-drop works correctly, and we can 'transfer' gesture-sequences between
two GRs.

[tdresser, 2014-11-24 21:44:40]

This handles arbitrarily ordered sync and async acks.

I'm not super happy with a reasonable quantity of the naming in this patch - let
me know if you have better ideas.

https://codereview.chromium.org/680413006/diff/140001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/680413006/diff/140001/ui/events/event.cc#newc...
ui/events/event.cc:594: return id++ % 100000;
This could use a SequentialIdGenerator, but that feels like it would be
overkill.

Even if we did get a duplicate ID, it wouldn't really hurt anything given the
_current_ consumers of |touch_event_id()|. What do you think?

[jdduke (slow), 2014-11-24 22:02:43]

https://codereview.chromium.org/680413006/diff/160001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/680413006/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2039: if (!point
&& host_->ShouldForwardTouchEvent()) {
Would it hurt to just make this conditional |if (!point) }|? The result should
be the same regardless of whether we should or should not forward the touch,
right?

[tdresser, 2014-11-25 13:58:10]

https://codereview.chromium.org/680413006/diff/160001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/680413006/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2039: if (!point
&& host_->ShouldForwardTouchEvent()) {
On 2014/11/24 22:02:43, jdduke wrote:
> Would it hurt to just make this conditional |if (!point) }|? The result should
> be the same regardless of whether we should or should not forward the touch,
> right?

Done.

[jdduke (slow), 2014-11-25 16:21:36]

https://codereview.chromium.org/680413006/diff/180001/ui/events/gesture_detec...
File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (right):

https://codereview.chromium.org/680413006/diff/180001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:222: bool
event_consumed) {
Hmm, but |event_consumed| should always be true, no? Otherwise we'll get
out-of-order gestures. So really this API should be phrased in terms of
dropping/ignoring the last touch or queue back.

https://codereview.chromium.org/680413006/diff/180001/ui/events/gesture_detec...
File ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc
(right):

https://codereview.chromium.org/680413006/diff/180001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc:1216:
PushGesture(ET_GESTURE_SCROLL_UPDATE);
Can you make this a different gesture type (than the previous one) so it's clear
from the expectation that the appropriate gesture was filtered?

https://codereview.chromium.org/680413006/diff/180001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc:1243:
// Both touches have now been acked.
Ideally we'd also have coverage for the case where we have a complete gesture
that's still queued (at the front), then one that gets synchronously ack'ed (at
the back).

[tdresser, 2014-11-25 20:07:50]

https://codereview.chromium.org/680413006/diff/180001/ui/events/gesture_detec...
File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (right):

https://codereview.chromium.org/680413006/diff/180001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:222: bool
event_consumed) {
On 2014/11/25 16:21:35, jdduke wrote:
> Hmm, but |event_consumed| should always be true, no? Otherwise we'll get
> out-of-order gestures. So really this API should be phrased in terms of
> dropping/ignoring the last touch or queue back.

How could this result in out-of-order gestures? The events in the
TouchDispositionGestureFilter are never dispatched out of order.

I think this is a clearer design. For instance, in
WindowEventDispatcher::PostDispatchEvent, it's nice to be able to call
AckSyncTouchEvent indicating that the touch is not consumed. I think this is
clearer even if in this context AckSyncTouchEvent and AckAsyncTouchEvent would
always do the same thing, which I'm not completely convinced is the case.

I'm not 100% positive that we don't have cases where we alternate between async
and sync acks where some of the sync acks are "unconsumed." For example, can
|host_->ShouldForwardTouchEvent()| in RenderWidgetHostViewAura change during a
touch sequence? That could result in us switching between async and sync acks.

Being robust enough to handle arbitrary orderings of async and sync acks feels
like it's just as clear a design, and is less prone to falling down in edge
cases we haven't thought of.

[jdduke (slow), 2014-11-25 20:11:31]

https://codereview.chromium.org/680413006/diff/180001/ui/events/gesture_detec...
File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (right):

https://codereview.chromium.org/680413006/diff/180001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:222: bool
event_consumed) {
On 2014/11/25 20:07:49, tdresser wrote:
> On 2014/11/25 16:21:35, jdduke wrote:
> > Hmm, but |event_consumed| should always be true, no? Otherwise we'll get
> > out-of-order gestures. So really this API should be phrased in terms of
> > dropping/ignoring the last touch or queue back.
> 
> How could this result in out-of-order gestures? The events in the
> TouchDispositionGestureFilter are never dispatched out of order.
> 
> I think this is a clearer design. For instance, in
> WindowEventDispatcher::PostDispatchEvent, it's nice to be able to call
> AckSyncTouchEvent indicating that the touch is not consumed. I think this is
> clearer even if in this context AckSyncTouchEvent and AckAsyncTouchEvent would
> always do the same thing, which I'm not completely convinced is the case.
> 
> I'm not 100% positive that we don't have cases where we alternate between
async
> and sync acks where some of the sync acks are "unconsumed." For example, can
> |host_->ShouldForwardTouchEvent()| in RenderWidgetHostViewAura change during a
> touch sequence? That could result in us switching between async and sync acks.
> 
> Being robust enough to handle arbitrary orderings of async and sync acks feels
> like it's just as clear a design, and is less prone to falling down in edge
> cases we haven't thought of.
> 
> 

Nevermind, you're right, the dispatch is deferred.

[jdduke (slow), 2014-11-25 20:26:37]

Thanks, this seems pretty reasonable.

https://codereview.chromium.org/680413006/diff/180001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/680413006/diff/180001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2040:
event->DisableSynchronousHandling();
Does this behave differently than just marking the event as handled and early
returning?

https://codereview.chromium.org/680413006/diff/180001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_event_data_packet.cc (right):

https://codereview.chromium.org/680413006/diff/180001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_event_data_packet.cc:47:
GestureEventDataPacket::GestureEventDataPacket()
I'm not sure this delegated constructor is any simpler...

https://codereview.chromium.org/680413006/diff/180001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_event_data_packet.cc:73: gestures_ =
other.gestures_;
ack_state_ = other.ack_state_;

https://codereview.chromium.org/680413006/diff/180001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_event_data_packet.h (right):

https://codereview.chromium.org/680413006/diff/180001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_event_data_packet.h:32: enum class AckState
: int {
Isn't the default type int? A shame to couple the ack here, but I guess the
packet has a very specific use anyhow.

https://codereview.chromium.org/680413006/diff/180001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_event_data_packet.h:59: void ack(bool
event_consumed) {
Hmm, I'd just as soon not make this inline.

https://codereview.chromium.org/680413006/diff/180001/ui/events/gesture_detec...
File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (right):

https://codereview.chromium.org/680413006/diff/180001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:213: if
(!sequence.empty() &&
If we have a a whole gesture queued that was ack'ed synchronously, we might hit
some nasty recursion here, not sure what the simplest solution is though.

[tdresser, 2014-11-26 15:17:18]

TDGF test fixes are pending
https://codereview.chromium.org/753743006/

This patch unifies the behavior of OnTouchEventAckForQueueFront and
OnTouchEventAckForQueueBack some, and gets rid of the recursion in gesture event
dispatch.

https://codereview.chromium.org/680413006/diff/180001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/680413006/diff/180001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2040:
event->DisableSynchronousHandling();
On 2014/11/25 20:26:36, jdduke wrote:
> Does this behave differently than just marking the event as handled and early
> returning?

We need to distinguish between the case where an event is being handled
asynchronously, and the case where an event has been consumed for some other
reason.

WindowEventDispatcher::PostDispatchEvent will ignore the event if its already
being handled async, but needs to pass the event to the GR if it's been consumed
for some other reason.

https://codereview.chromium.org/680413006/diff/180001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_event_data_packet.cc (right):

https://codereview.chromium.org/680413006/diff/180001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_event_data_packet.cc:47:
GestureEventDataPacket::GestureEventDataPacket()
On 2014/11/25 20:26:36, jdduke wrote:
> I'm not sure this delegated constructor is any simpler...

I'm not either.
Done.

https://codereview.chromium.org/680413006/diff/180001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_event_data_packet.cc:73: gestures_ =
other.gestures_;
On 2014/11/25 20:26:37, jdduke wrote:
> ack_state_ = other.ack_state_;

Done.

https://codereview.chromium.org/680413006/diff/180001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_event_data_packet.h (right):

https://codereview.chromium.org/680413006/diff/180001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_event_data_packet.h:32: enum class AckState
: int {
On 2014/11/25 20:26:37, jdduke wrote:
> Isn't the default type int? A shame to couple the ack here, but I guess the
> packet has a very specific use anyhow.

Perhaps I'm misinterpreting:
"Provide an explicit underlying type for enum classes and regular enums."
from 
https://chromium-cpp.appspot.com/

My interpretation was that you should always explicitly state the type.

Can you clarify what you mean by "Couple the ack"?

https://codereview.chromium.org/680413006/diff/180001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_event_data_packet.h:59: void ack(bool
event_consumed) {
On 2014/11/25 20:26:37, jdduke wrote:
> Hmm, I'd just as soon not make this inline.

Done.

https://codereview.chromium.org/680413006/diff/180001/ui/events/gesture_detec...
File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (right):

https://codereview.chromium.org/680413006/diff/180001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:213: if
(!sequence.empty() &&
On 2014/11/25 20:26:37, jdduke wrote:
> If we have a a whole gesture queued that was ack'ed synchronously, we might
hit
> some nasty recursion here, not sure what the simplest solution is though.

Done.

[jdduke (slow), 2014-11-26 16:07:09]

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_event_data_packet.cc (right):

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_event_data_packet.cc:100: ack_state_ =
event_consumed ? AckState::CONSUMED : AckState::UNCONSUMED;
maybe |DCHECK_EQ(ack_state_, AckState::PENDING)|.

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_event_data_packet.h (right):

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_event_data_packet.h:59: void ack(bool
event_consumed);
Nit: Uppercase. Also, the comment is a little unclear, as we also store the ack
with the packet for the async case.

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (left):

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:209:
DCHECK(touch_packet_for_current_ack_handled);
Do we still want to validate that at least one packet was handled?

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (right):

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:189:
if(IsEmpty() || Tail().empty())
Hmm, should an ack from the renderer ever hit the queue back? I kind of liked
the DCHECKs you had here.

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:192:
SendAckedEvents();
Hmm, I'm not sure about this. Don't we only want to try sending if there's only
one event in the entire queue?

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:195: void
TouchDispositionGestureFilter::SendAckedEvents() {
Nice, the recursion avoidance may have been premature on my part but this looks
good.

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:198: while
(true) {
Would it make sense to push the if condition below into the while condition?

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:218:
state_.OnTouchEventAck(sequence.front().ack_state() ==
Nit: (ack_state == GestureEventDataPacket::AckState::CONSUMED)

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
File ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc
(right):

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc:1244:
SendTouchNotConsumedAckForQueueBack();
Hmm, maybe add one more update here that is |NotConsumedForQueueBack| to handle
the case that multiple adjacent events at the back have been synchronously
ack'ed?

[tdresser, 2014-11-27 16:55:41]

On 2014/11/26 16:07:09, jdduke wrote:
>
https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
> File ui/events/gesture_detection/gesture_event_data_packet.cc (right):
> 
>
https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
> ui/events/gesture_detection/gesture_event_data_packet.cc:100: ack_state_ =
> event_consumed ? AckState::CONSUMED : AckState::UNCONSUMED;
> maybe |DCHECK_EQ(ack_state_, AckState::PENDING)|.
> 
>
https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
> File ui/events/gesture_detection/gesture_event_data_packet.h (right):
> 
>
https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
> ui/events/gesture_detection/gesture_event_data_packet.h:59: void ack(bool
> event_consumed);
> Nit: Uppercase. Also, the comment is a little unclear, as we also store the
ack
> with the packet for the async case.
> 
>
https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
> File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (left):
> 
>
https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
> ui/events/gesture_detection/touch_disposition_gesture_filter.cc:209:
> DCHECK(touch_packet_for_current_ack_handled);
> Do we still want to validate that at least one packet was handled?
> 
>
https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
> File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (right):
> 
>
https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
> ui/events/gesture_detection/touch_disposition_gesture_filter.cc:189:
> if(IsEmpty() || Tail().empty())
> Hmm, should an ack from the renderer ever hit the queue back? I kind of liked
> the DCHECKs you had here.
> 
>
https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
> ui/events/gesture_detection/touch_disposition_gesture_filter.cc:192:
> SendAckedEvents();
> Hmm, I'm not sure about this. Don't we only want to try sending if there's
only
> one event in the entire queue?
> 
>
https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
> ui/events/gesture_detection/touch_disposition_gesture_filter.cc:195: void
> TouchDispositionGestureFilter::SendAckedEvents() {
> Nice, the recursion avoidance may have been premature on my part but this
looks
> good.
> 
>
https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
> ui/events/gesture_detection/touch_disposition_gesture_filter.cc:198: while
> (true) {
> Would it make sense to push the if condition below into the while condition?
> 
>
https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
> ui/events/gesture_detection/touch_disposition_gesture_filter.cc:218:
> state_.OnTouchEventAck(sequence.front().ack_state() ==
> Nit: (ack_state == GestureEventDataPacket::AckState::CONSUMED)
> 
>
https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
> File ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
> ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc:1244:
> SendTouchNotConsumedAckForQueueBack();
> Hmm, maybe add one more update here that is |NotConsumedForQueueBack| to
handle
> the case that multiple adjacent events at the back have been synchronously
> ack'ed?

Following up on offline discussion: we do still need an additional flag on
|Event|.

|WindowEventDispatcher::PostDispatchEvent| needs to be able to distinguish
between 3 cases:
1. Ack the event unconsumed.
2. Ack the event consumed.
3. Don't ack the event. (It's being handled async)

We can't communicate that through the existing handled/consumed approach.

[tdresser, 2014-11-27 18:26:25]

Still TODO:
RWHVAura test that events are marked ER_DISABLE_SYNC_HANDLING if they are
invalid.

Pending https://codereview.chromium.org/753743006/

https://codereview.chromium.org/680413006/diff/180001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc:1216:
PushGesture(ET_GESTURE_SCROLL_UPDATE);
Can you make this a different gesture type (than the previous one) so it's clear
from the expectation that the appropriate gesture was filtered?

https://codereview.chromium.org/680413006/diff/180001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc:1243:
// Both touches have now been acked.
Ideally we'd also have coverage for the case where we have a complete gesture
that's still queued (at the front), then one that gets synchronously ack'ed (at
the back).

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_event_data_packet.cc (right):

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_event_data_packet.cc:100: ack_state_ =
event_consumed ? AckState::CONSUMED : AckState::UNCONSUMED;
On 2014/11/26 16:07:09, jdduke wrote:
> maybe |DCHECK_EQ(ack_state_, AckState::PENDING)|.

We explicitly allow spurious acks, so to add that check here I'd have to add
some additional guards elsewhere.

Otherwise I can add some comments indicating why we don't have those checks
here.
What do you think?

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_event_data_packet.h (right):

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_event_data_packet.h:59: void ack(bool
event_consumed);
On 2014/11/26 16:07:09, jdduke wrote:
> Nit: Uppercase. Also, the comment is a little unclear, as we also store the
ack
> with the packet for the async case.

Done.

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (left):

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:209:
DCHECK(touch_packet_for_current_ack_handled);
On 2014/11/26 16:07:09, jdduke wrote:
> Do we still want to validate that at least one packet was handled?

Done.

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (right):

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:189:
if(IsEmpty() || Tail().empty())
On 2014/11/26 16:07:09, jdduke wrote:
> Hmm, should an ack from the renderer ever hit the queue back? I kind of liked
> the DCHECKs you had here.

Done.

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:192:
SendAckedEvents();
On 2014/11/26 16:07:09, jdduke wrote:
> Hmm, I'm not sure about this. Don't we only want to try sending if there's
only
> one event in the entire queue?

I haven't come up with a case where having that guard here is any different from
just blocking on events closer to the head of the queue.

Do you think special casing that here is worth it?
-
On second though, it would let us assert that at least one packet was
dispatched, which is a nice property. Done.

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:195: void
TouchDispositionGestureFilter::SendAckedEvents() {
On 2014/11/26 16:07:09, jdduke wrote:
> Nice, the recursion avoidance may have been premature on my part but this
looks
> good.

I think its a fair bit clearer this way, yeah.

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:198: while
(true) {
On 2014/11/26 16:07:09, jdduke wrote:
> Would it make sense to push the if condition below into the while condition?

Yuck. Fixed.

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:218:
state_.OnTouchEventAck(sequence.front().ack_state() ==
On 2014/11/26 16:07:09, jdduke wrote:
> Nit: (ack_state == GestureEventDataPacket::AckState::CONSUMED)

Done.

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
File ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc
(right):

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc:1244:
SendTouchNotConsumedAckForQueueBack();
On 2014/11/26 16:07:09, jdduke wrote:
> Hmm, maybe add one more update here that is |NotConsumedForQueueBack| to
handle
> the case that multiple adjacent events at the back have been synchronously
> ack'ed? 

Done.

[sadrul, 2014-11-27 20:39:31]

https://codereview.chromium.org/680413006/diff/220001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/680413006/diff/220001/ui/events/event.cc#newc...
ui/events/event.cc:590: int TouchEvent::get_next_touch_event_id() {
Make this an internal function in the anon namespace.

https://codereview.chromium.org/680413006/diff/220001/ui/events/event.cc#newc...
ui/events/event.cc:594: return id++ % 100000;
const int kMaxTouchEventId = 100000;

But why have an upper limit? Why not unsigned long long?

https://codereview.chromium.org/680413006/diff/220001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/680413006/diff/220001/ui/events/event.h#newco...
ui/events/event.h:521: int touch_event_id() const { return touch_event_id_; }
touch_id() and touch_event_id() both sound very similar, and so this is a bit
confusing. Add a comment above this explaining the difference.

Perhaps 'unique_id()' would be better than 'touch_event_id()', in the sense that
this should be unique for most reasonable purposes.

[tdresser, 2014-11-28 18:33:37]

https://codereview.chromium.org/680413006/diff/180001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/680413006/diff/180001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2040:
event->DisableSynchronousHandling();
On 2014/11/26 15:17:18, tdresser wrote:
> On 2014/11/25 20:26:36, jdduke wrote:
> > Does this behave differently than just marking the event as handled and
early
> > returning?
> 
> We need to distinguish between the case where an event is being handled
> asynchronously, and the case where an event has been consumed for some other
> reason.
> 
> WindowEventDispatcher::PostDispatchEvent will ignore the event if its already
> being handled async, but needs to pass the event to the GR if it's been
consumed
> for some other reason.

Oh, you're right.
I do still need that flag, but I don't need to use it here.

Good catch.

https://codereview.chromium.org/680413006/diff/220001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/680413006/diff/220001/ui/events/event.cc#newc...
ui/events/event.cc:590: int TouchEvent::get_next_touch_event_id() {
On 2014/11/27 20:39:30, sadrul wrote:
> Make this an internal function in the anon namespace.

Done.

https://codereview.chromium.org/680413006/diff/220001/ui/events/event.cc#newc...
ui/events/event.cc:594: return id++ % 100000;
On 2014/11/27 20:39:30, sadrul wrote:
> const int kMaxTouchEventId = 100000;
> 
> But why have an upper limit? Why not unsigned long long?

Done.

https://codereview.chromium.org/680413006/diff/220001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/680413006/diff/220001/ui/events/event.h#newco...
ui/events/event.h:521: int touch_event_id() const { return touch_event_id_; }
On 2014/11/27 20:39:30, sadrul wrote:
> touch_id() and touch_event_id() both sound very similar, and so this is a bit
> confusing. Add a comment above this explaining the difference.
> 
> Perhaps 'unique_id()' would be better than 'touch_event_id()', in the sense
that
> this should be unique for most reasonable purposes.

Went with unique_event_id, to clarify that it is unique per event, not unique
per pointer or anything like that.
Done.

[tdresser, 2014-12-02 15:41:42]

On 2014/11/28 18:33:37, tdresser wrote:
>
https://codereview.chromium.org/680413006/diff/180001/content/browser/rendere...
> File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> 
>
https://codereview.chromium.org/680413006/diff/180001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_view_aura.cc:2040:
> event->DisableSynchronousHandling();
> On 2014/11/26 15:17:18, tdresser wrote:
> > On 2014/11/25 20:26:36, jdduke wrote:
> > > Does this behave differently than just marking the event as handled and
> early
> > > returning?
> > 
> > We need to distinguish between the case where an event is being handled
> > asynchronously, and the case where an event has been consumed for some other
> > reason.
> > 
> > WindowEventDispatcher::PostDispatchEvent will ignore the event if its
already
> > being handled async, but needs to pass the event to the GR if it's been
> consumed
> > for some other reason.
> 
> Oh, you're right.
> I do still need that flag, but I don't need to use it here.
> 
> Good catch.
> 
> https://codereview.chromium.org/680413006/diff/220001/ui/events/event.cc
> File ui/events/event.cc (right):
> 
>
https://codereview.chromium.org/680413006/diff/220001/ui/events/event.cc#newc...
> ui/events/event.cc:590: int TouchEvent::get_next_touch_event_id() {
> On 2014/11/27 20:39:30, sadrul wrote:
> > Make this an internal function in the anon namespace.
> 
> Done.
> 
>
https://codereview.chromium.org/680413006/diff/220001/ui/events/event.cc#newc...
> ui/events/event.cc:594: return id++ % 100000;
> On 2014/11/27 20:39:30, sadrul wrote:
> > const int kMaxTouchEventId = 100000;
> > 
> > But why have an upper limit? Why not unsigned long long?
> 
> Done.
> 
> https://codereview.chromium.org/680413006/diff/220001/ui/events/event.h
> File ui/events/event.h (right):
> 
>
https://codereview.chromium.org/680413006/diff/220001/ui/events/event.h#newco...
> ui/events/event.h:521: int touch_event_id() const { return touch_event_id_; }
> On 2014/11/27 20:39:30, sadrul wrote:
> > touch_id() and touch_event_id() both sound very similar, and so this is a
bit
> > confusing. Add a comment above this explaining the difference.
> > 
> > Perhaps 'unique_id()' would be better than 'touch_event_id()', in the sense
> that
> > this should be unique for most reasonable purposes.
> 
> Went with unique_event_id, to clarify that it is unique per event, not unique
> per pointer or anything like that.
> Done.

Ping.

[sadrul, 2014-12-02 15:54:10]

https://codereview.chromium.org/680413006/diff/240001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/680413006/diff/240001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:501:
ui::GestureRecognizer::Get()->AckSyncTouchEvent(
Now that TouchEvent carries a unique-id with it, is there a reason to have
AckSyncTouchEvent/AckAsyncTouchEvent in the public GR interface anymore? Why not
just AckTouchEvent()? The GR can decide whether or not to release the gesture
events based on whether the touch-event acked is at the front of the queue or
not. Would that work?

[tdresser, 2014-12-02 16:02:12]

On 2014/12/02 15:54:10, sadrul wrote:
>
https://codereview.chromium.org/680413006/diff/240001/ui/aura/window_event_di...
> File ui/aura/window_event_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/680413006/diff/240001/ui/aura/window_event_di...
> ui/aura/window_event_dispatcher.cc:501:
> ui::GestureRecognizer::Get()->AckSyncTouchEvent(
> Now that TouchEvent carries a unique-id with it, is there a reason to have
> AckSyncTouchEvent/AckAsyncTouchEvent in the public GR interface anymore? Why
not
> just AckTouchEvent()? The GR can decide whether or not to release the gesture
> events based on whether the touch-event acked is at the front of the queue or
> not. Would that work?

Jared and I discussed this, and we decided that it was nice to have the extra
verification that we're acking the correct event.

You're right that this is redundant, but we think the redundancy is worth the
extra confidence that the correct event is being acked.

[jdduke (slow), 2014-12-02 16:22:00]

ui/events/gesture_detection lgtm.

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_event_data_packet.cc (right):

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_event_data_packet.cc:100: ack_state_ =
event_consumed ? AckState::CONSUMED : AckState::UNCONSUMED;
On 2014/11/27 18:26:24, tdresser wrote:
> On 2014/11/26 16:07:09, jdduke wrote:
> > maybe |DCHECK_EQ(ack_state_, AckState::PENDING)|.
> 
> We explicitly allow spurious acks, so to add that check here I'd have to add
> some additional guards elsewhere.
> 
> Otherwise I can add some comments indicating why we don't have those checks
> here.
> What do you think?

Some kind of check would be nice, e.g., can we at least verify that it hasn't
been consumed already?

https://codereview.chromium.org/680413006/diff/240001/ui/events/gesture_detec...
File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (right):

https://codereview.chromium.org/680413006/diff/240001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:189:
CHECK(!IsEmpty() && !Tail().empty());
Nit: Make this 2 checks so it's immediately clear which condition fails.

https://codereview.chromium.org/680413006/diff/240001/ui/events/gesture_detec...
File ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc
(right):

https://codereview.chromium.org/680413006/diff/240001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc:1204:
TEST_F(TouchDispositionGestureFilterTest, AckQueueBack) {
I'd like to see another separate test where we have the following:

1) A touch sequence that is queued (delayed, async ack).
2) A touch sequence that is ack'ed synchronously.
3) Another touch sequence that is ack'ed synchronously.
4) A touch sequence that is queued (delayed, async ack).

Then, we ack the touch sequence from (1) and ensure the right thing happens
(i.e., once we hit the final touch, sequences (2) and (3) get dispatched, but
(4) remains queued).

[tdresser, 2014-12-02 21:29:27]

https://codereview.chromium.org/680413006/diff/180001/ui/events/gesture_detec...
File ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc
(right):

https://codereview.chromium.org/680413006/diff/180001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc:1216:
PushGesture(ET_GESTURE_SCROLL_UPDATE);
On 2014/11/25 16:21:36, jdduke wrote:
> Can you make this a different gesture type (than the previous one) so it's
clear
> from the expectation that the appropriate gesture was filtered?

Gave the events different locations, which accomplishes the same goal.

https://codereview.chromium.org/680413006/diff/180001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc:1243:
// Both touches have now been acked.
On 2014/11/25 16:21:36, jdduke wrote:
> Ideally we'd also have coverage for the case where we have a complete gesture
> that's still queued (at the front), then one that gets synchronously ack'ed
(at
> the back).

Done.

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_event_data_packet.cc (right):

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_event_data_packet.cc:100: ack_state_ =
event_consumed ? AckState::CONSUMED : AckState::UNCONSUMED;
On 2014/12/02 16:21:59, jdduke wrote:
> On 2014/11/27 18:26:24, tdresser wrote:
> > On 2014/11/26 16:07:09, jdduke wrote:
> > > maybe |DCHECK_EQ(ack_state_, AckState::PENDING)|.
> > 
> > We explicitly allow spurious acks, so to add that check here I'd have to add
> > some additional guards elsewhere.
> > 
> > Otherwise I can add some comments indicating why we don't have those checks
> > here.
> > What do you think?
> 
> Some kind of check would be nice, e.g., can we at least verify that it hasn't
> been consumed already?

The renderer could send us spurious event consumed acks, couldn't it?

https://codereview.chromium.org/680413006/diff/240001/ui/events/gesture_detec...
File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (right):

https://codereview.chromium.org/680413006/diff/240001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:189:
CHECK(!IsEmpty() && !Tail().empty());
On 2014/12/02 16:21:59, jdduke wrote:
> Nit: Make this 2 checks so it's immediately clear which condition fails.

Done.

https://codereview.chromium.org/680413006/diff/240001/ui/events/gesture_detec...
File ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc
(right):

https://codereview.chromium.org/680413006/diff/240001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc:1204:
TEST_F(TouchDispositionGestureFilterTest, AckQueueBack) {
On 2014/12/02 16:21:59, jdduke wrote:
> I'd like to see another separate test where we have the following:
> 
> 1) A touch sequence that is queued (delayed, async ack).
> 2) A touch sequence that is ack'ed synchronously.
> 3) Another touch sequence that is ack'ed synchronously.
> 4) A touch sequence that is queued (delayed, async ack).
> 
> Then, we ack the touch sequence from (1) and ensure the right thing happens
> (i.e., once we hit the final touch, sequences (2) and (3) get dispatched, but
> (4) remains queued).

Done.

[jdduke (slow), 2014-12-02 21:34:03]

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_event_data_packet.cc (right):

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_event_data_packet.cc:100: ack_state_ =
event_consumed ? AckState::CONSUMED : AckState::UNCONSUMED;
On 2014/12/02 21:29:27, tdresser wrote:
> On 2014/12/02 16:21:59, jdduke wrote:
> > On 2014/11/27 18:26:24, tdresser wrote:
> > > On 2014/11/26 16:07:09, jdduke wrote:
> > > > maybe |DCHECK_EQ(ack_state_, AckState::PENDING)|.
> > > 
> > > We explicitly allow spurious acks, so to add that check here I'd have to
add
> > > some additional guards elsewhere.
> > > 
> > > Otherwise I can add some comments indicating why we don't have those
checks
> > > here.
> > > What do you think?
> > 
> > Some kind of check would be nice, e.g., can we at least verify that it
hasn't
> > been consumed already?
> 
> The renderer could send us spurious event consumed acks, couldn't it?

Hmm, but the spurious ack from the renderer should always be async, in which
case the queue should either be empty or we'll be ack'ing an event that's never
been ack'ed (events that may have been ack'ed synchrously would have been
dispatched already).

[tdresser, 2014-12-02 22:00:22]

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_event_data_packet.cc (right):

https://codereview.chromium.org/680413006/diff/200001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_event_data_packet.cc:100: ack_state_ =
event_consumed ? AckState::CONSUMED : AckState::UNCONSUMED;
On 2014/12/02 21:34:03, jdduke wrote:
> On 2014/12/02 21:29:27, tdresser wrote:
> > On 2014/12/02 16:21:59, jdduke wrote:
> > > On 2014/11/27 18:26:24, tdresser wrote:
> > > > On 2014/11/26 16:07:09, jdduke wrote:
> > > > > maybe |DCHECK_EQ(ack_state_, AckState::PENDING)|.
> > > > 
> > > > We explicitly allow spurious acks, so to add that check here I'd have to
> add
> > > > some additional guards elsewhere.
> > > > 
> > > > Otherwise I can add some comments indicating why we don't have those
> checks
> > > > here.
> > > > What do you think?
> > > 
> > > Some kind of check would be nice, e.g., can we at least verify that it
> hasn't
> > > been consumed already?
> > 
> > The renderer could send us spurious event consumed acks, couldn't it?
> 
> Hmm, but the spurious ack from the renderer should always be async, in which
> case the queue should either be empty or we'll be ack'ing an event that's
never
> been ack'ed (events that may have been ack'ed synchrously would have been
> dispatched already).

Good point.
Done.

[sadrul, 2014-12-03 21:30:11]

On 2014/12/02 16:02:12, tdresser wrote:
> On 2014/12/02 15:54:10, sadrul wrote:
> >
>
https://codereview.chromium.org/680413006/diff/240001/ui/aura/window_event_di...
> > File ui/aura/window_event_dispatcher.cc (right):
> > 
> >
>
https://codereview.chromium.org/680413006/diff/240001/ui/aura/window_event_di...
> > ui/aura/window_event_dispatcher.cc:501:
> > ui::GestureRecognizer::Get()->AckSyncTouchEvent(
> > Now that TouchEvent carries a unique-id with it, is there a reason to have
> > AckSyncTouchEvent/AckAsyncTouchEvent in the public GR interface anymore? Why
> not
> > just AckTouchEvent()? The GR can decide whether or not to release the
gesture
> > events based on whether the touch-event acked is at the front of the queue
or
> > not. Would that work?
> 
> Jared and I discussed this, and we decided that it was nice to have the extra
> verification that we're acking the correct event.
> 
> You're right that this is redundant, but we think the redundancy is worth the
> extra confidence that the correct event is being acked.

My understanding is we can ack any touch event in the queue, and it will do the
right thing. Is that not the case?

https://codereview.chromium.org/680413006/diff/280001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/680413006/diff/280001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:163: // pass an event here.
Is this comment still correct?

https://codereview.chromium.org/680413006/diff/280001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:503: static_cast<Window*>(target)));
This touch-event is being sent in the target's coord space. I believe the rest
of the touch-events are sent in the root-window's coord space. Is this not
problematic?

https://codereview.chromium.org/680413006/diff/280001/ui/aura/window_unittest.cc
File ui/aura/window_unittest.cc (right):

https://codereview.chromium.org/680413006/diff/280001/ui/aura/window_unittest...
ui/aura/window_unittest.cc:827: // Events are now untargetted.
Why?

https://codereview.chromium.org/680413006/diff/280001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/680413006/diff/280001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:153:
gesture_provider_->OnAsyncTouchEventAck(false);
Isn't this actually sync ack?

https://codereview.chromium.org/680413006/diff/280001/ui/events/event_constan...
File ui/events/event_constants.h (right):

https://codereview.chromium.org/680413006/diff/280001/ui/events/event_constan...
ui/events/event_constants.h:130: //  happens if the event is being handled
spacing is off

[sadrul, 2014-12-03 21:47:29]

PS: You should remove '(WIP)' from CL desc :)

[jdduke (slow), 2014-12-04 19:27:44]

https://codereview.chromium.org/680413006/diff/280001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator.cc (right):

https://codereview.chromium.org/680413006/diff/280001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.cc:218: if
(native_stream_active_sequence_count_) {
I wonder if it's worth making these synchronous acks?

[jdduke (slow), 2014-12-04 19:28:16]

https://codereview.chromium.org/680413006/diff/280001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator.cc (right):

https://codereview.chromium.org/680413006/diff/280001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.cc:218: if
(native_stream_active_sequence_count_) {
On 2014/12/04 19:27:44, jdduke wrote:
> I wonder if it's worth making these synchronous acks?

By these I mean the two acks within ForwardTouchEventToClient

[tdresser, 2014-12-05 16:44:31]

https://codereview.chromium.org/680413006/diff/240001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/680413006/diff/240001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:501:
ui::GestureRecognizer::Get()->AckSyncTouchEvent(
On 2014/12/02 15:54:10, sadrul wrote:
> Now that TouchEvent carries a unique-id with it, is there a reason to have
> AckSyncTouchEvent/AckAsyncTouchEvent in the public GR interface anymore? Why
not
> just AckTouchEvent()? The GR can decide whether or not to release the gesture
> events based on whether the touch-event acked is at the front of the queue or
> not. Would that work?

(Oops, I should have kept this conversation in one place.)

>> tdresser: Jared and I discussed this, and we decided that it was nice to have
the extra
>> verification that we're acking the correct event.
>> 
>> You're right that this is redundant, but we think the redundancy is worth the
>> extra confidence that the correct event is being acked.
>
> sadrul: My understanding is we can ack any touch event in the queue, and it
will do the
> right thing. Is that not the case?

That is the case, however, we still want to impose some constraints. Despite the
fact that we could ack any touch event in the queue, if our understanding of the
system is correct, we should only be acking events at the front and back of the
queue. We want to double check that this understanding is correct.

https://codereview.chromium.org/680413006/diff/280001/content/browser/rendere...
File content/browser/renderer_host/input/touch_emulator.cc (right):

https://codereview.chromium.org/680413006/diff/280001/content/browser/rendere...
content/browser/renderer_host/input/touch_emulator.cc:218: if
(native_stream_active_sequence_count_) {
On 2014/12/04 19:28:16, jdduke wrote:
> On 2014/12/04 19:27:44, jdduke wrote:
> > I wonder if it's worth making these synchronous acks?
> 
> By these I mean the two acks within ForwardTouchEventToClient

Done.

https://codereview.chromium.org/680413006/diff/280001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/680413006/diff/280001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:163: // pass an event here.
On 2014/12/03 21:30:10, sadrul wrote:
> Is this comment still correct?

I'm performing that cleanup in a followup patch:
https://codereview.chromium.org/503883004/

Note that we're only using the touch event unique identifier to double check
that we're acking the right event for the synchronous case.

https://codereview.chromium.org/680413006/diff/280001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:503: static_cast<Window*>(target)));
On 2014/12/03 21:30:10, sadrul wrote:
> This touch-event is being sent in the target's coord space. I believe the rest
> of the touch-events are sent in the root-window's coord space. Is this not
> problematic?

It isn't problematic because we're only using the touch event's unique_event_id.

I've clarified that we're only using the unique_event_id by only passing the id
- I think strongly typing the id is probably worth it here, though it is a
little ugly.

https://codereview.chromium.org/680413006/diff/280001/ui/aura/window_unittest.cc
File ui/aura/window_unittest.cc (right):

https://codereview.chromium.org/680413006/diff/280001/ui/aura/window_unittest...
ui/aura/window_unittest.cc:827: // Events are now untargetted.
On 2014/12/03 21:30:11, sadrul wrote:
> Why?

Window 2 sees an invalid touch event stream (a move with no associated press).
The eager GR requires that invalid touch events not be dispatched, so w2 doesn't
see that touch move. 

Perhaps "untargetted' isn't quite the correct word, but I'm not sure what would
be better.

Any thoughts?

https://codereview.chromium.org/680413006/diff/280001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/680413006/diff/280001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:153:
gesture_provider_->OnAsyncTouchEventAck(false);
On 2014/12/03 21:30:11, sadrul wrote:
> Isn't this actually sync ack?

Done.

https://codereview.chromium.org/680413006/diff/280001/ui/events/event_constan...
File ui/events/event_constants.h (right):

https://codereview.chromium.org/680413006/diff/280001/ui/events/event_constan...
ui/events/event_constants.h:130: //  happens if the event is being handled
On 2014/12/03 21:30:11, sadrul wrote:
> spacing is off

Done.

[tdresser, 2014-12-05 16:49:01]

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

[tdresser, 2014-12-05 16:49:02]

davidben@, can you take a look at:
content/browser/frame_host/render_widget_host_view_guest.cc

[davidben, 2014-12-05 17:28:24]

content/ lgtm (just an OWNERS stamp; I'm not familiar with touch, so I'm
asssuming jdduke/sadrul are appropriate main reviewers)

By the way, you seem to have a compile failure on the Mac try bot.

[tdresser, 2014-12-05 18:01:43]

On 2014/12/05 17:28:24, David Benjamin wrote:
> content/ lgtm (just an OWNERS stamp; I'm not familiar with touch, so I'm
> asssuming jdduke/sadrul are appropriate main reviewers)
> 
> By the way, you seem to have a compile failure on the Mac try bot.

Ah, yeah, thanks for pointing that out.

sadrul/jdduke - do you think it's worth using a strongly typed id for this?
If so, any suggestions where it should live, where OS X can see it?

[sadrul, 2014-12-07 17:25:44]

A couple of nits. I still think having a single AckTouchEvent() in the API is
better, but I won't block this CL on that. Assuming this doesn't break
tab-dragging, LGTM

https://codereview.chromium.org/680413006/diff/240001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/680413006/diff/240001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:501:
ui::GestureRecognizer::Get()->AckSyncTouchEvent(
On 2014/12/05 16:44:31, tdresser wrote:
> On 2014/12/02 15:54:10, sadrul wrote:
> > Now that TouchEvent carries a unique-id with it, is there a reason to have
> > AckSyncTouchEvent/AckAsyncTouchEvent in the public GR interface anymore? Why
> not
> > just AckTouchEvent()? The GR can decide whether or not to release the
gesture
> > events based on whether the touch-event acked is at the front of the queue
or
> > not. Would that work?
> 
> (Oops, I should have kept this conversation in one place.)
> 
> >> tdresser: Jared and I discussed this, and we decided that it was nice to
have
> the extra
> >> verification that we're acking the correct event.
> >> 
> >> You're right that this is redundant, but we think the redundancy is worth
the
> >> extra confidence that the correct event is being acked.
> >
> > sadrul: My understanding is we can ack any touch event in the queue, and it
> will do the
> > right thing. Is that not the case?
> 
> That is the case, however, we still want to impose some constraints. Despite
the
> fact that we could ack any touch event in the queue, if our understanding of
the
> system is correct, we should only be acking events at the front and back of
the
> queue. We want to double check that this understanding is correct.

Sure. But can't you do that in GR::AckTouchEvent() by verifying that the event
being ack'ed is either at the front or back of the queue? Having a public API
for this feels unclean (and can be confusing, e.g.
https://codereview.chromium.org/680413006/diff/280001/ui/chromeos/touch_explo...)

https://codereview.chromium.org/680413006/diff/280001/ui/aura/window_unittest.cc
File ui/aura/window_unittest.cc (right):

https://codereview.chromium.org/680413006/diff/280001/ui/aura/window_unittest...
ui/aura/window_unittest.cc:827: // Events are now untargetted.
On 2014/12/05 16:44:31, tdresser wrote:
> On 2014/12/03 21:30:11, sadrul wrote:
> > Why?
> 
> Window 2 sees an invalid touch event stream (a move with no associated press).
> The eager GR requires that invalid touch events not be dispatched, so w2
doesn't
> see that touch move. 

I was under the impression that existing GR sequences are 'transferred' to the
capturing window, and so the sequence for |w1| would start going to |w2|. But it
doesn't look like that particular code is in //ui/wm, and so does not trigger
here.

Can you verify that tab dragging still works after this change?

> 
> Perhaps "untargetted' isn't quite the correct word, but I'm not sure what
would
> be better.
> 
> Any thoughts?

I think 'untargeted' is OK.

https://codereview.chromium.org/680413006/diff/320001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/680413006/diff/320001/ui/events/event.h#newco...
ui/events/event.h:208: void DisableSynchronousHandling();
Can you move DisableSynchronousHandling() inside TouchEvent?

https://codereview.chromium.org/680413006/diff/320001/ui/events/event.h#newco...
ui/events/event.h:557: const uint64 unique_event_id_;
Remove the second sentence.

[tdresser, 2014-12-08 14:49:30]

https://codereview.chromium.org/680413006/diff/240001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/680413006/diff/240001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:501:
ui::GestureRecognizer::Get()->AckSyncTouchEvent(
On 2014/12/07 17:25:43, sadrul (OOO - back on Wed) wrote:
> On 2014/12/05 16:44:31, tdresser wrote:
> > On 2014/12/02 15:54:10, sadrul wrote:
> > > Now that TouchEvent carries a unique-id with it, is there a reason to have
> > > AckSyncTouchEvent/AckAsyncTouchEvent in the public GR interface anymore?
Why
> > not
> > > just AckTouchEvent()? The GR can decide whether or not to release the
> gesture
> > > events based on whether the touch-event acked is at the front of the queue
> or
> > > not. Would that work?
> > 
> > (Oops, I should have kept this conversation in one place.)
> > 
> > >> tdresser: Jared and I discussed this, and we decided that it was nice to
> have
> > the extra
> > >> verification that we're acking the correct event.
> > >> 
> > >> You're right that this is redundant, but we think the redundancy is worth
> the
> > >> extra confidence that the correct event is being acked.
> > >
> > > sadrul: My understanding is we can ack any touch event in the queue, and
it
> > will do the
> > > right thing. Is that not the case?
> > 
> > That is the case, however, we still want to impose some constraints. Despite
> the
> > fact that we could ack any touch event in the queue, if our understanding of
> the
> > system is correct, we should only be acking events at the front and back of
> the
> > queue. We want to double check that this understanding is correct.
> 
> Sure. But can't you do that in GR::AckTouchEvent() by verifying that the event
> being ack'ed is either at the front or back of the queue? Having a public API
> for this feels unclean (and can be confusing, e.g.
>
https://codereview.chromium.org/680413006/diff/280001/ui/chromeos/touch_explo...)

Yeah, Jared and I discussed that. It's a little more work to get the unique
touch event id persisted into the async case.

Jared, what do you think? I think this is a bit safer, but its true it may be a
little more confusing.

https://codereview.chromium.org/680413006/diff/280001/ui/aura/window_unittest.cc
File ui/aura/window_unittest.cc (right):

https://codereview.chromium.org/680413006/diff/280001/ui/aura/window_unittest...
ui/aura/window_unittest.cc:827: // Events are now untargetted.
On 2014/12/07 17:25:44, sadrul (OOO - back on Wed) wrote:
> On 2014/12/05 16:44:31, tdresser wrote:
> > On 2014/12/03 21:30:11, sadrul wrote:
> > > Why?
> > 
> > Window 2 sees an invalid touch event stream (a move with no associated
press).
> > The eager GR requires that invalid touch events not be dispatched, so w2
> doesn't
> > see that touch move. 
> 
> I was under the impression that existing GR sequences are 'transferred' to the
> capturing window, and so the sequence for |w1| would start going to |w2|. But
it
> doesn't look like that particular code is in //ui/wm, and so does not trigger
> here.
> 
> Can you verify that tab dragging still works after this change?
> 
> > 
> > Perhaps "untargetted' isn't quite the correct word, but I'm not sure what
> would
> > be better.
> > 
> > Any thoughts?
> 
> I think 'untargeted' is OK.

We do transfer gesture sequences sometimes, but (I think) only when we have a
single sequence being transferred. Suppose I have 3 windows with active touch
sequences when another window gains capture. We can't really transfer all of
those active sequences to the window which just got capture.

Tab dragging does work - I was worried about that as well.

Fixed "targetted".

https://codereview.chromium.org/680413006/diff/320001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/680413006/diff/320001/ui/events/event.h#newco...
ui/events/event.h:208: void DisableSynchronousHandling();
On 2014/12/07 17:25:44, sadrul (OOO - back on Wed) wrote:
> Can you move DisableSynchronousHandling() inside TouchEvent?

Done.

https://codereview.chromium.org/680413006/diff/320001/ui/events/event.h#newco...
ui/events/event.h:557: const uint64 unique_event_id_;
On 2014/12/07 17:25:44, sadrul (OOO - back on Wed) wrote:
> Remove the second sentence.

Good call. Done.

[jdduke (slow), 2014-12-08 16:38:41]

https://codereview.chromium.org/680413006/diff/240001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/680413006/diff/240001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.cc:501:
ui::GestureRecognizer::Get()->AckSyncTouchEvent(
On 2014/12/08 14:49:30, tdresser wrote:
> On 2014/12/07 17:25:43, sadrul (OOO - back on Wed) wrote:
> > On 2014/12/05 16:44:31, tdresser wrote:
> > > On 2014/12/02 15:54:10, sadrul wrote:
> > > > Now that TouchEvent carries a unique-id with it, is there a reason to
have
> > > > AckSyncTouchEvent/AckAsyncTouchEvent in the public GR interface anymore?
> Why
> > > not
> > > > just AckTouchEvent()? The GR can decide whether or not to release the
> > gesture
> > > > events based on whether the touch-event acked is at the front of the
queue
> > or
> > > > not. Would that work?
> > > 
> > > (Oops, I should have kept this conversation in one place.)
> > > 
> > > >> tdresser: Jared and I discussed this, and we decided that it was nice
to
> > have
> > > the extra
> > > >> verification that we're acking the correct event.
> > > >> 
> > > >> You're right that this is redundant, but we think the redundancy is
worth
> > the
> > > >> extra confidence that the correct event is being acked.
> > > >
> > > > sadrul: My understanding is we can ack any touch event in the queue, and
> it
> > > will do the
> > > > right thing. Is that not the case?
> > > 
> > > That is the case, however, we still want to impose some constraints.
Despite
> > the
> > > fact that we could ack any touch event in the queue, if our understanding
of
> > the
> > > system is correct, we should only be acking events at the front and back
of
> > the
> > > queue. We want to double check that this understanding is correct.
> > 
> > Sure. But can't you do that in GR::AckTouchEvent() by verifying that the
event
> > being ack'ed is either at the front or back of the queue? Having a public
API
> > for this feels unclean (and can be confusing, e.g.
> >
>
https://codereview.chromium.org/680413006/diff/280001/ui/chromeos/touch_explo...)
> 
> Yeah, Jared and I discussed that. It's a little more work to get the unique
> touch event id persisted into the async case.
> 
> Jared, what do you think? I think this is a bit safer, but its true it may be
a
> little more confusing.

Since Aura is the only platform that makes the distinction, one thought would be
to have GestureProviderAura take the touch event and the ack (or
GestureRecognizer), and have it be responsible for calling the right method.
Would you even need to track id's in that case? Can't you just look at the type
of ack and whether sync handling was disabled?

[tdresser, 2014-12-09 19:08:14]

On 2014/12/08 16:38:41, jdduke wrote:
>
https://codereview.chromium.org/680413006/diff/240001/ui/aura/window_event_di...
> File ui/aura/window_event_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/680413006/diff/240001/ui/aura/window_event_di...
> ui/aura/window_event_dispatcher.cc:501:
> ui::GestureRecognizer::Get()->AckSyncTouchEvent(
> On 2014/12/08 14:49:30, tdresser wrote:
> > On 2014/12/07 17:25:43, sadrul (OOO - back on Wed) wrote:
> > > On 2014/12/05 16:44:31, tdresser wrote:
> > > > On 2014/12/02 15:54:10, sadrul wrote:
> > > > > Now that TouchEvent carries a unique-id with it, is there a reason to
> have
> > > > > AckSyncTouchEvent/AckAsyncTouchEvent in the public GR interface
anymore?
> > Why
> > > > not
> > > > > just AckTouchEvent()? The GR can decide whether or not to release the
> > > gesture
> > > > > events based on whether the touch-event acked is at the front of the
> queue
> > > or
> > > > > not. Would that work?
> > > > 
> > > > (Oops, I should have kept this conversation in one place.)
> > > > 
> > > > >> tdresser: Jared and I discussed this, and we decided that it was nice
> to
> > > have
> > > > the extra
> > > > >> verification that we're acking the correct event.
> > > > >> 
> > > > >> You're right that this is redundant, but we think the redundancy is
> worth
> > > the
> > > > >> extra confidence that the correct event is being acked.
> > > > >
> > > > > sadrul: My understanding is we can ack any touch event in the queue,
and
> > it
> > > > will do the
> > > > > right thing. Is that not the case?
> > > > 
> > > > That is the case, however, we still want to impose some constraints.
> Despite
> > > the
> > > > fact that we could ack any touch event in the queue, if our
understanding
> of
> > > the
> > > > system is correct, we should only be acking events at the front and back
> of
> > > the
> > > > queue. We want to double check that this understanding is correct.
> > > 
> > > Sure. But can't you do that in GR::AckTouchEvent() by verifying that the
> event
> > > being ack'ed is either at the front or back of the queue? Having a public
> API
> > > for this feels unclean (and can be confusing, e.g.
> > >
> >
>
https://codereview.chromium.org/680413006/diff/280001/ui/chromeos/touch_explo...)
> > 
> > Yeah, Jared and I discussed that. It's a little more work to get the unique
> > touch event id persisted into the async case.
> > 
> > Jared, what do you think? I think this is a bit safer, but its true it may
be
> a
> > little more confusing.
> 
> Since Aura is the only platform that makes the distinction, one thought would
be
> to have GestureProviderAura take the touch event and the ack (or
> GestureRecognizer), and have it be responsible for calling the right method.
> Would you even need to track id's in that case? Can't you just look at the
type
> of ack and whether sync handling was disabled?

As per offline discussion with jdduke, I'll land now, and we can iterate on the
API.

[tdresser, 2014-12-09 19:08:23]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-12-09 19:09:18]

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

[commit-bot: I haz the power, 2014-12-09 21:00:27]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2014-12-09 21:01:12]

Patchset 18 (id:??) landed as
https://crrev.com/8879cfca3b19c304004e34e6dec9629752cd3068
Cr-Commit-Position: refs/heads/master@{#307548}