Support pen in TouchEventConverterEvdev

Support pen in TouchEventConverterEvdev

This change extends the TouchEventConverterEvdev to generate MouseEvents
when the device reports the BTN_TOOL_PEN key.

BUG=636458
Committed: https://crrev.com/6bc51438ed0dac4173c2b797e060bd36c605b960
Cr-Commit-Position: refs/heads/master@{#413923}

[denniskempin, 2016-08-15 22:38:26]

Description was changed from

==========
Support pen in TouchEventConverterEvdev

This change extends the TouchEventConverterEvdev to generate MouseEvents
when the device reports the BTN_TOOL_PEN key.

BUG=636458
==========

to

==========
Support pen in TouchEventConverterEvdev

This change extends the TouchEventConverterEvdev to generate MouseEvents
when the device reports the BTN_TOOL_PEN key.

BUG=636458
==========

[spang, 2016-08-16 21:36:20]

spang@chromium.org changed reviewers:
+ spang@chromium.org

[spang, 2016-08-16 21:36:21]

https://codereview.chromium.org/2248933002/diff/40001/ui/events/ozone/evdev/t...
File ui/events/ozone/evdev/touch_event_converter_evdev.cc (right):

https://codereview.chromium.org/2248933002/diff/40001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:340: if
(events_[current_slot_].was_touching)
Does this actually happen?

https://codereview.chromium.org/2248933002/diff/40001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:364: input_device_.id,
EF_DIRECT_INPUT, gfx::PointF(event.x, event.y), button,
There's no guarantee the X/Y position is updated by the time this code runs. All
events between two SYN_REPORT events are considered simultaneous.

So if you get ABS_X, ABS_Y, SYN_REPORT, BTN_LEFT, ABS_X, ABS_Y, SYN_REPORT in
order then the left click should happen at the 2nd position, not the first.

So I think you need to add some member variables

 btn_left_
 btn_right_
 btn_middle_

and just update those here.

At the end of the packet, in ReportEvents(), you can use them to send clicks as
necessary.

[denniskempin, 2016-08-17 21:47:16]

denniskempin@google.com changed reviewers:
+ sadrul@chromium.org

[denniskempin, 2016-08-17 21:47:17]

PTAL, this is ready for review.

https://codereview.chromium.org/2248933002/diff/40001/ui/events/ozone/evdev/t...
File ui/events/ozone/evdev/touch_event_converter_evdev.cc (right):

https://codereview.chromium.org/2248933002/diff/40001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:340: if
(events_[current_slot_].was_touching)
On 2016/08/16 21:36:21, spang wrote:
> Does this actually happen?
I think a kernel driver shouldn't be allowed to do this. 
BUT I put it there since this could theoretically get us into a state where
buttons are stuck if a device would do this. 
I don't feel strongly about keeping it.. it's mostly been an 'I feel safer this
way' kind of decision.

https://codereview.chromium.org/2248933002/diff/40001/ui/events/ozone/evdev/t...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:364: input_device_.id,
EF_DIRECT_INPUT, gfx::PointF(event.x, event.y), button,
On 2016/08/16 21:36:21, spang wrote:
> There's no guarantee the X/Y position is updated by the time this code runs.
All
> events between two SYN_REPORT events are considered simultaneous.
> 
> So if you get ABS_X, ABS_Y, SYN_REPORT, BTN_LEFT, ABS_X, ABS_Y, SYN_REPORT in
> order then the left click should happen at the 2nd position, not the first.
> 
> So I think you need to add some member variables
> 
>  btn_left_
>  btn_right_
>  btn_middle_
> 
> and just update those here.
> 
> At the end of the packet, in ReportEvents(), you can use them to send clicks
as
> necessary.

Done.

[spang, 2016-08-18 19:12:58]

Can you check a few tricky cases-

(1) Unplugging the device need to release mouse buttons.

This is handled in OnDisabled()

The TouchEventConverterEvdevTest.ShouldReleaseContactsOnStop test checks that
this works for touches.

(2) Queue overruns in the kernel needs cause dropped events and we need to make
sure to release mouse buttons if they are no longer actually pressed (or since
drops are rare, just unconditionally releasing them to avoid stuck buttons).

I think this works for touch but it looks like we don't have a very good test
for it.

The class handles state synchronization over a drop with the following general
strategy:

- Sync entire device state in Initialize()  (at startup, and after drops)
- Incrementally update the device state in ProcessMultitouchEvent
- In ReportEvents, update client state (e..g "was_touching") from device state
(e.g. touching) and send the corresponding events.

And for unplugging:

- OnDisabled() alters the device state as if there were no contacts, and runs
the above reconciliation (ReportEvents).

[denniskempin, 2016-08-19 00:34:11]

I do really like your approach to synchronization, it's far easier to work with
than libevdev which we used to use in Xorg times. 

I have added code to make sure buttons are released. Ideally I'd like to figure
out the key state by doing EVIOCGBIT ioctls during init, but I think for now it
should be ok to just make sure the buttons are not stuck. Let me know how you
feel about that!

[spang, 2016-08-22 20:29:09]

lgtm

[denniskempin, 2016-08-22 20:33:11]

The CQ bit was checked by denniskempin@google.com

[denniskempin, 2016-08-22 20:33:29]

On 2016/08/22 20:29:09, spang wrote:
> lgtm

Thanks for the review, Michael! I really appreciate it!

[commit-bot: I haz the power, 2016-08-22 20:34:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 20:47:19]

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

[commit-bot: I haz the power, 2016-08-22 20:47:19]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sadrul, 2016-08-23 06:28:47]

