Unified Gesture Recognizer

Unified Gesture Recognizer

This adds the unified GR for Aura behind the --use-unified-gesture-detector flag.

BUG=332418
TEST=GestureRecognizer/GestureRecognizerTest.*
  MotionEventUITest.*

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

[tdresser, 2014-05-01 14:31:20]

I'm still working out some windows compilation issues, but I shouldn't need to
change anything major to get that working.

sadrul@, can you take a look at:
ui/aura/aura.gyp
ui/aura/gestures/gesture_recognizer_unittest.cc
ui/aura/window_event_dispatcher.h
ui/aura/window_event_dispatcher.cc
ui/events/event_switches.h
ui/events/event_switches.cc
ui/events/gestures/gesture_configuration.h	
ui/events/gestures/gesture_configuration.cc
ui/events/gestures/gesture_recognizer_impl.h	
ui/events/gestures/gesture_recognizer_impl.cc	
ui/events/gestures/gesture_types.h

I'll get you to take a look at ui/events/events.gyp once I've figured out my
linking problems.

GestureRecognizerImpl is pretty ugly right now. Cleaning it up significantly
would require an additional layer of abstraction which would be thrown away as
soon as we permanently switch to the unified GR, so I don't think it's worth it.
Let me know what you think.

[tdresser, 2014-05-02 13:24:15]

jdduke@, can you take a look at ui/events/gesture_detection?

[jdduke (slow), 2014-05-02 17:56:43]

