Target touches to the correct display.

Target touches to the correct display.

ui::TouchEvents now know the id of their source device.

The target of a touch can't be effected by a touch on another display.

BUG=315651
TEST=GestureRecognizerTest.GestureEventTouchLockIgnoresOtherScreens

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

[sadrul, 2014-01-07 21:13:40]

You set the wrong bug# in the description.

Where would you call set_source_device_id()?

Instead of tacking  the device-id to the event, why not make sure the locations
of the touch-events are correct for multi-monitor setting?

[tdresser, 2014-01-07 21:57:50]

On 2014/01/07 21:13:40, sadrul wrote:
> You set the wrong bug# in the description.
Whoops, fixed.
> 
> Where would you call set_source_device_id()?
It's called on the gesture point in gesture_sequence, and on the event in
gesture_recognizer_unittest.
> 
> Instead of tacking  the device-id to the event, why not make sure the
locations
> of the touch-events are correct for multi-monitor setting?
That was my initial thought as well, but I don't think that "correcting" the
locations of the touch events would handle the following case:
1. Press in the top right hand corner of the left monitor.
2. Press in the top left hand corner of the right monitor.

I believe that we'd place the second touch on the wrong monitor.

Also, miletus thought that this approach would be simpler. I'm not positive of
that, but it seems reasonably likely. There are lots of awkward cases we don't
want to have to test involving different ways of setting up multiple monitors,
and using the source device id feels like it would be less likely to fail in
these edge cases.

Is your concern with adding source_device_id that it's adding an extra word per
event, or that it's adding complexity?

[sadrul, 2014-01-07 22:00:20]

On 2014/01/07 21:57:50, tdresser wrote:
> On 2014/01/07 21:13:40, sadrul wrote:
> > You set the wrong bug# in the description.
> Whoops, fixed.
> > 
> > Where would you call set_source_device_id()?
> It's called on the gesture point in gesture_sequence, and on the event in
> gesture_recognizer_unittest.

Where would you call this on the event in non-test cases?

[tdresser, 2014-01-07 22:03:10]

On 2014/01/07 22:00:20, sadrul wrote:
> On 2014/01/07 21:57:50, tdresser wrote:
> > On 2014/01/07 21:13:40, sadrul wrote:
> > > You set the wrong bug# in the description.
> > Whoops, fixed.
> > > 
> > > Where would you call set_source_device_id()?
> > It's called on the gesture point in gesture_sequence, and on the event in
> > gesture_recognizer_unittest.
> 
> Where would you call this on the event in non-test cases?

Nowhere, it's set in the TouchEvent constructor.

[sadrul, 2014-01-07 22:39:25]

On 2014/01/07 22:03:10, tdresser wrote:
> On 2014/01/07 22:00:20, sadrul wrote:
> > On 2014/01/07 21:57:50, tdresser wrote:
> > > On 2014/01/07 21:13:40, sadrul wrote:
> > > > You set the wrong bug# in the description.
> > > Whoops, fixed.
> > > > 
> > > > Where would you call set_source_device_id()?
> > > It's called on the gesture point in gesture_sequence, and on the event in
> > > gesture_recognizer_unittest.
> > 
> > Where would you call this on the event in non-test cases?
> 
> Nowhere, it's set in the TouchEvent constructor.

