Prepare for Unified Gesture Recognizer landing in Aura

Prepare for Unified Gesture Recognizer landing in Aura

This is the final cleanup before the unified gesture recognizer is turned on.

Tests are passing with the unified GR flag on and with the flag off.

BUG=332418
Test=GestureRecognizerTest.*, GestureProviderTest.*

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

[jdduke (slow), 2014-05-28 16:25:59]

https://codereview.chromium.org/306483003/diff/210001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right):

https://codereview.chromium.org/306483003/diff/210001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:90:
release_location,
Nit: Alignment.

https://codereview.chromium.org/306483003/diff/210001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_provider.cc (left):

https://codereview.chromium.org/306483003/diff/210001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:653:
DCHECK(!gesture.time.is_null());
Why was this removed?

https://codereview.chromium.org/306483003/diff/210001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_provider.cc (right):

https://codereview.chromium.org/306483003/diff/210001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:361:
provider_->Send(CreateGesture(ET_GESTURE_SCROLL_UPDATE,
Please cache GetBoundingBox(e2) before using.

https://codereview.chromium.org/306483003/diff/210001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:364:
GetBoundingBox(e2).CenterPoint().x(),
Just curious, why was this change necessary? Does the Aura UI not lock to a
specific layer when scrolling?

https://codereview.chromium.org/306483003/diff/210001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:783: GetBoundingBox(event)));
Cache GetBoundingBox?

https://codereview.chromium.org/306483003/diff/210001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_provider_unittest.cc (right):

https://codereview.chromium.org/306483003/diff/210001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider_unittest.cc:88: static
MockMotionEvent ObtainMotionEvent(base::TimeTicks event_time,
Hmm, I don't actually see where the 4 pointer case is used?

https://codereview.chromium.org/306483003/diff/210001/ui/events/gesture_detec...
File ui/events/gesture_detection/mock_motion_event.cc (right):

https://codereview.chromium.org/306483003/diff/210001/ui/events/gesture_detec...
ui/events/gesture_detection/mock_motion_event.cc:74: for (size_t i = 0; i <
pointer_count; ++i)
Ooph, good catch...

https://codereview.chromium.org/306483003/diff/210001/ui/events/gesture_detec...
File ui/events/gesture_detection/mock_motion_event.h (right):

https://codereview.chromium.org/306483003/diff/210001/ui/events/gesture_detec...
ui/events/gesture_detection/mock_motion_event.h:37: float x0,
See note from gesture_provider_unittest.cc, I'm not sure we actually use this
constructor?

https://codereview.chromium.org/306483003/diff/210001/ui/events/gestures/gest...
File ui/events/gestures/gesture_provider_aura.cc (right):

https://codereview.chromium.org/306483003/diff/210001/ui/events/gestures/gest...
ui/events/gestures/gesture_provider_aura.cc:79: ui::GestureEvent* event =
Let's use scoped_ptr<ui::GestureEvent> if we absolutely must heap allocate...
Why do we have to heap allocate again?

https://codereview.chromium.org/306483003/diff/210001/ui/events/gestures/gest...
File ui/events/gestures/gesture_recognizer_impl.cc (right):

https://codereview.chromium.org/306483003/diff/210001/ui/events/gestures/gest...
ui/events/gestures/gesture_recognizer_impl.cc:311: ScopedVector<GestureEvent>*
GestureRecognizerImpl::ProcessTouchEventForGesture(
I'm trying to understand how this signature changed without changing the header?
In any case, must the ui::GestureEvent objects persist? It seems a little odd to
both heap allocate each gesture AND heap allocate the container.  If the objects
must persist, you can just rely on ScopedVector being a "move-only" type (you
can call .Pass() like a scoped_ptr), and return an empty ScopedVector in the
NULL case.

https://codereview.chromium.org/306483003/diff/210001/ui/events/gestures/gest...
ui/events/gestures/gesture_recognizer_impl.cc:322:
GetGestureProviderForConsumer(target);
I'm a little worried about not doing eager gesture recognition here, I guess as
long as it works...

[tdresser, 2014-05-29 14:26:19]

https://codereview.chromium.org/306483003/diff/210001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right):

https://codereview.chromium.org/306483003/diff/210001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:90:
release_location,
On 2014/05/28 16:26:00, jdduke wrote:
> Nit: Alignment.

Done.

https://codereview.chromium.org/306483003/diff/210001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_provider.cc (left):

https://codereview.chromium.org/306483003/diff/210001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:653:
DCHECK(!gesture.time.is_null());
On 2014/05/28 16:26:00, jdduke wrote:
> Why was this removed?

There are a whole pile of tests which synthesize gesture events without
assigning them a time. In theory I could go through and update all the tests,
but I don't think it's worth it.

https://codereview.chromium.org/306483003/diff/210001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_provider.cc (right):

https://codereview.chromium.org/306483003/diff/210001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:361:
provider_->Send(CreateGesture(ET_GESTURE_SCROLL_UPDATE,
On 2014/05/28 16:26:00, jdduke wrote:
> Please cache GetBoundingBox(e2) before using.

Done.

https://codereview.chromium.org/306483003/diff/210001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:364:
GetBoundingBox(e2).CenterPoint().x(),
On 2014/05/28 16:26:00, jdduke wrote:
> Just curious, why was this change necessary? Does the Aura UI not lock to a
> specific layer when scrolling?

Multi-finger window dragging uses the scroll update location delta to determine
how far to drag the window. This lets us use the same drag logic as is used for
mouse
(https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/toplevel_wi...).

https://codereview.chromium.org/306483003/diff/210001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:783: GetBoundingBox(event)));
On 2014/05/28 16:26:00, jdduke wrote:
> Cache GetBoundingBox?

Done.

https://codereview.chromium.org/306483003/diff/210001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_provider_unittest.cc (right):

https://codereview.chromium.org/306483003/diff/210001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider_unittest.cc:88: static
MockMotionEvent ObtainMotionEvent(base::TimeTicks event_time,
On 2014/05/28 16:26:00, jdduke wrote:
> Hmm, I don't actually see where the 4 pointer case is used?

Oh goodness, you're right. I was using 4 for the begin / end tests at one point,
but then figured 3 was adequate.

Thanks for the catch.

https://codereview.chromium.org/306483003/diff/210001/ui/events/gesture_detec...
File ui/events/gesture_detection/mock_motion_event.h (right):

https://codereview.chromium.org/306483003/diff/210001/ui/events/gesture_detec...
ui/events/gesture_detection/mock_motion_event.h:37: float x0,
On 2014/05/28 16:26:00, jdduke wrote:
> See note from gesture_provider_unittest.cc, I'm not sure we actually use this
> constructor?

Removed...

https://codereview.chromium.org/306483003/diff/210001/ui/events/gestures/gest...
File ui/events/gestures/gesture_provider_aura.cc (right):

https://codereview.chromium.org/306483003/diff/210001/ui/events/gestures/gest...
ui/events/gestures/gesture_provider_aura.cc:79: ui::GestureEvent* event =
On 2014/05/28 16:26:00, jdduke wrote:
> Let's use scoped_ptr<ui::GestureEvent> if we absolutely must heap allocate...
> Why do we have to heap allocate again?

We need to return a ScopedVector of GestureEvents in
ProcessTouchEventForGesture. We can't do that if we stack allocate the gesture
here.

We could stack allocate for the |!handling_event| case, but I didn't think
duplicating the constructor call was worth it.

Done.

https://codereview.chromium.org/306483003/diff/210001/ui/events/gestures/gest...
File ui/events/gestures/gesture_recognizer_impl.cc (right):

https://codereview.chromium.org/306483003/diff/210001/ui/events/gestures/gest...
ui/events/gestures/gesture_recognizer_impl.cc:311: ScopedVector<GestureEvent>*
GestureRecognizerImpl::ProcessTouchEventForGesture(
On 2014/05/28 16:26:00, jdduke wrote:
> I'm trying to understand how this signature changed without changing the
header?
> In any case, must the ui::GestureEvent objects persist? It seems a little odd
to
> both heap allocate each gesture AND heap allocate the container.  If the
objects
> must persist, you can just rely on ScopedVector being a "move-only" type (you
> can call .Pass() like a scoped_ptr), and return an empty ScopedVector in the
> NULL case.

GestureSequence::Gestures is typedef'ed to ScopedVector<GestureEvent>*. I think
it probably makes sense to keep the same signature here as we had before, we can
clean this up in a followup patch.

[jdduke (slow), 2014-05-29 16:25:39]

https://codereview.chromium.org/306483003/diff/210001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_provider.cc (left):

https://codereview.chromium.org/306483003/diff/210001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:653:
DCHECK(!gesture.time.is_null());
On 2014/05/29 14:26:21, tdresser wrote:
> On 2014/05/28 16:26:00, jdduke wrote:
> > Why was this removed?
> 
> There are a whole pile of tests which synthesize gesture events without
> assigning them a time. In theory I could go through and update all the tests,
> but I don't think it's worth it.

My only worry here is that gesture detection is *very* timestamp sensitive. If
any of those tests are actually testing generated gestures, a stream of zeroed
timestamps may not yield what we want? If we don't care about the output
gestures, even a base::TimeTicks::Now() would be better so that we can at least
verify runtime behavior for Chrome builds.

https://codereview.chromium.org/306483003/diff/210001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_provider.cc (right):

https://codereview.chromium.org/306483003/diff/210001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:364:
GetBoundingBox(e2).CenterPoint().x(),
On 2014/05/29 14:26:21, tdresser wrote:
> On 2014/05/28 16:26:00, jdduke wrote:
> > Just curious, why was this change necessary? Does the Aura UI not lock to a
> > specific layer when scrolling?
> 
> Multi-finger window dragging uses the scroll update location delta to
determine
> how far to drag the window. This lets us use the same drag logic as is used
for
> mouse
>
(https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/toplevel_wi...).

sgtm.

https://codereview.chromium.org/306483003/diff/210001/ui/events/gestures/gest...
File ui/events/gestures/gesture_recognizer_impl.cc (right):

https://codereview.chromium.org/306483003/diff/210001/ui/events/gestures/gest...
ui/events/gestures/gesture_recognizer_impl.cc:311: ScopedVector<GestureEvent>*
GestureRecognizerImpl::ProcessTouchEventForGesture(
On 2014/05/29 14:26:21, tdresser wrote:
> On 2014/05/28 16:26:00, jdduke wrote:
> > I'm trying to understand how this signature changed without changing the
> header?
> > In any case, must the ui::GestureEvent objects persist? It seems a little
odd
> to
> > both heap allocate each gesture AND heap allocate the container.  If the
> objects
> > must persist, you can just rely on ScopedVector being a "move-only" type
(you
> > can call .Pass() like a scoped_ptr), and return an empty ScopedVector in the
> > NULL case.
> 
> GestureSequence::Gestures is typedef'ed to ScopedVector<GestureEvent>*. I
think
> it probably makes sense to keep the same signature here as we had before, we
can
> clean this up in a followup patch.

I see, that's really unfortunate.  At the least, could you file a bug? Seems to
me the gestures don't need to be heap allocated, and neither does the vector,
i.e., we could just return std::vector<ui::GestureEvent> and as long as we're
using swap and RVO properly, there should be much less overhead (we'll have just
1 heap allocation, instead of 1 for vector storage, 1 for the vector itself, and
1 for every member event =/).  It's a shame we don't have a SmallVector
construct as we do in Blink (hmm.... tempting...).

Keeping as is seems fine for now, as we're just matching existing behavior.

https://codereview.chromium.org/306483003/diff/270001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_provider.cc (right):

https://codereview.chromium.org/306483003/diff/270001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:359: const gfx::RectF&
bounding_box = GetBoundingBox(e2);
Wait, why are capturing a variable returned by value with a ref? What you had
looked OK.

https://codereview.chromium.org/306483003/diff/270001/ui/events/gesture_detec...
File ui/events/gesture_detection/mock_motion_event.h (right):

https://codereview.chromium.org/306483003/diff/270001/ui/events/gesture_detec...
ui/events/gesture_detection/mock_motion_event.h:15: enum { MAX_POINTERS = 4 };
Hmm, should we go back to 3 here?

https://codereview.chromium.org/306483003/diff/270001/ui/events/gestures/gest...
File ui/events/gestures/gesture_provider_aura.cc (right):

https://codereview.chromium.org/306483003/diff/270001/ui/events/gestures/gest...
ui/events/gestures/gesture_provider_aura.cc:101: ScopedVector<GestureEvent>*
old_pending_gestures =
Returning NULL is OK, right? If so, please check for an empty 
|pending_gestures_| before allocating the new ScopedVector, and if empty return
NULL.

[tdresser, 2014-05-29 16:38:37]

davemoore@, can you take a look at:
ash/shelf/shelf_layout_manager_unittest.cc

pkasting@, can you take a look at:
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc

[Peter Kasting, 2014-05-29 18:05:43]

LGTM

https://codereview.chromium.org/306483003/diff/290001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right):

https://codereview.chromium.org/306483003/diff/290001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:92:
base::TimeDelta::FromMilliseconds(10));
Nit: Since the code just above combines args onto fewer lines, I suggest doing
the same here for consistency.

[tdresser, 2014-05-30 14:05:37]

https://codereview.chromium.org/306483003/diff/210001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_provider.cc (left):

https://codereview.chromium.org/306483003/diff/210001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:653:
DCHECK(!gesture.time.is_null());
On 2014/05/29 16:25:40, jdduke wrote:
> On 2014/05/29 14:26:21, tdresser wrote:
> > On 2014/05/28 16:26:00, jdduke wrote:
> > > Why was this removed?
> > 
> > There are a whole pile of tests which synthesize gesture events without
> > assigning them a time. In theory I could go through and update all the
tests,
> > but I don't think it's worth it.
> 
> My only worry here is that gesture detection is *very* timestamp sensitive. If
> any of those tests are actually testing generated gestures, a stream of zeroed
> timestamps may not yield what we want? If we don't care about the output
> gestures, even a base::TimeTicks::Now() would be better so that we can at
least
> verify runtime behavior for Chrome builds. 

Done.

https://codereview.chromium.org/306483003/diff/210001/ui/events/gestures/gest...
File ui/events/gestures/gesture_recognizer_impl.cc (right):

https://codereview.chromium.org/306483003/diff/210001/ui/events/gestures/gest...
ui/events/gestures/gesture_recognizer_impl.cc:311: ScopedVector<GestureEvent>*
GestureRecognizerImpl::ProcessTouchEventForGesture(
On 2014/05/29 16:25:40, jdduke wrote:
> On 2014/05/29 14:26:21, tdresser wrote:
> > On 2014/05/28 16:26:00, jdduke wrote:
> > > I'm trying to understand how this signature changed without changing the
> > header?
> > > In any case, must the ui::GestureEvent objects persist? It seems a little
> odd
> > to
> > > both heap allocate each gesture AND heap allocate the container.  If the
> > objects
> > > must persist, you can just rely on ScopedVector being a "move-only" type
> (you
> > > can call .Pass() like a scoped_ptr), and return an empty ScopedVector in
the
> > > NULL case.
> > 
> > GestureSequence::Gestures is typedef'ed to ScopedVector<GestureEvent>*. I
> think
> > it probably makes sense to keep the same signature here as we had before, we
> can
> > clean this up in a followup patch.
> 
> I see, that's really unfortunate.  At the least, could you file a bug? Seems
to
> me the gestures don't need to be heap allocated, and neither does the vector,
> i.e., we could just return std::vector<ui::GestureEvent> and as long as we're
> using swap and RVO properly, there should be much less overhead (we'll have
just
> 1 heap allocation, instead of 1 for vector storage, 1 for the vector itself,
and
> 1 for every member event =/).  It's a shame we don't have a SmallVector
> construct as we do in Blink (hmm.... tempting...).
> 
> Keeping as is seems fine for now, as we're just matching existing behavior.

Bug filed: crbug.com/378817.

https://codereview.chromium.org/306483003/diff/270001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_provider.cc (right):

https://codereview.chromium.org/306483003/diff/270001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:359: const gfx::RectF&
bounding_box = GetBoundingBox(e2);
On 2014/05/29 16:25:40, jdduke wrote:
> Wait, why are capturing a variable returned by value with a ref? What you had
> looked OK.

Done.

https://codereview.chromium.org/306483003/diff/270001/ui/events/gesture_detec...
File ui/events/gesture_detection/mock_motion_event.h (right):

https://codereview.chromium.org/306483003/diff/270001/ui/events/gesture_detec...
ui/events/gesture_detection/mock_motion_event.h:15: enum { MAX_POINTERS = 4 };
On 2014/05/29 16:25:40, jdduke wrote:
> Hmm, should we go back to 3 here?

Whoops, thanks.

https://codereview.chromium.org/306483003/diff/270001/ui/events/gestures/gest...
File ui/events/gestures/gesture_provider_aura.cc (right):

https://codereview.chromium.org/306483003/diff/270001/ui/events/gestures/gest...
ui/events/gestures/gesture_provider_aura.cc:101: ScopedVector<GestureEvent>*
old_pending_gestures =
On 2014/05/29 16:25:40, jdduke wrote:
> Returning NULL is OK, right? If so, please check for an empty 
> |pending_gestures_| before allocating the new ScopedVector, and if empty
return
> NULL.

Done.

https://codereview.chromium.org/306483003/diff/290001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right):

https://codereview.chromium.org/306483003/diff/290001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:92:
base::TimeDelta::FromMilliseconds(10));
On 2014/05/29 18:05:44, Peter Kasting wrote:
> Nit: Since the code just above combines args onto fewer lines, I suggest doing
> the same here for consistency.

Done.

[jdduke (slow), 2014-05-30 14:56:27]

Thanks for hunting down the null timestamps! ui/events/gesture_detection lgtm
(with one not for a browsertest).

https://codereview.chromium.org/306483003/diff/320001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right):

https://codereview.chromium.org/306483003/diff/320001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:85:
ui::ET_TOUCH_PRESSED, press_location, 5, ui::EventTimeForNow());
Hmm, it's a bit odd using ui::EventTimeForNow() here but not subsequently, as
you're breaking monotonicity.  You might assign a |timestamp =
ui::EventTimeForNow()| and use that with the |FromMilliseconds(X)| offsets
below? I believe that's what is done in the
web_contents_view_aura_browsertest.cc.

[jdduke (slow), 2014-05-30 15:04:54]

https://codereview.chromium.org/306483003/diff/340001/content/common/input/sy...
File content/common/input/synthetic_web_input_event_builders.cc (left):

https://codereview.chromium.org/306483003/diff/340001/content/common/input/sy...
content/common/input/synthetic_web_input_event_builders.cc:150:
SyntheticWebTouchEvent::SyntheticWebTouchEvent() : WebTouchEvent() {}
I am curious what test failures this was causing... is it because the gesture
detector runs on ack'ed events with Aura, rather than the source events?

[tdresser, 2014-05-30 16:04:05]

sadrul@, can you take a look at:
ui/events/ (except for gesture_detection)
ui/aura

jochen@, can you take a look at:
content/browser/web_contents/web_contents_view_aura_browsertest.cc

https://codereview.chromium.org/306483003/diff/320001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right):

https://codereview.chromium.org/306483003/diff/320001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:85:
ui::ET_TOUCH_PRESSED, press_location, 5, ui::EventTimeForNow());
On 2014/05/30 14:56:27, jdduke wrote:
> Hmm, it's a bit odd using ui::EventTimeForNow() here but not subsequently, as
> you're breaking monotonicity.  You might assign a |timestamp =
> ui::EventTimeForNow()| and use that with the |FromMilliseconds(X)| offsets
> below? I believe that's what is done in the
> web_contents_view_aura_browsertest.cc.

Definitely a mistake, thanks for catching it.

https://codereview.chromium.org/306483003/diff/340001/content/common/input/sy...
File content/common/input/synthetic_web_input_event_builders.cc (left):

https://codereview.chromium.org/306483003/diff/340001/content/common/input/sy...
content/common/input/synthetic_web_input_event_builders.cc:150:
SyntheticWebTouchEvent::SyntheticWebTouchEvent() : WebTouchEvent() {}
On 2014/05/30 15:04:54, jdduke wrote:
> I am curious what test failures this was causing... is it because the gesture
> detector runs on ack'ed events with Aura, rather than the source events? 

    TouchInputBrowserTest.MultiPointTouchPress
    TouchInputBrowserTest.TouchHandlerConsume
    TouchInputBrowserTest.TouchHandlerNoConsume

You're right - this should stop being an issue when we switch to being eager.

[jochen (gone - plz use gerrit), 2014-06-02 07:18:02]

browsertest lgtm

[tdresser, 2014-06-02 12:40:39]

skuhn@, PTAL at
ash/shelf/shelf_layout_manager_unittest.cc

[sadrul, 2014-06-02 13:31:22]

https://codereview.chromium.org/306483003/diff/350001/ui/aura/gestures/gestur...
File ui/aura/gestures/gesture_recognizer_unittest.cc (right):

https://codereview.chromium.org/306483003/diff/350001/ui/aura/gestures/gestur...
ui/aura/gestures/gesture_recognizer_unittest.cc:692: virtual void TearDown()
OVERRIDE {
You can remove this override too

https://codereview.chromium.org/306483003/diff/350001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher_unittest.cc (right):

https://codereview.chromium.org/306483003/diff/350001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher_unittest.cc:508: }
Use std::find instead

https://codereview.chromium.org/306483003/diff/350001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher_unittest.cc:1597:
EXPECT_TRUE(recorder.HasReceivedEvent(ui::ET_GESTURE_END));
Can we check that the TAP_CANCEL and END are received in the correct order?

https://codereview.chromium.org/306483003/diff/350001/ui/events/gestures/gest...
File ui/events/gestures/gesture_provider_aura.h (right):

https://codereview.chromium.org/306483003/diff/350001/ui/events/gestures/gest...
ui/events/gestures/gesture_provider_aura.h:37: ScopedVector<GestureEvent>*
GetAndResetPendingGestures();
This should go above the override (i.e. in line ~35)

https://codereview.chromium.org/306483003/diff/350001/ui/events/test/events_t...
File ui/events/test/events_test_utils_x11.cc (right):

https://codereview.chromium.org/306483003/diff/350001/ui/events/test/events_t...
ui/events/test/events_test_utils_x11.cc:246: SetUpValuators(valuators);
Did you mean to return from here?

[tdresser, 2014-06-02 14:00:38]

https://codereview.chromium.org/306483003/diff/350001/ui/aura/gestures/gestur...
File ui/aura/gestures/gesture_recognizer_unittest.cc (right):

https://codereview.chromium.org/306483003/diff/350001/ui/aura/gestures/gestur...
ui/aura/gestures/gesture_recognizer_unittest.cc:692: virtual void TearDown()
OVERRIDE {
On 2014/06/02 13:31:23, sadrul wrote:
> You can remove this override too

Done.

https://codereview.chromium.org/306483003/diff/350001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher_unittest.cc (right):

https://codereview.chromium.org/306483003/diff/350001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher_unittest.cc:508: }
On 2014/06/02 13:31:23, sadrul wrote:
> Use std::find instead

Done.

https://codereview.chromium.org/306483003/diff/350001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher_unittest.cc:1597:
EXPECT_TRUE(recorder.HasReceivedEvent(ui::ET_GESTURE_END));
On 2014/06/02 13:31:23, sadrul wrote:
> Can we check that the TAP_CANCEL and END are received in the correct order?

The unified GR at this point will fire the GESTURE_END before the TAP_CANCEL.
I've updated the comments in event_constants.h to indicate that we don't
guarantee that GESTURE_END will be the last event sent.

I'm not aware of any cases where we rely on GESTURE_END being the last event
sent. Do you know of any cases where we do rely on this?

https://codereview.chromium.org/306483003/diff/350001/ui/events/gestures/gest...
File ui/events/gestures/gesture_provider_aura.h (right):

https://codereview.chromium.org/306483003/diff/350001/ui/events/gestures/gest...
ui/events/gestures/gesture_provider_aura.h:37: ScopedVector<GestureEvent>*
GetAndResetPendingGestures();
On 2014/06/02 13:31:23, sadrul wrote:
> This should go above the override (i.e. in line ~35)

Done.

https://codereview.chromium.org/306483003/diff/350001/ui/events/test/events_t...
File ui/events/test/events_test_utils_x11.cc (right):

https://codereview.chromium.org/306483003/diff/350001/ui/events/test/events_t...
ui/events/test/events_test_utils_x11.cc:246: SetUpValuators(valuators);
On 2014/06/02 13:31:23, sadrul wrote:
> Did you mean to return from here?

Thanks for catching that.
Done.

[sadrul, 2014-06-02 14:28:17]

lgtm

https://codereview.chromium.org/306483003/diff/350001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher_unittest.cc (right):

https://codereview.chromium.org/306483003/diff/350001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher_unittest.cc:1597:
EXPECT_TRUE(recorder.HasReceivedEvent(ui::ET_GESTURE_END));
On 2014/06/02 14:00:38, tdresser wrote:
> On 2014/06/02 13:31:23, sadrul wrote:
> > Can we check that the TAP_CANCEL and END are received in the correct order?
> 
> The unified GR at this point will fire the GESTURE_END before the TAP_CANCEL.
> I've updated the comments in event_constants.h to indicate that we don't
> guarantee that GESTURE_END will be the last event sent.
> 
> I'm not aware of any cases where we rely on GESTURE_END being the last event
> sent. Do you know of any cases where we do rely on this? 

Probably not, but this seems unfortunate. Can you file a bug for fixing this?

[Mr4D (OOO till 08-26), 2014-06-02 14:32:58]

lgtm

https://codereview.chromium.org/306483003/diff/370001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager_unittest.cc (right):

https://codereview.chromium.org/306483003/diff/370001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager_unittest.cc:423: kTimeDelta, kNumScrollSteps,
Our style guide requires to put each parameter into a new line. But I am fine
with it the way it is.

[tdresser, 2014-06-02 14:43:46]

https://codereview.chromium.org/306483003/diff/350001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher_unittest.cc (right):

https://codereview.chromium.org/306483003/diff/350001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher_unittest.cc:1597:
EXPECT_TRUE(recorder.HasReceivedEvent(ui::ET_GESTURE_END));
On 2014/06/02 14:28:18, sadrul wrote:
> On 2014/06/02 14:00:38, tdresser wrote:
> > On 2014/06/02 13:31:23, sadrul wrote:
> > > Can we check that the TAP_CANCEL and END are received in the correct
order?
> > 
> > The unified GR at this point will fire the GESTURE_END before the
TAP_CANCEL.
> > I've updated the comments in event_constants.h to indicate that we don't
> > guarantee that GESTURE_END will be the last event sent.
> > 
> > I'm not aware of any cases where we rely on GESTURE_END being the last event
> > sent. Do you know of any cases where we do rely on this? 
> 
> Probably not, but this seems unfortunate. Can you file a bug for fixing this?

crbug.com/379772

https://codereview.chromium.org/306483003/diff/370001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager_unittest.cc (right):

https://codereview.chromium.org/306483003/diff/370001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager_unittest.cc:423: kTimeDelta, kNumScrollSteps,
On 2014/06/02 14:32:59, Mr4D wrote:
> Our style guide requires to put each parameter into a new line. But I am fine
> with it the way it is.

Done.

[tdresser, 2014-06-02 14:43:53]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-06-02 14:44:42]

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

[commit-bot: I haz the power, 2014-06-02 18:02:53]

Change committed as 274277