Nice.  I think my only structural comment is that it might make sense to use the
|Aura| suffix with the new provider/event types (using UI when we're already in
ui:: might be a little confusing, and for all intents and purposes the new types
are for Aura only).  In that case, would it make sense to put the new types in
the gestures/ folder (for now anyway)?

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
File ui/events/gesture_detection/filtered_gesture_provider.h (right):

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/filtered_gesture_provider.h:19: private
ui::GestureProviderClient {
Was this intentional? I usually take private inheritance to mean "has-a" or
"implemented in terms of", and I'm not sure how that applies here?

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event_ui.cc (right):

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.cc:53: SetCachedAction(touch);
UpdateCachedAction?

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.cc:54: last_touch_time_ =
touch.time_stamp();
We should probably make this member a |TimeTicks|, and do the conversion upon
construction/update.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.cc:64: DCHECK(cached_action_ ==
ACTION_POINTER_DOWN ||
Hmm, we should probably make a note in the MotionEvent interface that this call
is valid only for ACTION_PIONTER_UP/ACTION_POINTER_DOWN events.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.cc:66: DCHECK(0 <=
cached_action_index_ &&
DCHECK_GE + DCHECK_LT

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.cc:74: DCHECK(pointer_index <
pointer_count_);
DCHECK_LT for all |pointer_index| checks.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.cc:107: NOTIMPLEMENTED();
Hmm, I think you want |NOTIMPLEMENTED()| before the return for each of these
functions?

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.cc:130: last_touch_time_,
Might as well return scoped_ptr<MotionEvent>(new ...)

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.cc:139:
static_cast<MotionEventUI*>(clone.get())->cached_action_ = ACTION_CANCEL;
I'm not sure re-using clone buys us much here.  Let's just use the constructor
directly, but pass in ACTION_CANCEL and -1 for the cached_action_index_.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.cc:175: void
MotionEventUI::AddTouch(const TouchEvent& touch) {
Would it ever make sense for |AddTouch()| to be the caller of
|CleanupRemovedTouchPoints()| ? So it's in charge of flushing?

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.cc:179: PointData* point_data =
&active_touches_[pointer_count_];
What if we had a free function |GetPointDataFromTouchEvent()|, then we just call

active_touches_[pointer_count_] = GetPointDataFromTouchEvent(touch);

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.cc:187: PointData* point_data =
&active_touches_[id_to_index_[touch.touch_id()]];
What if touch_id() is not contained in id_to_index_? We should probably have a
short helper function |GetIndexFromId()| that does some kind of validation.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.cc:192: switch (touch.type()) {
Might be worth another DCHECK(pointer_count_) here.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event_ui.h (right):

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.h:17: namespace ui {
Nit: Line before namespace

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.h:19: // Implementation of
MotionEvent which takes a stream of ui::Events.
ui::TouchEvents

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.h:20: class EVENTS_EXPORT
MotionEventUI : public MotionEvent {
Would MotionEventAura be more appropriate?

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.h:27: virtual int GetId() const
OVERRIDE;
// MotionEvent implementation.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.h:61: float x;
Let's give this a default constructor that zero's everything, else asan
complain.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.h:93: std::map<int, int>
id_to_index_;
Would a base::SmallMap be more appropriate here? Do we know the range of
touch_id()?

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event_ui_unittest.cc (right):

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui_unittest.cc:16: ui::TouchEvent
TouchWithPosition(ui::EventType type, int id,
Nit, this probably needs a git cl format.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
File ui/events/gesture_detection/ui_gesture_provider.cc (right):

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/ui_gesture_provider.cc:69: bool result =
FilteredGestureProvider::OnTouchEvent(motion_event_);
I think this will be a little cleaner if the FilteredGestureProvider is a member
(see header comments).

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
File ui/events/gesture_detection/ui_gesture_provider.h (right):

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/ui_gesture_provider.h:21: virtual void
OnGestureEvent(GestureEvent* event) = 0;
scoped_ptr?

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/ui_gesture_provider.h:26: class EVENTS_EXPORT
UIGestureProvider : public FilteredGestureProvider {
GestureProviderAura?

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/ui_gesture_provider.h:27: public:
Hmm, can we compose FilteredGestureProvider and instead implement
ui::GestureProviderClient directly? Passing this to the member filtered
provider? Then we don't need the GestureProviderClientConverter, we can just do
the conversion directly.

[jdduke (slow), 2014-05-02 17:58:41]

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
File ui/events/gesture_detection/filtered_gesture_provider.h (right):

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/filtered_gesture_provider.h:19: private
ui::GestureProviderClient {
On 2014/05/02 17:56:43, jdduke wrote:
> Was this intentional? I usually take private inheritance to mean "has-a" or
> "implemented in terms of", and I'm not sure how that applies here?

I see, you did use it in that way... I'd really prefer we compose rather than
inherit from FilteredGestureProvider, if possible.

[tdresser, 2014-05-05 15:42:35]

Some linking stuff to clean up due to the renames to _aura, but in the meantime,
feel like taking another look?

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
File ui/events/gesture_detection/filtered_gesture_provider.h (right):

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/filtered_gesture_provider.h:19: private
ui::GestureProviderClient {
On 2014/05/02 17:58:42, jdduke wrote:
> On 2014/05/02 17:56:43, jdduke wrote:
> > Was this intentional? I usually take private inheritance to mean "has-a" or
> > "implemented in terms of", and I'm not sure how that applies here?
> 
> I see, you did use it in that way... I'd really prefer we compose rather than
> inherit from FilteredGestureProvider, if possible.

Yup, this is broken, we do want an "is-a" relationship, as we pass the
FilteredGestureProvider to the GestureProvider as a GestureProviderClient.

Fixed.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event_ui.cc (right):

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.cc:53: SetCachedAction(touch);
On 2014/05/02 17:56:43, jdduke wrote:
> UpdateCachedAction?

Done.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.cc:54: last_touch_time_ =
touch.time_stamp();
On 2014/05/02 17:56:43, jdduke wrote:
> We should probably make this member a |TimeTicks|, and do the conversion upon
> construction/update.

Done.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.cc:64: DCHECK(cached_action_ ==
ACTION_POINTER_DOWN ||
On 2014/05/02 17:56:43, jdduke wrote:
> Hmm, we should probably make a note in the MotionEvent interface that this
call
> is valid only for ACTION_PIONTER_UP/ACTION_POINTER_DOWN events.

Done.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.cc:66: DCHECK(0 <=
cached_action_index_ &&
On 2014/05/02 17:56:43, jdduke wrote:
> DCHECK_GE + DCHECK_LT

Done.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.cc:74: DCHECK(pointer_index <
pointer_count_);
On 2014/05/02 17:56:43, jdduke wrote:
> DCHECK_LT for all |pointer_index| checks.

Done.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.cc:107: NOTIMPLEMENTED();
On 2014/05/02 17:56:43, jdduke wrote:
> Hmm, I think you want |NOTIMPLEMENTED()| before the return for each of these
> functions?

Pffft, done.
Thanks.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.cc:130: last_touch_time_,
On 2014/05/02 17:56:43, jdduke wrote:
> Might as well return scoped_ptr<MotionEvent>(new ...)

Done.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.cc:139:
static_cast<MotionEventUI*>(clone.get())->cached_action_ = ACTION_CANCEL;
On 2014/05/02 17:56:43, jdduke wrote:
> I'm not sure re-using clone buys us much here.  Let's just use the constructor
> directly, but pass in ACTION_CANCEL and -1 for the cached_action_index_.

Done.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.cc:175: void
MotionEventUI::AddTouch(const TouchEvent& touch) {
On 2014/05/02 17:56:43, jdduke wrote:
> Would it ever make sense for |AddTouch()| to be the caller of
> |CleanupRemovedTouchPoints()| ? So it's in charge of flushing? 

In GestureProviderAura::OnTouchEvent, where we ignore invalid touch streams, we
need to know the flushed data.

AddTouch will be called after those checks occur, so I don't think that approach
will work.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.cc:179: PointData* point_data =
&active_touches_[pointer_count_];
On 2014/05/02 17:56:43, jdduke wrote:
> What if we had a free function |GetPointDataFromTouchEvent()|, then we just
call
> 
> active_touches_[pointer_count_] = GetPointDataFromTouchEvent(touch);

Can't quite be free, since PointData is (and I believe should continue to be)
private.
Made it static.

Done.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.cc:187: PointData* point_data =
&active_touches_[id_to_index_[touch.touch_id()]];
On 2014/05/02 17:56:43, jdduke wrote:
> What if touch_id() is not contained in id_to_index_? We should probably have a
> short helper function |GetIndexFromId()| that does some kind of validation.

Done.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.cc:192: switch (touch.type()) {
On 2014/05/02 17:56:43, jdduke wrote:
> Might be worth another DCHECK(pointer_count_) here.

Done.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event_ui.h (right):

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.h:17: namespace ui {
On 2014/05/02 17:56:43, jdduke wrote:
> Nit: Line before namespace

Done.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.h:19: // Implementation of
MotionEvent which takes a stream of ui::Events.
On 2014/05/02 17:56:43, jdduke wrote:
> ui::TouchEvents

Done.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.h:20: class EVENTS_EXPORT
MotionEventUI : public MotionEvent {
On 2014/05/02 17:56:43, jdduke wrote:
> Would MotionEventAura be more appropriate?

Done.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.h:27: virtual int GetId() const
OVERRIDE;
On 2014/05/02 17:56:43, jdduke wrote:
> // MotionEvent implementation.

Done.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.h:61: float x;
On 2014/05/02 17:56:43, jdduke wrote:
> Let's give this a default constructor that zero's everything, else asan
> complain.

Done.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui.h:93: std::map<int, int>
id_to_index_;
On 2014/05/02 17:56:43, jdduke wrote:
> Would a base::SmallMap be more appropriate here? Do we know the range of
> touch_id()?

I switched to iterating through |active_touches_|. |pointer_count_| will stay
pretty low.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
File ui/events/gesture_detection/motion_event_ui_unittest.cc (right):

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/motion_event_ui_unittest.cc:16: ui::TouchEvent
TouchWithPosition(ui::EventType type, int id,
On 2014/05/02 17:56:43, jdduke wrote:
> Nit, this probably needs a git cl format.

Done.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
File ui/events/gesture_detection/ui_gesture_provider.cc (right):

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/ui_gesture_provider.cc:69: bool result =
FilteredGestureProvider::OnTouchEvent(motion_event_);
On 2014/05/02 17:56:43, jdduke wrote:
> I think this will be a little cleaner if the FilteredGestureProvider is a
member
> (see header comments).

Done.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
File ui/events/gesture_detection/ui_gesture_provider.h (right):

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/ui_gesture_provider.h:21: virtual void
OnGestureEvent(GestureEvent* event) = 0;
On 2014/05/02 17:56:43, jdduke wrote:
> scoped_ptr?

Yeah, that probably is clearer.
I've let the GestureRecognizerImpl dispose of these events for now - in the aura
GR, GestureSequence disposes of events which are dispatched in this way.

Does this seem reasonable?

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/ui_gesture_provider.h:26: class EVENTS_EXPORT
UIGestureProvider : public FilteredGestureProvider {
On 2014/05/02 17:56:43, jdduke wrote:
> GestureProviderAura? 

Done.

https://codereview.chromium.org/251543003/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/ui_gesture_provider.h:27: public:
On 2014/05/02 17:56:43, jdduke wrote:
> Hmm, can we compose FilteredGestureProvider and instead implement
> ui::GestureProviderClient directly? Passing this to the member filtered
> provider? Then we don't need the GestureProviderClientConverter, we can just
do
> the conversion directly.

Much, much better. Thanks for catching that!
Done.

[jdduke (slow), 2014-05-05 16:57:08]

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/gest...
File ui/events/gestures/gesture_configuration.h (right):

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/gest...
ui/events/gestures/gesture_configuration.h:17: class EVENTS_BASE_EXPORT
GestureConfiguration {
Hmm, do we want this in events_base? Android doesn't use this, does iOS or Mac?
When we do move it to events_base, I think I'd prefer that all platforms use it
for the relevant constants.

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

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/gest...
ui/events/gestures/gesture_provider_aura.cc:43:
DCHECK(motion_event_.GetPointerCount());
Probably don't need this DCHECK as we don't use the pointer count here
(GestureProvider makes the necessary DCHECK).

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/gest...
ui/events/gestures/gesture_provider_aura.cc:50: return
filtered_gesture_provider_.OnTouchEventAck(event_consumed);
Nit: no return.

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

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/gest...
ui/events/gestures/gesture_provider_aura.h:33: const MotionEventAura&
get_pointer_state() { return motion_event_; }
Nitty nit: Normally the getter name should match the variable name in some
fashion? I would say either rename motion_event_ to pointer_state_, or make it
non-inline.

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/gest...
ui/events/gestures/gesture_provider_aura.h:43: int flags_;
Maybe last_touch_event_flags_ or something like that?

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

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/gest...
ui/events/gestures/gesture_recognizer_impl.cc:362: scoped_ptr<GestureEvent>
gesture_event = event.Pass();
Nit: No need to Pass() here, we can just use |event.get()|.  Maybe put a note
that |DispatchGestureEvent()| does the cleanup? (unless that's the way Aura
always works with events, where passing the pointer hands off ownership).

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/moti...
File ui/events/gestures/motion_event_aura.cc (right):

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura.cc:150: last_touch_time_ -
base::TimeTicks(),
We should probably just take a TimeTicks in the constructor?

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura.cc:150: last_touch_time_ -
base::TimeTicks(),
Looks like we need a reformat here for indentation?

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura.cc:228: size_t
MotionEventAura::GetIndexFromId(int id) {
This seems pretty reasonable.  It's O(N) but N is bounded small, and the call
happens at most 1 or 2x per new TouchEvent.

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/moti...
File ui/events/gestures/motion_event_aura.h (right):

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura.h:28: // MotionEventAura implementation.
Hmm, think this get the rename treatment, "MotionEvent implementation."

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura.h:64: PointData() {
Nit: Constructor implementation in the .cc file, and might as well use
initializer list.

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura.h:91: size_t GetIndexFromId(int id);
Nit: const method.

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/moti...
File ui/events/gestures/motion_event_aura_unittest.cc (right):

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura_unittest.cc:20: float r,
For my sanity could you use full names for r(adius) and p(ressure)? =/

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura_unittest.cc:104: // been removed.
Hmm, I don't see any |GetActionIndex()| calls in this test?

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura_unittest.cc:286: scoped_ptr<MotionEvent>
clone = event.Cancel();
Nit: Maybe rename clone to cancel?

[tdresser, 2014-05-05 17:45:13]

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/gest...
File ui/events/gestures/gesture_configuration.h (right):

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/gest...
ui/events/gestures/gesture_configuration.h:17: class EVENTS_BASE_EXPORT
GestureConfiguration {
On 2014/05/05 16:57:09, jdduke wrote:
> Hmm, do we want this in events_base? Android doesn't use this, does iOS or
Mac?
> When we do move it to events_base, I think I'd prefer that all platforms use
it
> for the relevant constants.

Let's leave cleaning that up until we sort out the gesture_config_helper*
locations.

I think it's reasonable to leave that until after this has landed?

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

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/gest...
ui/events/gestures/gesture_provider_aura.cc:43:
DCHECK(motion_event_.GetPointerCount());
On 2014/05/05 16:57:09, jdduke wrote:
> Probably don't need this DCHECK as we don't use the pointer count here
> (GestureProvider makes the necessary DCHECK).

Done.

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/gest...
ui/events/gestures/gesture_provider_aura.cc:50: return
filtered_gesture_provider_.OnTouchEventAck(event_consumed);
On 2014/05/05 16:57:09, jdduke wrote:
> Nit: no return.

Done.

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

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/gest...
ui/events/gestures/gesture_provider_aura.h:33: const MotionEventAura&
get_pointer_state() { return motion_event_; }
On 2014/05/05 16:57:09, jdduke wrote:
> Nitty nit: Normally the getter name should match the variable name in some
> fashion? I would say either rename motion_event_ to pointer_state_, or make it
> non-inline.

Done.

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/gest...
ui/events/gestures/gesture_provider_aura.h:43: int flags_;
On 2014/05/05 16:57:09, jdduke wrote:
> Maybe last_touch_event_flags_ or something like that?

Done.

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

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/gest...
ui/events/gestures/gesture_recognizer_impl.cc:362: scoped_ptr<GestureEvent>
gesture_event = event.Pass();
On 2014/05/05 16:57:09, jdduke wrote:
> Nit: No need to Pass() here, we can just use |event.get()|.  Maybe put a note
> that |DispatchGestureEvent()| does the cleanup? (unless that's the way Aura
> always works with events, where passing the pointer hands off ownership).

|DispatchGestureEvent()| doesn't do the cleanup. In this version of the CL, the
cleanup is happening right here, after the DispatchGestureEvent call.

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/moti...
File ui/events/gestures/motion_event_aura.cc (right):

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura.cc:150: last_touch_time_ -
base::TimeTicks(),
On 2014/05/05 16:57:09, jdduke wrote:
> We should probably just take a TimeTicks in the constructor?

Done.

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura.cc:150: last_touch_time_ -
base::TimeTicks(),
On 2014/05/05 16:57:09, jdduke wrote:
> Looks like we need a reformat here for indentation?

Done.

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/moti...
File ui/events/gestures/motion_event_aura.h (right):

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura.h:28: // MotionEventAura implementation.
On 2014/05/05 16:57:09, jdduke wrote:
> Hmm, think this get the rename treatment, "MotionEvent implementation."

Done.

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura.h:64: PointData() {
On 2014/05/05 16:57:09, jdduke wrote:
> Nit: Constructor implementation in the .cc file, and might as well use
> initializer list.

Done.

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura.h:91: size_t GetIndexFromId(int id);
On 2014/05/05 16:57:09, jdduke wrote:
> Nit: const method.

Done.

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/moti...
File ui/events/gestures/motion_event_aura_unittest.cc (right):

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura_unittest.cc:20: float r,
On 2014/05/05 16:57:09, jdduke wrote:
> For my sanity could you use full names for r(adius) and p(ressure)? =/

Done, though I haven't changed |PointerLocations|. Let me know if you want me
to.

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura_unittest.cc:104: // been removed.
On 2014/05/05 16:57:09, jdduke wrote:
> Hmm, I don't see any |GetActionIndex()| calls in this test?

Done.

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura_unittest.cc:286: scoped_ptr<MotionEvent>
clone = event.Cancel();
On 2014/05/05 16:57:09, jdduke wrote:
> Nit: Maybe rename clone to cancel?

Done.

[jdduke (slow), 2014-05-05 17:52:48]

ui/events/gesture_detection lgtm, as well as the new files previously in
gesture_detection (with a couple nits).

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/gest...
File ui/events/gestures/gesture_configuration.h (right):

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/gest...
ui/events/gestures/gesture_configuration.h:17: class EVENTS_BASE_EXPORT
GestureConfiguration {
On 2014/05/05 17:45:14, tdresser wrote:
> On 2014/05/05 16:57:09, jdduke wrote:
> > Hmm, do we want this in events_base? Android doesn't use this, does iOS or
> Mac?
> > When we do move it to events_base, I think I'd prefer that all platforms use
> it
> > for the relevant constants.
> 
> Let's leave cleaning that up until we sort out the gesture_config_helper*
> locations.
> 
> I think it's reasonable to leave that until after this has landed?

sgtm, maybe file a bug if there's not one already?

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

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/gest...
ui/events/gestures/gesture_recognizer_impl.cc:362: scoped_ptr<GestureEvent>
gesture_event = event.Pass();
On 2014/05/05 17:45:14, tdresser wrote:
> On 2014/05/05 16:57:09, jdduke wrote:
> > Nit: No need to Pass() here, we can just use |event.get()|.  Maybe put a
note
> > that |DispatchGestureEvent()| does the cleanup? (unless that's the way Aura
> > always works with events, where passing the pointer hands off ownership).
> 
> |DispatchGestureEvent()| doesn't do the cleanup. In this version of the CL,
the
> cleanup is happening right here, after the DispatchGestureEvent call.

OK, but why do we need to pass to a local scoped_ptr? |event| already is local?

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

https://codereview.chromium.org/251543003/diff/210001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_config_helper_aura.cc:39:
static_cast<float>(M_PI);
Oops, good catch... Do you think it's worth having the angle be in radians? Or
would you prefer it remain degrees?

https://codereview.chromium.org/251543003/diff/210001/ui/events/gestures/moti...
File ui/events/gestures/motion_event_aura.h (right):

https://codereview.chromium.org/251543003/diff/210001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura.h:84: const size_t GetIndexFromId(int id);
Sorry, I should have been more clear, this should be:

size_t GetIndexFromId(int id) const;

[tdresser, 2014-05-05 18:11:18]

Sadrul, can you take a look at 

ui/aura/aura.gyp
ui/aura/gestures/gesture_recognizer_unittest.cc
ui/aura/window_event_dispatcher.h
ui/aura/window_event_dispatcher.cc
ui/events/event_switches.h
ui/events/event_switches.cc
ui/events/gestures/gesture_configuration.h	
ui/events/gestures/gesture_configuration.cc
ui/events/gestures/gesture_recognizer_impl.h	
ui/events/gestures/gesture_recognizer_impl.cc	
ui/events/gestures/gesture_types.h

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/gest...
File ui/events/gestures/gesture_configuration.h (right):

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/gest...
ui/events/gestures/gesture_configuration.h:17: class EVENTS_BASE_EXPORT
GestureConfiguration {
On 2014/05/05 17:52:48, jdduke wrote:
> On 2014/05/05 17:45:14, tdresser wrote:
> > On 2014/05/05 16:57:09, jdduke wrote:
> > > Hmm, do we want this in events_base? Android doesn't use this, does iOS or
> > Mac?
> > > When we do move it to events_base, I think I'd prefer that all platforms
use
> > it
> > > for the relevant constants.
> > 
> > Let's leave cleaning that up until we sort out the gesture_config_helper*
> > locations.
> > 
> > I think it's reasonable to leave that until after this has landed?
> 
> sgtm, maybe file a bug if there's not one already?

Bug is https://code.google.com/p/chromium/issues/detail?id=339203.

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

https://codereview.chromium.org/251543003/diff/160001/ui/events/gestures/gest...
ui/events/gestures/gesture_recognizer_impl.cc:362: scoped_ptr<GestureEvent>
gesture_event = event.Pass();
On 2014/05/05 17:52:48, jdduke wrote:
> On 2014/05/05 17:45:14, tdresser wrote:
> > On 2014/05/05 16:57:09, jdduke wrote:
> > > Nit: No need to Pass() here, we can just use |event.get()|.  Maybe put a
> note
> > > that |DispatchGestureEvent()| does the cleanup? (unless that's the way
Aura
> > > always works with events, where passing the pointer hands off ownership).
> > 
> > |DispatchGestureEvent()| doesn't do the cleanup. In this version of the CL,
> the
> > cleanup is happening right here, after the DispatchGestureEvent call.
> 
> OK, but why do we need to pass to a local scoped_ptr? |event| already is
local?

Yup, that's true.
Fixed.

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

https://codereview.chromium.org/251543003/diff/210001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_config_helper_aura.cc:39:
static_cast<float>(M_PI);
On 2014/05/05 17:52:48, jdduke wrote:
> Oops, good catch... Do you think it's worth having the angle be in radians? Or
> would you prefer it remain degrees?

I'd like to end up with ui::GestureConfiguration storing an angle in degrees, as
it seems most people are better able to visualize degrees than radians, and
that'll end up exposed through about://gestures.

Keeping the intermediary values consistent with that (in degrees) is probably
reasonable.

https://codereview.chromium.org/251543003/diff/210001/ui/events/gestures/moti...
File ui/events/gestures/motion_event_aura.h (right):

https://codereview.chromium.org/251543003/diff/210001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura.h:84: const size_t GetIndexFromId(int id);
On 2014/05/05 17:52:48, jdduke wrote:
> Sorry, I should have been more clear, this should be:
> 
> size_t GetIndexFromId(int id) const;

Pffft, that should not have required additional clarity, that was me being
sloppy.
Done.

[jdduke (slow), 2014-05-05 18:40:28]

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

https://codereview.chromium.org/251543003/diff/250001/ui/events/gestures/gest...
ui/events/gestures/gesture_provider_aura.cc:65: 1 << gesture.motion_event_id));
One last question. If the reference to ui::GestureEvent doesn't have to persist
(i.e., it's "consumed" directly), do we need to heap allocate? Could we just
stack allocate and pass by const ref to the client?

[tdresser, 2014-05-05 18:56:06]

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

https://codereview.chromium.org/251543003/diff/250001/ui/events/gestures/gest...
ui/events/gestures/gesture_provider_aura.cc:65: 1 << gesture.motion_event_id));
On 2014/05/05 18:40:29, jdduke wrote:
> One last question. If the reference to ui::GestureEvent doesn't have to
persist
> (i.e., it's "consumed" directly), do we need to heap allocate? Could we just
> stack allocate and pass by const ref to the client? 

It can't be const.

https://codereview.chromium.org/251543003/diff/270001/ui/events/events.gyp
File ui/events/events.gyp (right):

https://codereview.chromium.org/251543003/diff/270001/ui/events/events.gyp#ne...
ui/events/events.gyp:330: 'target_name': 'events_tests',
I could split out events_base_tests and gesture_detection_tests, but I don't see
much point.

Does this seem reasonable?

[tdresser, 2014-05-07 12:34:18]

jochen@, could you take a look at:
content/browser/frame_host/render_widget_host_view_guest.h
content/browser/frame_host/render_widget_host_view_guest.cc
content/content_tests.gypi

It just renames single method rename, and adds a dependency to
test_support_content.

[jochen (gone - plz use gerrit), 2014-05-07 12:44:39]

lgtm

[sadrul, 2014-05-07 13:42:17]

LGTM

https://codereview.chromium.org/251543003/diff/320001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.h (right):

https://codereview.chromium.org/251543003/diff/320001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.h:77: // events are dispatched from
GestureRecognizer instead of WindowTreeHost.
This comment is wrong. Remove.

https://codereview.chromium.org/251543003/diff/320001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.h:78: virtual void
DispatchGestureEvent(ui::GestureEvent* event) OVERRIDE;
Move this down in line ~187

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

https://codereview.chromium.org/251543003/diff/320001/ui/events/gestures/gest...
ui/events/gestures/gesture_provider_aura.cc:54: scoped_ptr<ui::GestureEvent>
event(
Why scoped_ptr<>? Can't we just create this on the stack and pass a
pointer/const-ref to the client?

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

https://codereview.chromium.org/251543003/diff/320001/ui/events/gestures/gest...
ui/events/gestures/gesture_recognizer_impl.cc:331:
base::MessageLoop::current()->PostTask(
Use DeleteSoon instead

https://codereview.chromium.org/251543003/diff/320001/ui/events/gestures/moti...
File ui/events/gestures/motion_event_aura.h (right):

https://codereview.chromium.org/251543003/diff/320001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura.h:95: PointData
active_touches_[GestureSequence::kMaxGesturePoints];
DISALLOW_COPY_...?

[tdresser, 2014-05-07 14:04:17]

https://codereview.chromium.org/251543003/diff/320001/ui/aura/window_event_di...
File ui/aura/window_event_dispatcher.h (right):

https://codereview.chromium.org/251543003/diff/320001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.h:77: // events are dispatched from
GestureRecognizer instead of WindowTreeHost.
On 2014/05/07 13:42:17, sadrul wrote:
> This comment is wrong. Remove.

Done.

https://codereview.chromium.org/251543003/diff/320001/ui/aura/window_event_di...
ui/aura/window_event_dispatcher.h:78: virtual void
DispatchGestureEvent(ui::GestureEvent* event) OVERRIDE;
On 2014/05/07 13:42:17, sadrul wrote:
> Move this down in line ~187

Done.

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

https://codereview.chromium.org/251543003/diff/320001/ui/events/gestures/gest...
ui/events/gestures/gesture_provider_aura.cc:54: scoped_ptr<ui::GestureEvent>
event(
On 2014/05/07 13:42:17, sadrul wrote:
> Why scoped_ptr<>? Can't we just create this on the stack and pass a
> pointer/const-ref to the client?

Done.

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

https://codereview.chromium.org/251543003/diff/320001/ui/events/gestures/gest...
ui/events/gestures/gesture_recognizer_impl.cc:331:
base::MessageLoop::current()->PostTask(
On 2014/05/07 13:42:17, sadrul wrote:
> Use DeleteSoon instead

Nice, thanks!
Done.

https://codereview.chromium.org/251543003/diff/320001/ui/events/gestures/moti...
File ui/events/gestures/motion_event_aura.h (right):

https://codereview.chromium.org/251543003/diff/320001/ui/events/gestures/moti...
ui/events/gestures/motion_event_aura.h:95: PointData
active_touches_[GestureSequence::kMaxGesturePoints];
On 2014/05/07 13:42:17, sadrul wrote:
> DISALLOW_COPY_...?

Done.

[tdresser, 2014-05-07 14:09:00]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-05-07 14:10:30]

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

[tdresser, 2014-05-07 15:35:15]

The CQ bit was unchecked by tdresser@chromium.org

[tdresser, 2014-05-07 15:38:37]

Oshima, can you take a look at:
ash/drag_drop/drag_drop_controller_unittest.cc

[oshima, 2014-05-07 17:03:30]

lgtm

[tdresser, 2014-05-07 17:04:32]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-05-07 17:06:47]

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

[commit-bot: I haz the power, 2014-05-07 19:59:01]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_clang_dbg on tryserver.chromium
  android_dbg_triggered_tests on tryserver.chromium

[commit-bot: I haz the power, 2014-05-07 20:18:06]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_clang_dbg on tryserver.chromium
  android_dbg_triggered_tests on tryserver.chromium

[commit-bot: I haz the power, 2014-05-07 20:27:54]

FYI, CQ is re-trying this CL (attempt #2).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_clang_dbg on tryserver.chromium
  android_dbg_triggered_tests on tryserver.chromium

[commit-bot: I haz the power, 2014-05-07 20:38:00]

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

[commit-bot: I haz the power, 2014-05-07 20:38:00]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium
  android_dbg_triggered_tests on tryserver.chromium

[tdresser, 2014-05-07 23:11:01]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-05-07 23:14:22]

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

[commit-bot: I haz the power, 2014-05-07 23:27:35]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_clang_dbg on tryserver.chromium
  android_dbg_triggered_tests on tryserver.chromium

[commit-bot: I haz the power, 2014-05-07 23:34:18]

FYI, CQ is re-trying this CL (attempt #2).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_clang_dbg on tryserver.chromium
  android_dbg_triggered_tests on tryserver.chromium

[commit-bot: I haz the power, 2014-05-07 23:45:38]

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

[commit-bot: I haz the power, 2014-05-07 23:45:39]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium
  android_dbg_triggered_tests on tryserver.chromium

[tdresser, 2014-05-08 12:09:02]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-05-08 12:12:40]

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

[tdresser, 2014-05-08 15:04:02]

The CQ bit was unchecked by tdresser@chromium.org

[tdresser, 2014-05-08 15:04:03]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-05-08 15:09:43]

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

[tdresser, 2014-05-09 11:53:47]

The CQ bit was unchecked by tdresser@chromium.org

[tdresser, 2014-05-09 11:53:47]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-05-09 12:00:26]

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

[commit-bot: I haz the power, 2014-05-09 16:28:21]

Change committed as 269325

[Lei Zhang, 2014-05-10 02:00:06]

A revert of this CL has been created in
https://codereview.chromium.org/278073004/ by thestig@chromium.org.

The reason for reverting is: GestureRecognizerTest is leaking memory..

[Alpha Left Google, 2014-05-10 20:03:25]

A revert of this CL has been created in
https://codereview.chromium.org/278183002/ by hclam@chromium.org.

The reason for reverting is: Memory leak detected:
http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20....