ui::GestureProvider gives GestureEventData the number of touch points.

ui::GestureProvider gives GestureEventData the number of touch points.

This is required for Aura to use ui::GestureProvider.

BUG=360790
TEST=GestureProviderTest.{GestureBeginAndEnd, GestureTapTap, ...}

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

[tdresser, 2014-04-08 20:11:33]

PTAL, thanks.

[jdduke (slow), 2014-04-08 21:07:41]

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

https://codereview.chromium.org/227743007/diff/100001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:310: e2.GetEventTime(),
This is a change in behavior.  GestureScrollBegin attaches to a layer based on
the provided coordinate, and I think we want that coordinate to remain the point
of the first touch.

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

https://codereview.chromium.org/227743007/diff/100001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:39: const GestureEventDetails&
details) {
Might as well pass this by value, then you can call |details.set_touch_points|
directly.

https://codereview.chromium.org/227743007/diff/100001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:55: return
GestureEventData(type, motion_event_id, time, x, y, details);
Hmm, now that we only use one constructor for GestureEventData, I wonder if we
should just change its constructor to take a touch_point_count, and have it set
|set_touch_points()| inside the constructor?

Otherwise, please delete the constructor that doesn't take details.

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

https://codereview.chromium.org/227743007/diff/100001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider_unittest.cc:1050:
event.SetId(++motion_event_id);
Hmm, it's a little odd you stop checking |touch_points()| in this test, when
pinch/multitouch is arguably the most important place to be checking (as the
default GestureEventDetails constructor always initializes the touch count to
1).

[tdresser, 2014-04-09 14:50:48]

Let me know if the CreateGesture change isn't what you were thinking - I wasn't
entirely clear on the approach you were suggesting.

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

https://codereview.chromium.org/227743007/diff/100001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:310: e2.GetEventTime(),
On 2014/04/08 21:07:41, jdduke wrote:
> This is a change in behavior.  GestureScrollBegin attaches to a layer based on
> the provided coordinate, and I think we want that coordinate to remain the
point
> of the first touch.

Done.

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

https://codereview.chromium.org/227743007/diff/100001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:39: const GestureEventDetails&
details) {
On 2014/04/08 21:07:41, jdduke wrote:
> Might as well pass this by value, then you can call |details.set_touch_points|
> directly.

I'm still passing by reference, based on the change I made to address your
comment below.

https://codereview.chromium.org/227743007/diff/100001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:55: return
GestureEventData(type, motion_event_id, time, x, y, details);
On 2014/04/08 21:07:41, jdduke wrote:
> Hmm, now that we only use one constructor for GestureEventData, I wonder if we
> should just change its constructor to take a touch_point_count, and have it
set
> |set_touch_points()| inside the constructor?
> 
> Otherwise, please delete the constructor that doesn't take details.

Is this what you were thinking?

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

https://codereview.chromium.org/227743007/diff/100001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider_unittest.cc:1050:
event.SetId(++motion_event_id);
On 2014/04/08 21:07:41, jdduke wrote:
> Hmm, it's a little odd you stop checking |touch_points()| in this test, when
> pinch/multitouch is arguably the most important place to be checking (as the
> default GestureEventDetails constructor always initializes the touch count to
> 1).

Done.

[jdduke (slow), 2014-04-09 16:53:05]

lgtm with a couple nits.

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

https://codereview.chromium.org/227743007/diff/120001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:297: DCHECK_NE(0u,
e1.GetPointerCount());
I think we have sufficient DCHECK coverage that we don't need this.

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

https://codereview.chromium.org/227743007/diff/120001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:276:
CreateGesture(ET_GESTURE_TAP_CANCEL, ending_event_motion_event_id_, 1));
I don't think we'll ever generate a touch count > 1, so let's have the utility
method supply that.

[tdresser, 2014-04-09 18:21:31]

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

https://codereview.chromium.org/227743007/diff/120001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:297: DCHECK_NE(0u,
e1.GetPointerCount());
On 2014/04/09 16:53:05, jdduke wrote:
> I think we have sufficient DCHECK coverage that we don't need this.

Done.

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

https://codereview.chromium.org/227743007/diff/120001/ui/events/gesture_detec...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:276:
CreateGesture(ET_GESTURE_TAP_CANCEL, ending_event_motion_event_id_, 1));
On 2014/04/09 16:53:05, jdduke wrote:
> I don't think we'll ever generate a touch count > 1, so let's have the utility
> method supply that.

Done.

[tdresser, 2014-04-09 18:21:34]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-04-09 18:21:44]

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

[commit-bot: I haz the power, 2014-04-09 21:00:08]

Change committed as 262807