ozone: Map TouchEvent::touch_id_ into expected range

ozone: Map TouchEvent::touch_id_ into expected range

Touch events from evdev have a slot identifier to represent which
fingers are touched. Prior to this change this slot identifier was used
as the TouchEvent::touch_id_. However this breaks with multiple
touchscreens as the range for the slots will be the same.

This CL resolves that by using a SequentialIDGenerator to map a unique
device and slot id to an unused touch id in the sequence.

The touch id is released when the touch is released or cancelled.

BUG=472308
TEST=Manual testing on link_freon with uncommitted VLOG messages.

Committed: https://crrev.com/6b00b5adca67ecbf6accd74d7d51d9b596ef066b
Cr-Commit-Position: refs/heads/master@{#325255}

[robert.bradford, 2015-04-13 17:12:27]

robert.bradford@intel.com changed reviewers:
+ sadrul@chromium.org, spang@chromium.org

[robert.bradford, 2015-04-13 17:12:28]

robert.bradford@intel.com changed required reviewers:
+ sadrul@chromium.org, spang@chromium.org

[robert.bradford, 2015-04-13 17:12:28]

spang@: Please review the changes in ui/events/ozone

sadrul@: Please review the changes in ui/aura (and ui/events if you wish.)

Thanks!

[spang, 2015-04-13 17:19:21]

On 2015/04/13 17:12:28, robert.bradford wrote:
> spang@: Please review the changes in ui/events/ozone
> 
> sadrul@: Please review the changes in ui/aura (and ui/events if you wish.)
> 
> Thanks!

Does the UI code work if the touches aren't densely packed around 0?

The X11 code allocates fingers dynamically. If finger 0 isn't in use, the next
touch is finger 0. I'm thinking we may want to do the same.

It means, in particular, that having 10 fingers will always use ids 0 - 9 no
matter what combination of screens are used.

[spang, 2015-04-13 17:19:54]

https://codereview.chromium.org/1048333005/diff/1/ui/aura/window_event_dispat...
File ui/aura/window_event_dispatcher.h (right):

https://codereview.chromium.org/1048333005/diff/1/ui/aura/window_event_dispat...
ui/aura/window_event_dispatcher.h:243: std::set<int> touch_ids_down_;
std::bitset?

[robert.bradford, 2015-04-14 18:26:29]

> Does the UI code work if the touches aren't densely packed around 0?
> 
> The X11 code allocates fingers dynamically. If finger 0 isn't in use, the next
> touch is finger 0. I'm thinking we may want to do the same.
> 
> It means, in particular, that having 10 fingers will always use ids 0 - 9 no
> matter what combination of screens are used.

In follow up to this comment I rethought the approach. The touch_id is really
just a logical identifier and the first touch on any device should be touch_id =
0 and the second, whilst the first is down should be touch_id = 1.

I reused a bunch of the existing infrastructure from the X11 code to apply this
to Ozone. This change keeps the code to create unique ids across the devices
(and calls them tracking ids) but then uses a sequence generator to map the
tracking id into a touch id in the range [0, n (9 probably :-)].

The touch id is generated in the TouchEventConverterEvdev and the id released by
using the code already in the ui::TouchEvent destructor.

The unit test needed to be modified to ensure that the touch ids were always
released as it does not work with ui::TouchEvent but rather the intermediate
TouchEventParams.

With this reworked change it was no longer necessary to modify code ui/aura.

Let me know what you think.

[spang, 2015-04-14 18:46:42]

On 2015/04/14 18:26:29, robert.bradford wrote:
> > Does the UI code work if the touches aren't densely packed around 0?
> > 
> > The X11 code allocates fingers dynamically. If finger 0 isn't in use, the
next
> > touch is finger 0. I'm thinking we may want to do the same.
> > 
> > It means, in particular, that having 10 fingers will always use ids 0 - 9 no
> > matter what combination of screens are used.
> 
> In follow up to this comment I rethought the approach. The touch_id is really
> just a logical identifier and the first touch on any device should be touch_id
=
> 0 and the second, whilst the first is down should be touch_id = 1.

I agree, SGTM.

> 
> I reused a bunch of the existing infrastructure from the X11 code to apply
this
> to Ozone. This change keeps the code to create unique ids across the devices
> (and calls them tracking ids) but then uses a sequence generator to map the
> tracking id into a touch id in the range [0, n (9 probably :-)].
> 
> The touch id is generated in the TouchEventConverterEvdev and the id released
by
> using the code already in the ui::TouchEvent destructor.

This is asymmetric. It doesn't make sense that ui::TouchEvent would release
something created by TouchEventConverterEvdev.

Please move all of the logic inside ui/events/ozone/evdev, and in particular
don't use set_should_remove_native_touch_id_mapping(). NativeEvents and all
associated semantics are deprecated.

I don't think we need a DeviceDataManagerOzone. EventFactoryEvdev is the right
place to put multi-device state such as touch id mapping (see for example
KeyboardEvdev).

Shouldn't even need to change TouchEventConverterEvdev. We can send {device id,
slot} pairs from there and map them to touch ids in
EventFactoryEvdev::DispatchTouchEvent.

