Store the id of a contributing motion event in GestureEventData

Store the id of a contributing motion event in GestureEventData

The unified GR needs to be able to construct ui::GestureEvents from
GestureEventData objects. Currently no information on
contributing touch ids is available in GestureEventData.

This information is used for figuring out which window to target 
a gesture to - the gesture is targeted to the window which the 
contributing touch is locked to.

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

[tdresser, 2014-04-01 14:58:07]

jdduke@, can you take a look?

I was initially playing with an approach which I think is somewhat cleaner (see
patch set 9), but it would cause a bunch of code churn.

I think this is probably a reasonable place to start, and we can do some future
cleanup if it looks worthwhile.

[tdresser, 2014-04-02 17:46:14]

As discussed with jdduke offline, I've switched to storing the motion event id,
instead of the touch id.
jdduke does this look about right?

I've plumbed it up to my WIP unified GR patch, and it works fine for my
purposes.

[jdduke (slow), 2014-04-02 18:33:12]

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

https://codereview.chromium.org/212663010/diff/240001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:296: e1.GetId(),
Hmm... e1 is the ACTION_DOWN, don't we want the id of the event that triggered
the scroll?

https://codereview.chromium.org/212663010/diff/240001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:337: e1.GetId(),
Hmm, I think we want e2.getId() on the fling (the ACTION_UP event).

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

https://codereview.chromium.org/212663010/diff/240001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider_unittest.cc:184:
event.SetId(motion_event_id);
Please increment motion_event_id whenever you |SetId()| so that we're sure we're
seeing the new value, e.g., 

int current_motion_event_id = 5;
...
event.SetId(++current_motion_event_id);
...
event.SetId(++current_motion_event_id);

https://codereview.chromium.org/212663010/diff/240001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider_unittest.cc:221:
event.SetId(motion_event_id);
Same as above, make sure each MotionEvent is getting a different id, and for all
following tests.

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

https://codereview.chromium.org/212663010/diff/240001/ui/events/gesture_detec...
ui/events/gesture_detection/mock_motion_event.h:32: void SetId(int new_id);
Nit: Please move |SetId()| down by the other utility methods as it's not really
a part of the interface.

[jdduke (slow), 2014-04-02 18:35:14]

+cc: mkosiba@, this will be useful for the listener interface for WebView.   I
think all we'll need is some wiring for the ID we supply to MotionEventAndroid
back to wherever WebView has it's event queue, and a way to carry the ID with/in
the WebGestureEvent.

[tdresser, 2014-04-03 14:01:23]

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

https://codereview.chromium.org/212663010/diff/240001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:296: e1.GetId(),
On 2014/04/02 18:33:12, jdduke wrote:
> Hmm... e1 is the ACTION_DOWN, don't we want the id of the event that triggered
> the scroll? 

There were a couple other similar issues - my initial implementation which
focused solely on what I needed for Aura didn't care which event the id came
from.

Done.

https://codereview.chromium.org/212663010/diff/240001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:337: e1.GetId(),
On 2014/04/02 18:33:12, jdduke wrote:
> Hmm, I think we want e2.getId() on the fling (the ACTION_UP event).

Done.

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

https://codereview.chromium.org/212663010/diff/240001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider_unittest.cc:184:
event.SetId(motion_event_id);
On 2014/04/02 18:33:12, jdduke wrote:
> Please increment motion_event_id whenever you |SetId()| so that we're sure
we're
> seeing the new value, e.g., 
> 
> int current_motion_event_id = 5;
> ...
> event.SetId(++current_motion_event_id);
> ...
> event.SetId(++current_motion_event_id);

Done.

https://codereview.chromium.org/212663010/diff/240001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider_unittest.cc:221:
event.SetId(motion_event_id);
On 2014/04/02 18:33:12, jdduke wrote:
> Same as above, make sure each MotionEvent is getting a different id, and for
all
> following tests.

Done.

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

https://codereview.chromium.org/212663010/diff/240001/ui/events/gesture_detec...
ui/events/gesture_detection/mock_motion_event.h:32: void SetId(int new_id);
On 2014/04/02 18:33:12, jdduke wrote:
> Nit: Please move |SetId()| down by the other utility methods as it's not
really
> a part of the interface.