https://codereview.chromium.org/2248933002/diff/140001/ui/wm/core/compound_ev...
File ui/wm/core/compound_event_filter.cc (right):

https://codereview.chromium.org/2248933002/diff/140001/ui/wm/core/compound_ev...
ui/wm/core/compound_event_filter.cc:222: !(event->flags() &
ui::EF_DIRECT_INPUT));
Should this look at the pointer-type instead?

[denniskempin, 2016-08-23 15:23:47]

On 2016/08/23 06:28:47, sadrul wrote:
>
https://codereview.chromium.org/2248933002/diff/140001/ui/wm/core/compound_ev...
> File ui/wm/core/compound_event_filter.cc (right):
> 
>
https://codereview.chromium.org/2248933002/diff/140001/ui/wm/core/compound_ev...
> ui/wm/core/compound_event_filter.cc:222: !(event->flags() &
> ui::EF_DIRECT_INPUT));
> Should this look at the pointer-type instead?

I think using EF_DIRECT_INPUT would be a little more consistent since it is
possible for new pointer types to be added (for example we will be adding an
eraser pointer type). Also direct input implies that the user knows where the
pointer is through the way he is interacting with the device and does not need
the cursor.

[sadrul, 2016-08-23 23:25:25]

lgtm

[denniskempin, 2016-08-23 23:56:50]

The CQ bit was checked by denniskempin@google.com

[commit-bot: I haz the power, 2016-08-23 23:57:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 01:16:25]

Description was changed from

==========
Support pen in TouchEventConverterEvdev

This change extends the TouchEventConverterEvdev to generate MouseEvents
when the device reports the BTN_TOOL_PEN key.

BUG=636458
==========

to

==========
Support pen in TouchEventConverterEvdev

This change extends the TouchEventConverterEvdev to generate MouseEvents
when the device reports the BTN_TOOL_PEN key.

BUG=636458
==========

[commit-bot: I haz the power, 2016-08-24 01:16:30]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-08-24 01:18:04]

Description was changed from

==========
Support pen in TouchEventConverterEvdev

This change extends the TouchEventConverterEvdev to generate MouseEvents
when the device reports the BTN_TOOL_PEN key.

BUG=636458
==========

to

==========
Support pen in TouchEventConverterEvdev

This change extends the TouchEventConverterEvdev to generate MouseEvents
when the device reports the BTN_TOOL_PEN key.

BUG=636458

Committed: https://crrev.com/6bc51438ed0dac4173c2b797e060bd36c605b960
Cr-Commit-Position: refs/heads/master@{#413923}
==========

[commit-bot: I haz the power, 2016-08-24 01:18:06]

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