Ah yes, I missed that, sorry. (I somehow thought the source-id was going to be
the display-id, and didn't think it could just as easily be the input device-id)

> > Instead of tacking  the device-id to the event, why not make sure the
> locations
> > of the touch-events are correct for multi-monitor setting?
> That was my initial thought as well, but I don't think that "correcting" the
> locations of the touch events would handle the following case:
> 1. Press in the top right hand corner of the left monitor.
> 2. Press in the top left hand corner of the right monitor.
> 
> I believe that we'd place the second touch on the wrong monitor.
> 
> Also, miletus thought that this approach would be simpler. I'm not positive of
> that, but it seems reasonably likely. There are lots of awkward cases we don't
> want to have to test involving different ways of setting up multiple monitors,
> and using the source device id feels like it would be less likely to fail in
> these edge cases.
> 
> Is your concern with adding source_device_id that it's adding an extra word
per
> event, or that it's adding complexity?

The change in size of Event is not a concern, but it seems unfortunate to have a
single GestureRecognizer for the whole system, and not use a single coordinate
space. Using the screen coordinate space (instead of the host coordinate space)
should be useful/sufficient for that?

[tdresser, 2014-01-08 20:39:34]

On 2014/01/07 22:39:25, sadrul wrote:
> On 2014/01/07 22:03:10, tdresser wrote:
> > On 2014/01/07 22:00:20, sadrul wrote:
> > > On 2014/01/07 21:57:50, tdresser wrote:
> > > > On 2014/01/07 21:13:40, sadrul wrote:
> > > > > You set the wrong bug# in the description.
> > > > Whoops, fixed.
> > > > > 
> > > > > Where would you call set_source_device_id()?
> > > > It's called on the gesture point in gesture_sequence, and on the event
in
> > > > gesture_recognizer_unittest.
> > > 
> > > Where would you call this on the event in non-test cases?
> > 
> > Nowhere, it's set in the TouchEvent constructor.
> 
> Ah yes, I missed that, sorry. (I somehow thought the source-id was going to be
> the display-id, and didn't think it could just as easily be the input
device-id)
> 
> > > Instead of tacking  the device-id to the event, why not make sure the
> > locations
> > > of the touch-events are correct for multi-monitor setting?
> > That was my initial thought as well, but I don't think that "correcting" the
> > locations of the touch events would handle the following case:
> > 1. Press in the top right hand corner of the left monitor.
> > 2. Press in the top left hand corner of the right monitor.
> > 
> > I believe that we'd place the second touch on the wrong monitor.
> > 
> > Also, miletus thought that this approach would be simpler. I'm not positive
of
> > that, but it seems reasonably likely. There are lots of awkward cases we
don't
> > want to have to test involving different ways of setting up multiple
monitors,
> > and using the source device id feels like it would be less likely to fail in
> > these edge cases.
> > 
> > Is your concern with adding source_device_id that it's adding an extra word
> per
> > event, or that it's adding complexity?
> 
> The change in size of Event is not a concern, but it seems unfortunate to have
a
> single GestureRecognizer for the whole system, and not use a single coordinate
> space. Using the screen coordinate space (instead of the host coordinate
space)
> should be useful/sufficient for that?

ping, as discussed offline.

[sadrul, 2014-01-08 20:50:52]

LGTM

We discussed offline: using a common coordinate space still has issues, e.g. if
there is are two touch-points in two different touch-screens, but in close
proximity in the common coordinate space, then the GR would target both touches
to the same window. This is undesirable. So this approach is better.

https://codereview.chromium.org/103173004/diff/1/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/103173004/diff/1/ui/events/event.cc#newcode452
ui/events/event.cc:452: XIDeviceEvent* xiev =
reinterpret_cast<XIDeviceEvent*>(event);
You don't need XIEvent. Just directly static_cast<> to XIDeviceEvent

https://codereview.chromium.org/103173004/diff/1/ui/events/event.cc#newcode488
ui/events/event.cc:488: source_device_id_(-1){
space before {

https://codereview.chromium.org/103173004/diff/1/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/103173004/diff/1/ui/events/event.h#newcode512
ui/events/event.h:512: // The device id of the screen the event came from.
Mention that this defaults to -1

[tdresser, 2014-01-09 19:07:35]

Most recent patches also addresses sadrul's comments. 

I'm surprised we weren't having more issues with tests failing due to the GR not
being reset. It was possible for a touch to get locked to a window which didn't
exist in the current test, due to data the GR persisted between tests.

https://codereview.chromium.org/103173004/diff/1/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/103173004/diff/1/ui/events/event.cc#newcode452
ui/events/event.cc:452: XIDeviceEvent* xiev =
reinterpret_cast<XIDeviceEvent*>(event);
On 2014/01/08 20:50:52, sadrul wrote:
> You don't need XIEvent. Just directly static_cast<> to XIDeviceEvent

Done.

https://codereview.chromium.org/103173004/diff/1/ui/events/event.cc#newcode488
ui/events/event.cc:488: source_device_id_(-1){
On 2014/01/08 20:50:52, sadrul wrote:
> space before {

Done.

https://codereview.chromium.org/103173004/diff/1/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/103173004/diff/1/ui/events/event.h#newcode512
ui/events/event.h:512: // The device id of the screen the event came from.
On 2014/01/08 20:50:52, sadrul wrote:
> Mention that this defaults to -1

Done.

[sadrul, 2014-01-09 19:11:56]

Cool. Thanks for the fix. SLGTM

[commit-bot: I haz the power, 2014-01-09 19:14:12]

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

[commit-bot: I haz the power, 2014-01-09 21:12:19]

Change committed as 243974