Done.

[mkosiba (inactive), 2014-04-03 14:31:49]

On 2014/04/02 18:35:14, jdduke wrote:
> +cc: mkosiba@, this will be useful for the listener interface for WebView.   I
> think all we'll need is some wiring for the ID we supply to MotionEventAndroid
> back to wherever WebView has it's event queue, and a way to carry the ID
with/in
> the WebGestureEvent.

sounds like a plan :)

[jdduke (slow), 2014-04-03 15:47:53]

lgtm with one cleanup suggestion.

https://codereview.chromium.org/212663010/diff/260001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_provider.h (right):

https://codereview.chromium.org/212663010/diff/260001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.h:87: int motion_event_id,
Hmm, can we have Fling take a const MotionEvent& arg? Then we can have
EndTouchScrollIfNecessary also take a const MotionEvent& arg.

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

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-04-03 18:08:31]

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

[tdresser, 2014-04-03 18:09:46]

https://codereview.chromium.org/212663010/diff/260001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_provider.h (right):

https://codereview.chromium.org/212663010/diff/260001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.h:87: int motion_event_id,
On 2014/04/03 15:47:54, jdduke wrote:
> Hmm, can we have Fling take a const MotionEvent& arg? Then we can have
> EndTouchScrollIfNecessary also take a const MotionEvent& arg.

Done.

[commit-bot: I haz the power, 2014-04-03 23:35:48]

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

[commit-bot: I haz the power, 2014-04-03 23:35:49]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[tdresser, 2014-04-04 12:18:09]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-04-04 12:18:26]

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

[commit-bot: I haz the power, 2014-04-04 12:18:33]

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

[commit-bot: I haz the power, 2014-04-04 12:18:34]

Failed to apply patch for ui/events/gesture_detection/gesture_provider.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ui/events/gesture_detection/gesture_provider.cc
  Hunk #1 FAILED at 31.
  Hunk #2 FAILED at 105.
  Hunk #3 succeeded at 147 with fuzz 2 (offset 14 lines).
  Hunk #4 succeeded at 294 (offset 18 lines).
  Hunk #5 succeeded at 334 (offset 18 lines).
  Hunk #6 FAILED at 404.
  Hunk #7 FAILED at 422.
  Hunk #8 FAILED at 638.
  Hunk #9 succeeded at 630 (offset -73 lines).
  Hunk #10 succeeded at 645 (offset -73 lines).
  Hunk #11 succeeded at 672 (offset -73 lines).
  Hunk #12 succeeded at 700 (offset -73 lines).
  Hunk #13 succeeded at 728 (offset -70 lines).
  Hunk #14 succeeded at 754 with fuzz 1 (offset -70 lines).
  5 out of 14 hunks FAILED -- saving rejects to file
ui/events/gesture_detection/gesture_provider.cc.rej

Patch:       ui/events/gesture_detection/gesture_provider.cc
Index: ui/events/gesture_detection/gesture_provider.cc
diff --git a/ui/events/gesture_detection/gesture_provider.cc
b/ui/events/gesture_detection/gesture_provider.cc
index
4deeba2580f3a7a72928cd3cec30f4a401aa2f18..afa3009b7a2634c507299e78963dc7fedad14b39
100644
--- a/ui/events/gesture_detection/gesture_provider.cc
+++ b/ui/events/gesture_detection/gesture_provider.cc
@@ -31,30 +31,40 @@ const char* GetMotionEventActionName(MotionEvent::Action
action) {
 }
 
 GestureEventData CreateGesture(EventType type,
+                               int motion_event_id,
                                base::TimeTicks time,
                                float x,
                                float y,
                                const GestureEventDetails& details) {
-  return GestureEventData(type, time, x, y, details);
+  return GestureEventData(type, motion_event_id, time, x, y, details);
 }
 
 GestureEventData CreateGesture(EventType type,
+                               int motion_event_id,
                                base::TimeTicks time,
                                float x,
                                float y) {
-  return GestureEventData(type, time, x, y);
-  }
+  return GestureEventData(type, motion_event_id, time, x, y);
+}
 
 GestureEventData CreateGesture(EventType type,
                                const MotionEvent& event,
                                const GestureEventDetails& details) {
-  return CreateGesture(
-      type, event.GetEventTime(), event.GetX(), event.GetY(), details);
+  return GestureEventData(type,
+                          event.GetId(),
+                          event.GetEventTime(),
+                          event.GetX(),
+                          event.GetY(),
+                          details);
 }
 
 GestureEventData CreateGesture(EventType type,
                                const MotionEvent& event) {
-  return CreateGesture(type, event.GetEventTime(), event.GetX(), event.GetY());
+  return CreateGesture(type,
+                       event.GetId(),
+                       event.GetEventTime(),
+                       event.GetX(),
+                       event.GetY());
 }
 
 float Round(float f) {
@@ -105,27 +115,34 @@ class GestureProvider::ScaleGestureListenerImpl
   }
 
   // ScaleGestureDetector::ScaleGestureListener implementation.
