Gestures are now possible using touch events with any ids

Gestures are now possible using touch events with any ids

Each gesture point stores a "point_id," which is used for state transitions in the GestureRecognizer.
The point_ids are maintained such that the set of point_ids is always contiguous, from 0 to the number of current touches.
A lower point_id indicates that a touch occurred first.

BUG=113144
TEST=GestureRecognizerTest.*


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=123989

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124071

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124668

[rjkroege, 2012-02-23 19:39:45]

https://chromiumcodereview.appspot.com/9452024/diff/1/ui/aura/gestures/gestur...
File ui/aura/gestures/gesture_point.h (right):

https://chromiumcodereview.appspot.com/9452024/diff/1/ui/aura/gestures/gestur...
ui/aura/gestures/gesture_point.h:56: void set_point_id(int point_id) { point_id_
= point_id; }
can you add some comments saying what these are for

https://chromiumcodereview.appspot.com/9452024/diff/1/ui/aura/gestures/gestur...
ui/aura/gestures/gesture_point.h:93: bool in_use_;
do we need both the bool and the point_id? positive point_id == in_use, negative
point_id == not in use?

https://chromiumcodereview.appspot.com/9452024/diff/1/ui/aura/gestures/gestur...
File ui/aura/gestures/gesture_sequence.cc (right):