> 
> The unit test needed to be modified to ensure that the touch ids were always
> released as it does not work with ui::TouchEvent but rather the intermediate
> TouchEventParams.
> 
> With this reworked change it was no longer necessary to modify code ui/aura.
> 
> Let me know what you think.

Thanks!

[spang, 2015-04-14 18:47:24]

On 2015/04/14 18:46:42, spang wrote:
> On 2015/04/14 18:26:29, robert.bradford wrote:
> > > Does the UI code work if the touches aren't densely packed around 0?
> > > 
> > > The X11 code allocates fingers dynamically. If finger 0 isn't in use, the
> next
> > > touch is finger 0. I'm thinking we may want to do the same.
> > > 
> > > It means, in particular, that having 10 fingers will always use ids 0 - 9
no
> > > matter what combination of screens are used.
> > 
> > In follow up to this comment I rethought the approach. The touch_id is
really
> > just a logical identifier and the first touch on any device should be
touch_id
> =
> > 0 and the second, whilst the first is down should be touch_id = 1.
> 
> I agree, SGTM.
> 
> > 
> > I reused a bunch of the existing infrastructure from the X11 code to apply
> this
> > to Ozone. This change keeps the code to create unique ids across the devices
> > (and calls them tracking ids) but then uses a sequence generator to map the
> > tracking id into a touch id in the range [0, n (9 probably :-)].
> > 
> > The touch id is generated in the TouchEventConverterEvdev and the id
released
> by
> > using the code already in the ui::TouchEvent destructor.
> 
> This is asymmetric. It doesn't make sense that ui::TouchEvent would release
> something created by TouchEventConverterEvdev.

It's also not thread safe :)

> 
> Please move all of the logic inside ui/events/ozone/evdev, and in particular
> don't use set_should_remove_native_touch_id_mapping(). NativeEvents and all
> associated semantics are deprecated.
> 
> I don't think we need a DeviceDataManagerOzone. EventFactoryEvdev is the right
> place to put multi-device state such as touch id mapping (see for example
> KeyboardEvdev).
> 
> Shouldn't even need to change TouchEventConverterEvdev. We can send {device
id,
> slot} pairs from there and map them to touch ids in
> EventFactoryEvdev::DispatchTouchEvent.
> 
> > 
> > The unit test needed to be modified to ensure that the touch ids were always
> > released as it does not work with ui::TouchEvent but rather the intermediate
> > TouchEventParams.
> > 
> > With this reworked change it was no longer necessary to modify code ui/aura.
> > 
> > Let me know what you think.
> 
> Thanks!

[robert.bradford, 2015-04-15 15:20:54]

> > Shouldn't even need to change TouchEventConverterEvdev. We can send {device
> id,
> > slot} pairs from there and map them to touch ids in
> > EventFactoryEvdev::DispatchTouchEvent.
> > 

What about this version? So much simpler: all the logic is moved to
EventFactoryEvdev. The other changes are only to s/touch_id/slot/ in
TouchEventParams to make it's meaning very precise.

Is it safe to release the ID after the dispatch? I believe so as a PRESSED will
always come after a RELEASED for the same touch id, so any use asynchronously of
a TouchEvent should preserve that ordering (or there would be other problems!)

Thanks for your patience :-)

[robert.bradford, 2015-04-15 15:21:26]

robert.bradford@intel.com changed required reviewers:
- sadrul@chromium.org

[spang, 2015-04-15 15:49:04]

lgtm

[robert.bradford, 2015-04-15 15:49:28]

The CQ bit was checked by robert.bradford@intel.com

[commit-bot: I haz the power, 2015-04-15 15:50:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1048333005/60001

[commit-bot: I haz the power, 2015-04-15 16:20:06]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-15 16:20:51]

Patchset 4 (id:??) landed as
https://crrev.com/6b00b5adca67ecbf6accd74d7d51d9b596ef066b
Cr-Commit-Position: refs/heads/master@{#325255}