-  virtual bool OnScaleBegin(const ScaleGestureDetector& detector) OVERRIDE {
+  virtual bool OnScaleBegin(const ScaleGestureDetector& detector,
+                            const MotionEvent& e) OVERRIDE {
     if (ignore_detector_events_)
       return false;
     pinch_event_sent_ = false;
     return true;
   }
 
-  virtual void OnScaleEnd(const ScaleGestureDetector& detector) OVERRIDE {
+  virtual void OnScaleEnd(const ScaleGestureDetector& detector,
+                          const MotionEvent& e) OVERRIDE {
     if (!pinch_event_sent_)
       return;
-    provider_->Send(
-        CreateGesture(ET_GESTURE_PINCH_END, detector.GetEventTime(), 0, 0));
+    provider_->Send(CreateGesture(ET_GESTURE_PINCH_END,
+                                  e.GetId(),
+                                  detector.GetEventTime(),
+                                  0,
+                                  0));
     pinch_event_sent_ = false;
   }
 
-  virtual bool OnScale(const ScaleGestureDetector& detector) OVERRIDE {
+  virtual bool OnScale(const ScaleGestureDetector& detector,
+                       const MotionEvent& e) OVERRIDE {
     if (ignore_detector_events_)
       return false;
     if (!pinch_event_sent_) {
       pinch_event_sent_ = true;
       provider_->Send(CreateGesture(ET_GESTURE_PINCH_BEGIN,
+                                    e.GetId(),
                                     detector.GetEventTime(),
                                     detector.GetFocusX(),
                                     detector.GetFocusY()));
@@ -133,6 +150,7 @@ class GestureProvider::ScaleGestureListenerImpl
     GestureEventDetails pinch_details(
         ET_GESTURE_PINCH_UPDATE, detector.GetScaleFactor(), 0);
     provider_->Send(CreateGesture(ET_GESTURE_PINCH_UPDATE,
+                                  e.GetId(),
                                   detector.GetEventTime(),
                                   detector.GetFocusX(),
                                   detector.GetFocusY(),
@@ -275,6 +293,7 @@ class GestureProvider::GestureListenerImpl
       GestureEventDetails scroll_details(
           ET_GESTURE_SCROLL_BEGIN, -raw_distance_x, -raw_distance_y);
       provider_->Send(CreateGesture(ET_GESTURE_SCROLL_BEGIN,
+                                    e2.GetId(),
                                     e2.GetEventTime(),
                                     e1.GetX(),
                                     e1.GetY(),
@@ -314,8 +333,7 @@ class GestureProvider::GestureListenerImpl
       }
     }
 
-    provider_->Fling(
-        e2.GetEventTime(), e1.GetX(), e1.GetY(), velocity_x, velocity_y);
+    provider_->Fling(e2, velocity_x, velocity_y);
     return true;
   }
 
@@ -403,10 +421,11 @@ class GestureProvider::GestureListenerImpl
           if (IsDistanceGreaterThanTouchSlop(distance_x, distance_y)) {
             GestureEventDetails scroll_details(
                 ET_GESTURE_SCROLL_BEGIN, -distance_x, -distance_y);
-            provider_->Send(
-                CreateGesture(ET_GESTURE_SCROLL_BEGIN, e, scroll_details));
+            provider_->Send(CreateGesture(
+                ET_GESTURE_SCROLL_BEGIN, e, scroll_details));
             provider_->Send(
                 CreateGesture(ET_GESTURE_PINCH_BEGIN,
+                              e.GetId(),
                               e.GetEventTime(),
                               Round(double_tap_drag_zoom_anchor_x_),
                               Round(double_tap_drag_zoom_anchor_y_)));
@@ -421,6 +440,7 @@ class GestureProvider::GestureListenerImpl
                                  std::abs(dy * px_to_dp_));
           GestureEventDetails pinch_details(ET_GESTURE_PINCH_UPDATE, scale, 0);
           provider_->Send(CreateGesture(ET_GESTURE_PINCH_UPDATE,
+                                        e.GetId(),
                                         e.GetEventTime(),
                                         Round(double_tap_drag_zoom_anchor_x_),
                                         Round(double_tap_drag_zoom_anchor_y_),
@@ -637,7 +657,7 @@ bool GestureProvider::OnTouchEvent(const MotionEvent& event)
{
     // "Last finger raised" could be an end to movement, but it should
     // only terminate scrolling if the event did not cause a fling.
     if (was_touch_scrolling_ && !handled)
-      EndTouchScrollIfNecessary(event.GetEventTime(), true);
+      EndTouchScrollIfNecessary(event, true);
 
     // We shouldn't necessarily cancel a tap on ACTION_UP, as the double-tap
     // timeout may yet trigger a SINGLE_TAP.
@@ -702,13 +722,11 @@ bool GestureProvider::CanHandle(const MotionEvent& event)
const {
   return event.GetAction() == MotionEvent::ACTION_DOWN || current_down_event_;
 }
 
-void GestureProvider::Fling(base::TimeTicks time,
-                            float x,
-                            float y,
+void GestureProvider::Fling(const MotionEvent& event,
                             float velocity_x,
                             float velocity_y) {
   if (!velocity_x && !velocity_y) {
-    EndTouchScrollIfNecessary(time, true);
+    EndTouchScrollIfNecessary(event, true);
     return;
   }
 
@@ -719,13 +737,14 @@ void GestureProvider::Fling(base::TimeTicks time,
     // start hint.
     GestureEventDetails scroll_details(
         ET_GESTURE_SCROLL_BEGIN, velocity_x, velocity_y);
-    Send(CreateGesture(ET_GESTURE_SCROLL_BEGIN, time, x, y, scroll_details));
+    Send(CreateGesture(ET_GESTURE_SCROLL_BEGIN, event, scroll_details));
   }
-  EndTouchScrollIfNecessary(time, false);
+  EndTouchScrollIfNecessary(event, false);
 
   GestureEventDetails fling_details(
       ET_SCROLL_FLING_START, velocity_x, velocity_y);
-  Send(CreateGesture(ET_SCROLL_FLING_START, time, x, y, fling_details));
+  Send(CreateGesture(
+      ET_SCROLL_FLING_START, event, fling_details));
 }
 
 void GestureProvider::Send(const GestureEventData& gesture) {
@@ -745,8 +764,11 @@ void GestureProvider::Send(const GestureEventData& gesture)
{
       break;
     case ET_GESTURE_TAP:
       if (needs_show_press_event_)
-        Send(CreateGesture(
-            ET_GESTURE_SHOW_PRESS, gesture.time, gesture.x, gesture.y));
+        Send(CreateGesture(ET_GESTURE_SHOW_PRESS,
+                           gesture.motion_event_id,
+                           gesture.time,
+                           gesture.x,
+                           gesture.y));
       needs_tap_ending_event_ = false;
       break;
     case ET_GESTURE_DOUBLE_TAP:
@@ -770,7 +792,7 @@ void GestureProvider::Send(const GestureEventData& gesture)
{
       break;
     case ET_GESTURE_SCROLL_BEGIN:
       touch_scroll_in_progress_ = true;
-      SendTapCancelIfNecessary(*current_down_event_);
+      SendTapCancelIf…
(message too large)

[tdresser, 2014-04-04 13:51:53]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-04-04 13:52:03]

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

[commit-bot: I haz the power, 2014-04-04 15:29:28]

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

[commit-bot: I haz the power, 2014-04-04 18:51:55]

Change committed as 261808