https://chromiumcodereview.appspot.com/9452024/diff/1/ui/aura/gestures/gestur...
ui/aura/gestures/gesture_sequence.cc:272: if (event.type() ==
ui::ET_TOUCH_RELEASED) {
I am unclear why you are doing this. Can you explain?

https://chromiumcodereview.appspot.com/9452024/diff/1/ui/aura/gestures/gestur...
ui/aura/gestures/gesture_sequence.cc:307: GesturePoint*
GestureSequence::GetPointByPointId(int point_id) {
can't we preserve the radix lookup. you are maintaining a table that maps real
touch ids to virtual touch ids? and another table that maps virtual touch ids to
real touch ids?

https://chromiumcodereview.appspot.com/9452024/diff/1/ui/aura/gestures/gestur...
File ui/aura/gestures/gesture_sequence.h (right):

https://chromiumcodereview.appspot.com/9452024/diff/1/ui/aura/gestures/gestur...
ui/aura/gestures/gesture_sequence.h:57: GesturePoint* GetPointByPointId(int
point_id);
add an explanation?

[tdresser, 2012-02-23 20:13:07]

https://chromiumcodereview.appspot.com/9452024/diff/1/ui/aura/gestures/gestur...
File ui/aura/gestures/gesture_point.h (right):

https://chromiumcodereview.appspot.com/9452024/diff/1/ui/aura/gestures/gestur...
ui/aura/gestures/gesture_point.h:56: void set_point_id(int point_id) { point_id_
= point_id; }
On 2012/02/23 19:39:45, rjkroege wrote:
> can you add some comments saying what these are for

Done.

https://chromiumcodereview.appspot.com/9452024/diff/1/ui/aura/gestures/gestur...
ui/aura/gestures/gesture_point.h:93: bool in_use_;
On 2012/02/23 19:39:45, rjkroege wrote:
> do we need both the bool and the point_id? positive point_id == in_use,
negative
> point_id == not in use?

Done.

https://chromiumcodereview.appspot.com/9452024/diff/1/ui/aura/gestures/gestur...
File ui/aura/gestures/gesture_sequence.cc (right):

https://chromiumcodereview.appspot.com/9452024/diff/1/ui/aura/gestures/gestur...
ui/aura/gestures/gesture_sequence.cc:272: if (event.type() ==
ui::ET_TOUCH_RELEASED) {
On 2012/02/23 19:39:45, rjkroege wrote:
> I am unclear why you are doing this. Can you explain?

See comments.

https://chromiumcodereview.appspot.com/9452024/diff/1/ui/aura/gestures/gestur...
ui/aura/gestures/gesture_sequence.cc:307: GesturePoint*
GestureSequence::GetPointByPointId(int point_id) {
On 2012/02/23 19:39:45, rjkroege wrote:
> can't we preserve the radix lookup. you are maintaining a table that maps real
> touch ids to virtual touch ids? and another table that maps virtual touch ids
to
> real touch ids?

I believe that maintaining a second table would end up more expensive in the
long run. Ensuring that point_ids are contiguous would involve a lot of
shuffling around of table entries.

https://chromiumcodereview.appspot.com/9452024/diff/1/ui/aura/gestures/gestur...
File ui/aura/gestures/gesture_sequence.h (right):

https://chromiumcodereview.appspot.com/9452024/diff/1/ui/aura/gestures/gestur...
ui/aura/gestures/gesture_sequence.h:57: GesturePoint* GetPointByPointId(int
point_id);
On 2012/02/23 19:39:45, rjkroege wrote:
> add an explanation?

Done.

[sadrul, 2012-02-23 21:44:14]

Very nice. Some comments:

http://codereview.chromium.org/9452024/diff/6001/ui/aura/gestures/gesture_poi...
File ui/aura/gestures/gesture_point.h (right):

http://codereview.chromium.org/9452024/diff/6001/ui/aura/gestures/gesture_poi...
ui/aura/gestures/gesture_point.h:98: bool in_use_;
Remove

http://codereview.chromium.org/9452024/diff/6001/ui/aura/gestures/gesture_seq...
File ui/aura/gestures/gesture_sequence.cc (right):

http://codereview.chromium.org/9452024/diff/6001/ui/aura/gestures/gesture_seq...
ui/aura/gestures/gesture_sequence.cc:18: //            http://crbug.com/113144
Remove this TODO too

http://codereview.chromium.org/9452024/diff/6001/ui/aura/gestures/gesture_seq...
ui/aura/gestures/gesture_sequence.cc:103: G(GS_SCROLL, 0, TS_PRESSED, false),
I believe this should not be necessary anymore.

http://codereview.chromium.org/9452024/diff/6001/ui/aura/gestures/gesture_seq...
ui/aura/gestures/gesture_sequence.cc:106: G(GS_SCROLL, 1, TS_RELEASED, false),
ditto

http://codereview.chromium.org/9452024/diff/6001/ui/aura/gestures/gesture_seq...
ui/aura/gestures/gesture_sequence.cc:109: G(GS_SCROLL, 1, TS_MOVED, false),
ditto

http://codereview.chromium.org/9452024/diff/6001/ui/aura/gestures/gesture_seq...
ui/aura/gestures/gesture_sequence.cc:112: G(GS_SCROLL, 1, TS_CANCELLED, false),
ditto

http://codereview.chromium.org/9452024/diff/6001/ui/aura/gestures/gesture_seq...
ui/aura/gestures/gesture_sequence.cc:317: DCHECK(false);
NOTREACHED

[tdresser, 2012-02-23 22:16:55]

http://codereview.chromium.org/9452024/diff/6001/ui/aura/gestures/gesture_poi...
File ui/aura/gestures/gesture_point.h (right):

http://codereview.chromium.org/9452024/diff/6001/ui/aura/gestures/gesture_poi...
ui/aura/gestures/gesture_point.h:98: bool in_use_;
On 2012/02/23 21:44:14, sadrul wrote:
> Remove

Done.

http://codereview.chromium.org/9452024/diff/6001/ui/aura/gestures/gesture_seq...
File ui/aura/gestures/gesture_sequence.cc (right):

http://codereview.chromium.org/9452024/diff/6001/ui/aura/gestures/gesture_seq...
ui/aura/gestures/gesture_sequence.cc:18: //            http://crbug.com/113144
On 2012/02/23 21:44:14, sadrul wrote:
> Remove this TODO too

Done.

http://codereview.chromium.org/9452024/diff/6001/ui/aura/gestures/gesture_seq...
ui/aura/gestures/gesture_sequence.cc:103: G(GS_SCROLL, 0, TS_PRESSED, false),
On 2012/02/23 21:44:14, sadrul wrote:
> I believe this should not be necessary anymore.

Done.

http://codereview.chromium.org/9452024/diff/6001/ui/aura/gestures/gesture_seq...
ui/aura/gestures/gesture_sequence.cc:317: DCHECK(false);
On 2012/02/23 21:44:14, sadrul wrote:
> NOTREACHED

Done.

[sadrul, 2012-02-24 17:52:24]

Very nice! LGTM with the following changes:

http://codereview.chromium.org/9452024/diff/4005/ui/aura/gestures/gesture_seq...
File ui/aura/gestures/gesture_sequence.cc (right):

http://codereview.chromium.org/9452024/diff/4005/ui/aura/gestures/gesture_seq...
ui/aura/gestures/gesture_sequence.cc:256: for (int i = 0; i < kMaxGesturePoints;
++i) {
It should be OK to go only upto point_count_ here, right?

http://codereview.chromium.org/9452024/diff/4005/ui/aura/gestures/gesture_seq...
ui/aura/gestures/gesture_sequence.cc:491: DCHECK(point);
point being NULL will cause a crash in the next line anyway. In such cases,
CHECK/DCHECK's are seen as unnecessary.

[tdresser, 2012-02-24 18:19:50]

http://codereview.chromium.org/9452024/diff/4005/ui/aura/gestures/gesture_seq...
File ui/aura/gestures/gesture_sequence.cc (right):

http://codereview.chromium.org/9452024/diff/4005/ui/aura/gestures/gesture_seq...
ui/aura/gestures/gesture_sequence.cc:256: for (int i = 0; i < kMaxGesturePoints;
++i) {
On 2012/02/24 17:52:24, sadrul wrote:
> It should be OK to go only upto point_count_ here, right?

GesturePoints which represent current touches are not always contiguous in
points_, as they are indexed by touch_id. If we knew that they were contiguous,
we could only go up to point_count, but because we don't have that guarantee, we
have to scan the whole list.

http://codereview.chromium.org/9452024/diff/4005/ui/aura/gestures/gesture_seq...
ui/aura/gestures/gesture_sequence.cc:491: DCHECK(point);
On 2012/02/24 17:52:24, sadrul wrote:
> point being NULL will cause a crash in the next line anyway. In such cases,
> CHECK/DCHECK's are seen as unnecessary.

Done.

[sadrul, 2012-02-24 18:21:01]

http://codereview.chromium.org/9452024/diff/4005/ui/aura/gestures/gesture_seq...
File ui/aura/gestures/gesture_sequence.cc (right):

http://codereview.chromium.org/9452024/diff/4005/ui/aura/gestures/gesture_seq...
ui/aura/gestures/gesture_sequence.cc:256: for (int i = 0; i < kMaxGesturePoints;
++i) {
On 2012/02/24 18:19:51, tdresser wrote:
> On 2012/02/24 17:52:24, sadrul wrote:
> > It should be OK to go only upto point_count_ here, right?
> 
> GesturePoints which represent current touches are not always contiguous in
> points_, as they are indexed by touch_id. If we knew that they were
contiguous,
> we could only go up to point_count, but because we don't have that guarantee,
we
> have to scan the whole list.

Of course! My bad, I read it wrong.

[rjkroege, 2012-02-24 19:11:29]

lgtm

http://codereview.chromium.org/9452024/diff/4005/ui/aura/gestures/gesture_seq...
File ui/aura/gestures/gesture_sequence.cc (right):

http://codereview.chromium.org/9452024/diff/4005/ui/aura/gestures/gesture_seq...
ui/aura/gestures/gesture_sequence.cc:250: // The set of point_ids must be
contiguous and include 0.
my understanding is that we at least admit the desired property here: i can
pinch, start a drag with one finger and then scroll with the other finger
without releasing the finger.

[sadrul, 2012-02-25 19:06:30]

http://codereview.chromium.org/9452024/diff/4005/ui/aura/gestures/gesture_seq...
File ui/aura/gestures/gesture_sequence.cc (right):

http://codereview.chromium.org/9452024/diff/4005/ui/aura/gestures/gesture_seq...
ui/aura/gestures/gesture_sequence.cc:250: // The set of point_ids must be
contiguous and include 0.
On 2012/02/24 19:11:29, rjkroege wrote:
> my understanding is that we at least admit the desired property here: i can
> pinch, start a drag with one finger and then scroll with the other finger
> without releasing the finger.

Indeed. This is still possible with this change (Tim added a test for it too in
this CL!)

[Ben Goodger (Google), 2012-02-27 20:25:23]

LGTM for window_unittest.cc

[commit-bot: I haz the power, 2012-02-27 23:28:52]

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

[commit-bot: I haz the power, 2012-02-27 23:28:54]

Can't apply patch for file ui/aura/gestures/gesture_point.h.
While running patch -p1 --forward --force;
patching file ui/aura/gestures/gesture_point.h
Hunk #1 FAILED at 53.
1 out of 2 hunks FAILED -- saving rejects to file
ui/aura/gestures/gesture_point.h.rej

[commit-bot: I haz the power, 2012-02-28 14:43:11]

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

[commit-bot: I haz the power, 2012-02-28 17:09:58]

Change committed as 123989

[tdresser, 2012-02-28 17:58:27]

I needed to do a minor tweak to ash/wm/root_window_event_filter_unittest.cc.

Ben, could you take a look?

[Ben Goodger (Google), 2012-02-28 20:12:49]

lgtm

[commit-bot: I haz the power, 2012-02-28 21:19:43]

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

[commit-bot: I haz the power, 2012-02-28 22:06:08]

Try job failure for 9452024-21001 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-02-28 22:13:34]

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

[commit-bot: I haz the power, 2012-02-29 00:37:37]

Change committed as 124071

[tdresser, 2012-03-02 14:04:02]

Sadrul, could you take another look at this?

http://codereview.chromium.org/9452024/diff/34001/ui/aura/gestures/gesture_se...
File ui/aura/gestures/gesture_sequence.cc (right):

http://codereview.chromium.org/9452024/diff/34001/ui/aura/gestures/gesture_se...
ui/aura/gestures/gesture_sequence.cc:171:
DCHECK(!points_[event.touch_id()].in_use());
Is this correct?

If a touch release is preventDefaulted, will the GR ever be told that the touch
has ended?

[sadrul, 2012-03-02 14:20:31]

http://codereview.chromium.org/9452024/diff/34001/ui/aura/gestures/gesture_se...
File ui/aura/gestures/gesture_sequence.cc (right):

http://codereview.chromium.org/9452024/diff/34001/ui/aura/gestures/gesture_se...
ui/aura/gestures/gesture_sequence.cc:171:
DCHECK(!points_[event.touch_id()].in_use());
On 2012/03/02 14:04:02, tdresser wrote:
> Is this correct?
> 
> If a touch release is preventDefaulted, will the GR ever be told that the
touch
> has ended? 

In such a situation, the GR should receive a touch-cancel event for that
touch-id. But the code does not do this at the moment. It would be a good idea
to file a bug for this, comment out this DCHECK, and add a comment here
explaining why DCHECK is commented out.

[tdresser, 2012-03-02 15:11:27]

http://codereview.chromium.org/9452024/diff/34001/ui/aura/gestures/gesture_se...
File ui/aura/gestures/gesture_sequence.cc (right):

http://codereview.chromium.org/9452024/diff/34001/ui/aura/gestures/gesture_se...
ui/aura/gestures/gesture_sequence.cc:171:
DCHECK(!points_[event.touch_id()].in_use());
On 2012/03/02 14:20:31, sadrul wrote:
> On 2012/03/02 14:04:02, tdresser wrote:
> > Is this correct?
> > 
> > If a touch release is preventDefaulted, will the GR ever be told that the
> touch
> > has ended? 
> 
> In such a situation, the GR should receive a touch-cancel event for that
> touch-id. But the code does not do this at the moment. It would be a good idea
> to file a bug for this, comment out this DCHECK, and add a comment here
> explaining why DCHECK is commented out.

Done.

[sadrul, 2012-03-02 15:12:18]

lgtm

[commit-bot: I haz the power, 2012-03-02 15:13:35]

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

[commit-bot: I haz the power, 2012-03-02 16:52:00]

Change committed as 124668

[dhollowa, 2012-03-02 22:23:13]

http://codereview.chromium.org/9452024/diff/45001/ui/aura/gestures/gesture_po...
File ui/aura/gestures/gesture_point.h (right):

http://codereview.chromium.org/9452024/diff/45001/ui/aura/gestures/gesture_po...
ui/aura/gestures/gesture_point.h:63: const int point_id() const { return
point_id_; }
This use of const failed to compile on clang.  For extra coverage you can use
the 'mac_aura' try bot.

[tdresser, 2012-03-05 14:06:33]

On 2012/03/02 22:23:13, dhollowa wrote:
>
http://codereview.chromium.org/9452024/diff/45001/ui/aura/gestures/gesture_po...
> File ui/aura/gestures/gesture_point.h (right):
> 
>
http://codereview.chromium.org/9452024/diff/45001/ui/aura/gestures/gesture_po...
> ui/aura/gestures/gesture_point.h:63: const int point_id() const { return
> point_id_; }
> This use of const failed to compile on clang.  For extra coverage you can use
> the 'mac_aura' try bot.

dhollowa: thanks for landing the fix.