Pass location with gestures created in TouchDispositionGestureFilter

Pass location with gestures created in TouchDispositionGestureFilter

Aura uses the locations of some ending events which weren't
being assigned locations in the TDGF. To use the unified GR
on Aura, we need these locations to be assigned correctly.

We use the location of the first pointer down when the gesture was
created a the location of the gestures created by the TDGF.

BUG=371875
TEST=TouchDispositionGestureFilterTest.*

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

[tdresser, 2014-05-12 16:08:27]

jdduke@, PTAL.

[tdresser, 2014-05-12 16:43:17]

jdduke@, PTAL.

[jdduke (slow), 2014-05-12 17:02:34]

https://codereview.chromium.org/277373002/diff/1/ui/events/gesture_detection/...
File ui/events/gesture_detection/gesture_event_data_packet.cc (right):

https://codereview.chromium.org/277373002/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/gesture_event_data_packet.cc:84:
GestureEventDataPacket packet(gesture.time, TOUCH_TIMEOUT, gfx::PointF());
gfx::PointF(gesture.x, gesture.y)?

https://codereview.chromium.org/277373002/diff/1/ui/events/gesture_detection/...
File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (right):

https://codereview.chromium.org/277373002/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:210:
ending_event_location_ = packet.touch_location();
It appears we only call |{Cancel,End}FooIfNecessary()| within the context of a
|FilterAndSendPacket()|.  

I wonder if it would be simpler if we made this explicit, in which case we'd
have a GestureEventDataPacket* |packet_being_sent_| or |current_packet_| member
variable that we |base::AutoReset| here.  Then we can just pull the timestamp
and location directly in the cancel/end functions, asserting that
|packet_being_sent_| is non-null and ripping out the timestamp that we pass
around everywhere. What do you think?

If we do that, it's probably worth adding a DCHECK(!packet_being_sent_) in the
destructor.

[tdresser, 2014-05-12 18:09:30]

https://codereview.chromium.org/277373002/diff/1/ui/events/gesture_detection/...
File ui/events/gesture_detection/gesture_event_data_packet.cc (right):

https://codereview.chromium.org/277373002/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/gesture_event_data_packet.cc:84:
GestureEventDataPacket packet(gesture.time, TOUCH_TIMEOUT, gfx::PointF());
On 2014/05/12 17:02:34, jdduke wrote:
> gfx::PointF(gesture.x, gesture.y)?

Done.

https://codereview.chromium.org/277373002/diff/1/ui/events/gesture_detection/...
File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (right):

https://codereview.chromium.org/277373002/diff/1/ui/events/gesture_detection/...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:210:
ending_event_location_ = packet.touch_location();
On 2014/05/12 17:02:34, jdduke wrote:
> It appears we only call |{Cancel,End}FooIfNecessary()| within the context of a
> |FilterAndSendPacket()|.  
> 
> I wonder if it would be simpler if we made this explicit, in which case we'd
> have a GestureEventDataPacket* |packet_being_sent_| or |current_packet_|
member
> variable that we |base::AutoReset| here.  Then we can just pull the timestamp
> and location directly in the cancel/end functions, asserting that
> |packet_being_sent_| is non-null and ripping out the timestamp that we pass
> around everywhere. What do you think?
> 
> If we do that, it's probably worth adding a DCHECK(!packet_being_sent_) in the
> destructor.

SGTM.
Done.

[jdduke (slow), 2014-05-12 18:14:44]

lgtm with a couple nits.

https://codereview.chromium.org/277373002/diff/20001/ui/events/gesture_detect...
File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (right):

https://codereview.chromium.org/277373002/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:21: float x,
Nit: Let's just pass in a |const gfx::Vector2dF&|.

https://codereview.chromium.org/277373002/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:309:
SendGesture(CreateGesture(ET_GESTURE_TAP_CANCEL,
DCHECK(packet_being_sent_);

also in functions below.

https://codereview.chromium.org/277373002/diff/20001/ui/events/gesture_detect...
File ui/events/gesture_detection/touch_disposition_gesture_filter.h (right):

https://codereview.chromium.org/277373002/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/touch_disposition_gesture_filter.h:96: const
GestureEventDataPacket* packet_being_sent_;
Nit: Maybe a brief comment that this is valid only in the scope of
|FilterAndSendPacket()|.

[jdduke (slow), 2014-05-12 18:16:31]

https://codereview.chromium.org/277373002/diff/20001/ui/events/gesture_detect...
File ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc
(right):

https://codereview.chromium.org/277373002/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc:35:
most_recent_gesture_location_ = gfx::PointF(event.x, event.y);
Super nit: Ideally we'd have naming consistency, either |last_sent_foo| or
|most_recent_foo|.  I don't care which, as long as they're consistent.

[tdresser, 2014-05-12 18:28:10]

https://codereview.chromium.org/277373002/diff/20001/ui/events/gesture_detect...
File ui/events/gesture_detection/touch_disposition_gesture_filter.cc (right):

https://codereview.chromium.org/277373002/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:21: float x,
On 2014/05/12 18:14:45, jdduke wrote:
> Nit: Let's just pass in a |const gfx::Vector2dF&|.

Done.

https://codereview.chromium.org/277373002/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/touch_disposition_gesture_filter.cc:309:
SendGesture(CreateGesture(ET_GESTURE_TAP_CANCEL,
On 2014/05/12 18:14:45, jdduke wrote:
> DCHECK(packet_being_sent_);
> 
> also in functions below.

Done.

https://codereview.chromium.org/277373002/diff/20001/ui/events/gesture_detect...
File ui/events/gesture_detection/touch_disposition_gesture_filter.h (right):

https://codereview.chromium.org/277373002/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/touch_disposition_gesture_filter.h:96: const
GestureEventDataPacket* packet_being_sent_;
On 2014/05/12 18:14:45, jdduke wrote:
> Nit: Maybe a brief comment that this is valid only in the scope of
> |FilterAndSendPacket()|.

Done.

https://codereview.chromium.org/277373002/diff/20001/ui/events/gesture_detect...
File ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc
(right):

https://codereview.chromium.org/277373002/diff/20001/ui/events/gesture_detect...
ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc:35:
most_recent_gesture_location_ = gfx::PointF(event.x, event.y);
On 2014/05/12 18:16:31, jdduke wrote:
> Super nit: Ideally we'd have naming consistency, either |last_sent_foo| or
> |most_recent_foo|.  I don't care which, as long as they're consistent.

Done.

[tdresser, 2014-05-12 18:28:38]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-05-12 18:28:54]

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

[commit-bot: I haz the power, 2014-05-12 22:19:47]

Change committed as 269893