Ozone support for device special cases in keyboard event rewriting.

Ozone support for device special cases in keyboard event rewriting.

Keyboard event rewriting has some special cases keyed on the device
name (Apple keyboards) or USB VID/PID (Hotrod remote), which had been
handled by special-case device inspection under X11, and not handled
at all under Ozone. This moves the properties to ui::DeviceDataManager
and makes the use in chromeos::EventRewriter platform-independent.

BUG=471753
Committed: https://crrev.com/711f2fc883c05bb10466fb65fdaf9bc10c909b23
Cr-Commit-Position: refs/heads/master@{#325660}

Committed: https://crrev.com/16ce3d88cfcb6ab69af3dc8ba445da47d80c1d9b
Cr-Commit-Position: refs/heads/master@{#327060}

[kpschoedel, 2015-04-08 19:15:50]

kpschoedel@chromium.org changed reviewers:
+ derat@chromium.org, sadrul@chromium.org, spang@chromium.org

[kpschoedel, 2015-04-08 19:15:50]

+derat for chrome/browser/chromeos/events/event_rewriter.cc

+spang for ui/events/ozone/evdev/*

+sadrul for the rest of the world

Sadrul: I can see now unifying the keyboard classification of event_rewriter.cc
(Apple, Hotrod) and x11_hotplug_event_handler.cc (non-keyboard key event
sources) somewhere, perhaps ui::KeyboardDevice methods — what do you think?

[Daniel Erat, 2015-04-08 19:24:24]

https://codereview.chromium.org/1073573002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/events/event_rewriter.cc (right):

https://codereview.chromium.org/1073573002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/events/event_rewriter.cc:890: if (keyboard.id ==
static_cast<unsigned int>(device_id)) {
this is a wide-ranging comment, and doesn't look like it's directly related to
this change, but ui::InputDevice shouldn't be using an "unsigned int" type:
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types

xlib uses "unsigned" all over the place, but we should convert these into a
safer type (probably uint64_t if it could be either 32- or 64-bit depending on
the underlying architecture) as soon as they make it into chromium.

[kpschoedel, 2015-04-08 21:54:50]

On 2015/04/08 19:24:24, Daniel Erat wrote:
>
https://codereview.chromium.org/1073573002/diff/20001/chrome/browser/chromeos...
> File chrome/browser/chromeos/events/event_rewriter.cc (right):

Both the XInput2 and Ozone/evdev device IDs are plain (signed) 'int'. As Sadrul
owns input_device.h I'll wait for him to pick a type.

[spang, 2015-04-08 22:54:30]

https://codereview.chromium.org/1073573002/diff/20001/ui/events/ozone/evdev/e...
File ui/events/ozone/evdev/event_converter_evdev.cc (right):

https://codereview.chromium.org/1073573002/diff/20001/ui/events/ozone/evdev/e...
ui/events/ozone/evdev/event_converter_evdev.cc:32: if (ioctl(fd,
EVIOCGNAME(kMaximumDeviceNameLength - 1), &device_name) < 0) {
Can you move these to EventDeviceInfo? That's where we've been putting these
ioctls in there and it makes testing easier.

https://codereview.chromium.org/1073573002/diff/20001/ui/events/ozone/evdev/k...
File ui/events/ozone/evdev/keyboard_evdev.cc (right):

https://codereview.chromium.org/1073573002/diff/20001/ui/events/ozone/evdev/k...
ui/events/ozone/evdev/keyboard_evdev.cc:66: repeat_device_id_(0),
We're using -1 for invalid/synthetic/injected device id.

[spang, 2015-04-08 22:56:53]

https://codereview.chromium.org/1073573002/diff/20001/ui/events/ozone/evdev/k...
File ui/events/ozone/evdev/keyboard_evdev.cc (right):

https://codereview.chromium.org/1073573002/diff/20001/ui/events/ozone/evdev/k...
ui/events/ozone/evdev/keyboard_evdev.cc:66: repeat_device_id_(0),
On 2015/04/08 22:54:30, spang wrote:
> We're using -1 for invalid/synthetic/injected device id.

Oh, it appears I'm wrong. Nevermind.

[sadrul, 2015-04-09 01:07:43]

On 2015/04/08 19:15:50, kpschoedel wrote:
> ...
> Sadrul: I can see now unifying the keyboard classification of
event_rewriter.cc
> (Apple, Hotrod) and x11_hotplug_event_handler.cc (non-keyboard key event
> sources) somewhere, perhaps ui::KeyboardDevice methods — what do you think?

Yep, sgtm.

https://codereview.chromium.org/1073573002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/events/event_rewriter.cc (right):

https://codereview.chromium.org/1073573002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/events/event_rewriter.cc:890: if (keyboard.id ==
static_cast<unsigned int>(device_id)) {
On 2015/04/08 19:24:24, Daniel Erat wrote:
> this is a wide-ranging comment, and doesn't look like it's directly related to
> this change, but ui::InputDevice shouldn't be using an "unsigned int" type:
> https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types
> 
> xlib uses "unsigned" all over the place, but we should convert these into a
> safer type (probably uint64_t if it could be either 32- or 64-bit depending on
> the underlying architecture) as soon as they make it into chromium.

Agree. We use int for Event::source_device_id_ too, so it doesn't really make
sense for InputDevice::id to be unsigned int.

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

https://codereview.chromium.org/1073573002/diff/20001/ui/events/event.cc#newc...
ui/events/event.cc:248: #endif
Should the source-device-id be set here for OZONE instead of in KeyEvent ctor?

https://codereview.chromium.org/1073573002/diff/20001/ui/events/platform/x11/...
File ui/events/platform/x11/x11_hotplug_event_handler.cc (right):

https://codereview.chromium.org/1073573002/diff/20001/ui/events/platform/x11/...
ui/events/platform/x11/x11_hotplug_event_handler.cc:254: }
The X11 calls in this function are happening in the worker thread (i.e. in
non-UI thread), right? I believe we want to avoid doing that.

https://codereview.chromium.org/1073573002/diff/20001/ui/events/platform/x11/...
ui/events/platform/x11/x11_hotplug_event_handler.cc:273:
SetDeviceIdentification(&keyboard);
Note that we also want to collect the product/vendor ID for touchscreens. (see
//chrome/browser/metrics/chromeos_metrics_provider.cc:WriteExternalTouchscreensProto().
The code for this in TouchFactory should be removed, and use this new code
instead)

Is there a reason to not do this for all device types?

[kpschoedel, 2015-04-09 16:30:23]

Patchset #3 (id:40001) has been deleted

[kpschoedel, 2015-04-09 18:51:41]

Now depends on https://codereview.chromium.org/1071193002

https://codereview.chromium.org/1073573002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/events/event_rewriter.cc (right):

https://codereview.chromium.org/1073573002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/events/event_rewriter.cc:890: if (keyboard.id ==
static_cast<unsigned int>(device_id)) {
On 2015/04/09 01:07:43, sadrul wrote:
> On 2015/04/08 19:24:24, Daniel Erat wrote:
> > this is a wide-ranging comment, and doesn't look like it's directly related
to
> > this change, but ui::InputDevice shouldn't be using an "unsigned int" type:
> >
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types
> > 
> > xlib uses "unsigned" all over the place, but we should convert these into a
> > safer type (probably uint64_t if it could be either 32- or 64-bit depending
on
> > the underlying architecture) as soon as they make it into chromium.
> 
> Agree. We use int for Event::source_device_id_ too, so it doesn't really make
> sense for InputDevice::id to be unsigned int.

Moved this to its own CL since it's conceptually independent and touches a
number of files otherwise unrelated to this one.

https://codereview.chromium.org/1071193002

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

https://codereview.chromium.org/1073573002/diff/20001/ui/events/event.cc#newc...
ui/events/event.cc:248: #endif
On 2015/04/09 01:07:43, sadrul wrote:
> Should the source-device-id be set here for OZONE instead of in KeyEvent ctor?

Done.

https://codereview.chromium.org/1073573002/diff/20001/ui/events/ozone/evdev/e...
File ui/events/ozone/evdev/event_converter_evdev.cc (right):

https://codereview.chromium.org/1073573002/diff/20001/ui/events/ozone/evdev/e...
ui/events/ozone/evdev/event_converter_evdev.cc:32: if (ioctl(fd,
EVIOCGNAME(kMaximumDeviceNameLength - 1), &device_name) < 0) {
On 2015/04/08 22:54:30, spang wrote:
> Can you move these to EventDeviceInfo? That's where we've been putting these
> ioctls in there and it makes testing easier.

Done.

https://codereview.chromium.org/1073573002/diff/20001/ui/events/ozone/evdev/k...
File ui/events/ozone/evdev/keyboard_evdev.cc (right):

https://codereview.chromium.org/1073573002/diff/20001/ui/events/ozone/evdev/k...
ui/events/ozone/evdev/keyboard_evdev.cc:66: repeat_device_id_(0),
On 2015/04/08 22:56:53, spang wrote:
> On 2015/04/08 22:54:30, spang wrote:
> > We're using -1 for invalid/synthetic/injected device id.
> 
> Oh, it appears I'm wrong. Nevermind.

Acknowledged.

https://codereview.chromium.org/1073573002/diff/20001/ui/events/platform/x11/...
File ui/events/platform/x11/x11_hotplug_event_handler.cc (right):

https://codereview.chromium.org/1073573002/diff/20001/ui/events/platform/x11/...
ui/events/platform/x11/x11_hotplug_event_handler.cc:254: }
On 2015/04/09 01:07:43, sadrul wrote:
> The X11 calls in this function are happening in the worker thread (i.e. in
> non-UI thread), right? I believe we want to avoid doing that.

Done.

https://codereview.chromium.org/1073573002/diff/20001/ui/events/platform/x11/...
ui/events/platform/x11/x11_hotplug_event_handler.cc:273:
SetDeviceIdentification(&keyboard);
On 2015/04/09 01:07:43, sadrul wrote:
> Note that we also want to collect the product/vendor ID for touchscreens. (see
>
//chrome/browser/metrics/chromeos_metrics_provider.cc:WriteExternalTouchscreensProto().
> The code for this in TouchFactory should be removed, and use this new code
> instead)

Oh, joy.

> Is there a reason to not do this for all device types?

Purely laz^H^H^H velocity in fixing the Command-key regression.

Done.

[Daniel Erat, 2015-04-09 18:53:16]

lgtm for chrome/

[spang, 2015-04-09 19:18:00]

lgtm

[sadrul, 2015-04-09 20:44:29]

https://codereview.chromium.org/1073573002/diff/60001/ui/events/platform/x11/...
File ui/events/platform/x11/x11_hotplug_event_handler.cc (right):

https://codereview.chromium.org/1073573002/diff/60001/ui/events/platform/x11/...
ui/events/platform/x11/x11_hotplug_event_handler.cc:449:
XInternAtom(gfx::GetXDisplay(), XI_PROP_PRODUCT_ID, 1);
Use atom_cache_ instead.

https://codereview.chromium.org/1073573002/diff/60001/ui/events/platform/x11/...
ui/events/platform/x11/x11_hotplug_event_handler.cc:464: }
Whoops. I think |product_info| is leaking. XFree

[kpschoedel, 2015-04-09 21:22:15]

https://codereview.chromium.org/1073573002/diff/60001/ui/events/platform/x11/...
File ui/events/platform/x11/x11_hotplug_event_handler.cc (right):

https://codereview.chromium.org/1073573002/diff/60001/ui/events/platform/x11/...
ui/events/platform/x11/x11_hotplug_event_handler.cc:449:
XInternAtom(gfx::GetXDisplay(), XI_PROP_PRODUCT_ID, 1);
On 2015/04/09 20:44:29, sadrul wrote:
> Use atom_cache_ instead.

Done.

https://codereview.chromium.org/1073573002/diff/60001/ui/events/platform/x11/...
ui/events/platform/x11/x11_hotplug_event_handler.cc:464: }
On 2015/04/09 20:44:29, sadrul wrote:
> Whoops. I think |product_info| is leaking. XFree

Done. (It used to leak in EventRewriter, too.)

[sadrul, 2015-04-09 21:48:11]

lgtm

[kpschoedel, 2015-04-15 21:44:36]

The CQ bit was checked by kpschoedel@chromium.org

[kpschoedel, 2015-04-15 21:44:37]

The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org,
spang@chromium.org
Link to the patchset: https://codereview.chromium.org/1073573002/#ps80001
(title: "address review comments II")

[commit-bot: I haz the power, 2015-04-15 21:44:46]

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

[commit-bot: I haz the power, 2015-04-15 23:20:00]

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

[commit-bot: I haz the power, 2015-04-15 23:20:01]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[kpschoedel, 2015-04-16 22:08:50]

The CQ bit was checked by kpschoedel@chromium.org

[kpschoedel, 2015-04-16 22:08:51]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
derat@chromium.org, spang@chromium.org
Link to the patchset: https://codereview.chromium.org/1073573002/#ps100001
(title: "proper #includes to fix Windows build")

[commit-bot: I haz the power, 2015-04-16 22:12:35]

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

[commit-bot: I haz the power, 2015-04-16 22:56:37]

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

[commit-bot: I haz the power, 2015-04-16 22:56:38]

Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kpschoedel, 2015-04-17 16:09:07]

The CQ bit was checked by kpschoedel@chromium.org to run a CQ dry run

[kpschoedel, 2015-04-17 16:09:08]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
derat@chromium.org, spang@chromium.org
Link to the patchset: https://codereview.chromium.org/1073573002/#ps120001
(title: "no <cstdint> in Android?!?")

[commit-bot: I haz the power, 2015-04-17 16:10:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1073573002/120001

[commit-bot: I haz the power, 2015-04-17 17:09:49]

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

[commit-bot: I haz the power, 2015-04-17 17:09:50]

Dry run: This issue passed the CQ dry run.

[kpschoedel, 2015-04-17 17:16:22]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2015-04-17 17:16:36]

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

[commit-bot: I haz the power, 2015-04-17 17:23:32]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-04-17 17:24:57]

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

[Tim Song, 2015-04-22 21:27:22]

A revert of this CL (patchset #6 id:120001) has been created in
https://codereview.chromium.org/1102673002/ by tengs@chromium.org.

The reason for reverting is: Bisected this CL for crbug.com/478605.

Chrome crashes upon start-up in ASAN builds, suggesting there is a invalid
memory access somewhere in this CL..

[kpschoedel, 2015-04-27 15:41:13]

The CQ bit was checked by kpschoedel@chromium.org to run a CQ dry run

[kpschoedel, 2015-04-27 15:41:13]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
derat@chromium.org, spang@chromium.org
Link to the patchset: https://codereview.chromium.org/1073573002/#ps160001
(title: "Fix crash (thanks spang)")

[commit-bot: I haz the power, 2015-04-27 15:41:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1073573002/160001

[commit-bot: I haz the power, 2015-04-27 16:51:38]

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

[commit-bot: I haz the power, 2015-04-27 16:51:45]

Dry run: This issue passed the CQ dry run.

[kpschoedel, 2015-04-27 16:57:05]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2015-04-27 16:57:38]

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

[commit-bot: I haz the power, 2015-04-27 16:59:07]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2015-04-27 17:00:26]

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