Adds special support to the device manager for keyboards devices.

Adds special support to the device manager for keyboards devices.

This is the first step in using presence of different types of keyboards in automatically deploying the virtual keyboard.

BUG=373402
Committed: https://crrev.com/d761ce02ab03d1a40f053a9c054a5c0473d94906
Cr-Commit-Position: refs/heads/master@{#300764}

[rsadam, 2014-10-01 20:57:17]

rsadam@chromium.org changed reviewers:
+ sadrul@chromium.org

[rsadam, 2014-10-01 20:57:18]

Hey Sadrul, PTAL!

[rsadam, 2014-10-01 21:26:14]

rsadam@chromium.org changed reviewers:
+ dnicoara@chromium.org, flackr@chromium.org

[rsadam, 2014-10-01 21:27:05]

+flackr, dnicoara

[dnicoara, 2014-10-02 13:58:20]

dnicoara@chromium.org changed reviewers:
+ kpschoedel@chromium.org

[dnicoara, 2014-10-02 13:58:20]

+ kpschoedel@ since he's been working on X11 & keyboards for a while now.

https://codereview.chromium.org/618283003/diff/1/ui/events/device_hotplug_eve...
File ui/events/device_hotplug_event_observer.h (right):

https://codereview.chromium.org/618283003/diff/1/ui/events/device_hotplug_eve...
ui/events/device_hotplug_event_observer.h:19: // On a hotplug event this is
called with the list of available  touchscreen
nit: remove extra space

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

https://codereview.chromium.org/618283003/diff/1/ui/events/keyboard_device.h#...
ui/events/keyboard_device.h:23: // ID of the touch screen. This ID must uniquely
identify the touch screen.
Fix comment

https://codereview.chromium.org/618283003/diff/1/ui/events/x/hotplug_event_ha...
File ui/events/x/hotplug_event_handler_x11.cc (right):

https://codereview.chromium.org/618283003/diff/1/ui/events/x/hotplug_event_ha...
ui/events/x/hotplug_event_handler_x11.cc:38: bool IsKeyboard(std::string name) {
Not sure if this would cover all keyboard devices.

https://codereview.chromium.org/618283003/diff/1/ui/events/x/hotplug_event_ha...
ui/events/x/hotplug_event_handler_x11.cc:152: } else if (device_name ==
kTestKeyboardName) {
Is there anything special about test devices?

[rsadam, 2014-10-02 14:59:31]

https://codereview.chromium.org/618283003/diff/1/ui/events/device_hotplug_eve...
File ui/events/device_hotplug_event_observer.h (right):

https://codereview.chromium.org/618283003/diff/1/ui/events/device_hotplug_eve...
ui/events/device_hotplug_event_observer.h:19: // On a hotplug event this is
called with the list of available  touchscreen
On 2014/10/02 13:58:20, dnicoara wrote:
> nit: remove extra space

Done.

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

https://codereview.chromium.org/618283003/diff/1/ui/events/keyboard_device.h#...
ui/events/keyboard_device.h:23: // ID of the touch screen. This ID must uniquely
identify the touch screen.
On 2014/10/02 13:58:20, dnicoara wrote:
> Fix comment

Done.

https://codereview.chromium.org/618283003/diff/1/ui/events/x/hotplug_event_ha...
File ui/events/x/hotplug_event_handler_x11.cc (right):

https://codereview.chromium.org/618283003/diff/1/ui/events/x/hotplug_event_ha...
ui/events/x/hotplug_event_handler_x11.cc:38: bool IsKeyboard(std::string name) {
On 2014/10/02 13:58:20, dnicoara wrote:
> Not sure if this would cover all keyboard devices.

Added an UNKNOWN Enum, and marked these devices as that. This way we won't miss
any devices, and we correctly label the ones that are.

https://codereview.chromium.org/618283003/diff/1/ui/events/x/hotplug_event_ha...
ui/events/x/hotplug_event_handler_x11.cc:152: } else if (device_name ==
kTestKeyboardName) {
On 2014/10/02 13:58:20, dnicoara wrote:
> Is there anything special about test devices?

Each Master device has a XTEST device associated to it, which isn't an actual
keyboard. We want to distinguish these from physical devices.

[flackr, 2014-10-02 15:41:10]

https://codereview.chromium.org/618283003/diff/1/ui/events/x/hotplug_event_ha...
File ui/events/x/hotplug_event_handler_x11.cc (right):

https://codereview.chromium.org/618283003/diff/1/ui/events/x/hotplug_event_ha...
ui/events/x/hotplug_event_handler_x11.cc:38: bool IsKeyboard(std::string name) {
On 2014/10/02 13:58:20, dnicoara wrote:
> Not sure if this would cover all keyboard devices.

Agreed. I think false negatives are okay for how we're planning on using this,
since it will just mean we won't dismiss the keyboard when it's plugged in,
which is the current behavior for any keyboard. You should document that
explicitly though, that this can return false negatives.

https://codereview.chromium.org/618283003/diff/1/ui/events/x/hotplug_event_ha...
ui/events/x/hotplug_event_handler_x11.cc:152: } else if (device_name ==
kTestKeyboardName) {
On 2014/10/02 13:58:20, dnicoara wrote:
> Is there anything special about test devices?

This device shouldn't be considered a user accessible keyboard, and so we should
still display the virtual keyboard.

https://codereview.chromium.org/618283003/diff/40001/ui/events/keyboard_device.h
File ui/events/keyboard_device.h (right):

https://codereview.chromium.org/618283003/diff/40001/ui/events/keyboard_devic...
ui/events/keyboard_device.h:12: enum KeyboardDeviceType {
Document what these types mean.

https://codereview.chromium.org/618283003/diff/40001/ui/events/keyboard_devic...
ui/events/keyboard_device.h:16: UNKNOWN,
Also specifically that a real keyboard may be identified as unknown (could
mention if it doesn't have "keyboard" in device name) just to ensure this type
isn't relied upon.

[rsadam, 2014-10-02 15:50:05]

https://codereview.chromium.org/618283003/diff/40001/ui/events/keyboard_device.h
File ui/events/keyboard_device.h (right):

https://codereview.chromium.org/618283003/diff/40001/ui/events/keyboard_devic...
ui/events/keyboard_device.h:12: enum KeyboardDeviceType {
On 2014/10/02 15:41:10, flackr wrote:
> Document what these types mean.

Done.

https://codereview.chromium.org/618283003/diff/40001/ui/events/keyboard_devic...
ui/events/keyboard_device.h:16: UNKNOWN,
On 2014/10/02 15:41:10, flackr wrote:
> Also specifically that a real keyboard may be identified as unknown (could
> mention if it doesn't have "keyboard" in device name) just to ensure this type
> isn't relied upon.

Done.

[sadrul, 2014-10-02 15:53:54]

[ My comments are in patchset 3. Looks like you have already uploaded a new
patchset. From a quick look: my comments in PS3 also apply to PS4. ]

https://codereview.chromium.org/618283003/diff/40001/ui/events/device_data_ma...
File ui/events/device_data_manager.cc (right):

https://codereview.chromium.org/618283003/diff/40001/ui/events/device_data_ma...
ui/events/device_data_manager.cc:120: }
You should explicitly call into OnInputDeviceConfigurationChanged() from here
and OnTouchscreenDevicesUpdated(), and not have that in the delegate interface.

https://codereview.chromium.org/618283003/diff/40001/ui/events/keyboard_device.h
File ui/events/keyboard_device.h (right):

https://codereview.chromium.org/618283003/diff/40001/ui/events/keyboard_devic...
ui/events/keyboard_device.h:14: TEST,
I don't think we should have this.

https://codereview.chromium.org/618283003/diff/40001/ui/events/keyboard_devic...
ui/events/keyboard_device.h:27: KeyboardDeviceType type;
For TouchscreenDevice, we use a bool |is_internal| instead. It would be useful
to have the same approach here too. Or alternatively:

. Have a generic InputDevice
  - id (will be kInvalidId if unknown ]
  - type: [ internal, external, unknown ]
  - name (optional)

. TouchscreenDevice : InputDevice
  - size

. KeyboardDevice : InputDevice

https://codereview.chromium.org/618283003/diff/40001/ui/events/x/hotplug_even...
File ui/events/x/hotplug_event_handler_x11.cc (right):

https://codereview.chromium.org/618283003/diff/40001/ui/events/x/hotplug_even...
ui/events/x/hotplug_event_handler_x11.cc:32: const char kInternalKeyboardName[]
= "AT Translated Set 2 keyboard";
Is this only on chromeos? (i.e. does x11/linux devices also use this name for
internal keyboard?)

https://codereview.chromium.org/618283003/diff/40001/ui/events/x/hotplug_even...
ui/events/x/hotplug_event_handler_x11.cc:38: bool IsKnownKeyboard(std::string
name) {
const &, and make an internal copy if you need to ::tolower()

[rsadam, 2014-10-02 22:52:28]

The motivation between using two methods and removing
ONINputDeviceConfigurationChanged was as follows:

If we call it from both handlers, this would lead to duplicate events being
fired, which is messy. We only want listeners to be informed once about the
event. As a result, we check for changes in data_device_manager and fire off
observers iff the specific device list change. This approach will make adding
future device support (e.g mice) easier.

https://codereview.chromium.org/618283003/diff/40001/ui/events/device_data_ma...
File ui/events/device_data_manager.cc (right):

https://codereview.chromium.org/618283003/diff/40001/ui/events/device_data_ma...
ui/events/device_data_manager.cc:120: }
On 2014/10/02 15:53:53, sadrul wrote:
> You should explicitly call into OnInputDeviceConfigurationChanged() from here
> and OnTouchscreenDevicesUpdated(), and not have that in the delegate
interface.

Changed to two methods as discussed offline.

https://codereview.chromium.org/618283003/diff/40001/ui/events/keyboard_device.h
File ui/events/keyboard_device.h (right):

https://codereview.chromium.org/618283003/diff/40001/ui/events/keyboard_devic...
ui/events/keyboard_device.h:14: TEST,
On 2014/10/02 15:53:53, sadrul wrote:
> I don't think we should have this.

Done.

https://codereview.chromium.org/618283003/diff/40001/ui/events/keyboard_devic...
ui/events/keyboard_device.h:27: KeyboardDeviceType type;
On 2014/10/02 15:53:53, sadrul wrote:
> For TouchscreenDevice, we use a bool |is_internal| instead. It would be useful
> to have the same approach here too. Or alternatively:
> 
> . Have a generic InputDevice
>   - id (will be kInvalidId if unknown ]
>   - type: [ internal, external, unknown ]
>   - name (optional)
> 
> . TouchscreenDevice : InputDevice
>   - size
> 
> . KeyboardDevice : InputDevice

Done.

https://codereview.chromium.org/618283003/diff/40001/ui/events/x/hotplug_even...
File ui/events/x/hotplug_event_handler_x11.cc (right):

https://codereview.chromium.org/618283003/diff/40001/ui/events/x/hotplug_even...
ui/events/x/hotplug_event_handler_x11.cc:32: const char kInternalKeyboardName[]
= "AT Translated Set 2 keyboard";
I believe this is Unix wide.
(http://unix.stackexchange.com/questions/89538/how-to-detect-which-keyboard-de...)

https://codereview.chromium.org/618283003/diff/40001/ui/events/x/hotplug_even...
ui/events/x/hotplug_event_handler_x11.cc:38: bool IsKnownKeyboard(std::string
name) {
On 2014/10/02 15:53:53, sadrul wrote:
> const &, and make an internal copy if you need to ::tolower()

Done.

[sadrul, 2014-10-03 16:34:54]

Looks like ui/events/input_device* files are missing?

[rsadam, 2014-10-03 16:54:03]

Oops, forgot to git add them.

[flackr, 2014-10-04 00:35:13]

https://codereview.chromium.org/618283003/diff/120001/ui/events/device_data_m...
File ui/events/device_data_manager.cc (right):

https://codereview.chromium.org/618283003/diff/120001/ui/events/device_data_m...
ui/events/device_data_manager.cc:129: std::equal(devices.begin(), devices.end(),
Did you mean !std::equal(...?

https://codereview.chromium.org/618283003/diff/120001/ui/events/device_data_m...
ui/events/device_data_manager.cc:130: touchscreen_devices_.begin(),
TouchscreenEquals)) {
nit: indent 4 more (to show it's wrapped from std::equal(...
Or line up with devices.begin() after the '(' above.

https://codereview.chromium.org/618283003/diff/120001/ui/events/device_data_m...
ui/events/device_data_manager.cc:131: touchscreen_devices_ = devices;
Better yet, early return if the device list looks the same rather than wrapping
the contents of the function in an if.

https://codereview.chromium.org/618283003/diff/120001/ui/events/device_data_m...
ui/events/device_data_manager.cc:141: std::equal(devices.begin(), devices.end(),
ditto, !std::equal?

https://codereview.chromium.org/618283003/diff/120001/ui/events/device_data_m...
ui/events/device_data_manager.cc:142: keyboard_devices_.begin(),
KeyboardEquals)) {
ditto, indent 4 more or to just past '(' above.

https://codereview.chromium.org/618283003/diff/120001/ui/events/input_device.h
File ui/events/input_device.h (right):

https://codereview.chromium.org/618283003/diff/120001/ui/events/input_device....
ui/events/input_device.h:17: UNKNOWN,  // Device that may or may not be an
external device.
nit: align comment.

https://codereview.chromium.org/618283003/diff/120001/ui/events/x/hotplug_eve...
File ui/events/x/hotplug_event_handler_x11.cc (right):

https://codereview.chromium.org/618283003/diff/120001/ui/events/x/hotplug_eve...
ui/events/x/hotplug_event_handler_x11.cc:35: // Filters out all devices that
that are not known keyboards. This may return
nit: This function only determines devices that are not known keyboards for one
device, not all devices. Perhaps update comment to:

// Returns true if |name| is the name of a known keyboard device. Note, this may
return false negatives.

See also style guide on descriptive function comments:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm...

https://codereview.chromium.org/618283003/diff/120001/ui/events/x/hotplug_eve...
ui/events/x/hotplug_event_handler_x11.cc:43: // Filters out all devices that
that are XTEST keyboards.
Ditto.

https://codereview.chromium.org/618283003/diff/120001/ui/events/x/hotplug_eve...
ui/events/x/hotplug_event_handler_x11.cc:153: continue;
No need for else if after a continue. Perhaps put an empty line afterwards to
clearly separate from cases where device is added.

[flackr, 2014-10-04 00:51:31]

https://codereview.chromium.org/618283003/diff/120001/ui/events/device_data_m...
File ui/events/device_data_manager.h (right):

https://codereview.chromium.org/618283003/diff/120001/ui/events/device_data_m...
ui/events/device_data_manager.h:71: const std::vector<KeyboardDevice>& devices)
OVERRIDE;
Can you override this in DeviceDataManagerX11 to remove blocked devices? This
way we don't need to consider maximize mode a special case.

[rsadam, 2014-10-04 18:46:27]

https://codereview.chromium.org/618283003/diff/120001/ui/events/device_data_m...
File ui/events/device_data_manager.cc (right):

https://codereview.chromium.org/618283003/diff/120001/ui/events/device_data_m...
ui/events/device_data_manager.cc:129: std::equal(devices.begin(), devices.end(),
On 2014/10/04 00:35:12, flackr wrote:
> Did you mean !std::equal(...?

Done.

https://codereview.chromium.org/618283003/diff/120001/ui/events/device_data_m...
ui/events/device_data_manager.cc:130: touchscreen_devices_.begin(),
TouchscreenEquals)) {
On 2014/10/04 00:35:12, flackr wrote:
> nit: indent 4 more (to show it's wrapped from std::equal(...
> Or line up with devices.begin() after the '(' above.

Done.

https://codereview.chromium.org/618283003/diff/120001/ui/events/device_data_m...
ui/events/device_data_manager.cc:131: touchscreen_devices_ = devices;
On 2014/10/04 00:35:12, flackr wrote:
> Better yet, early return if the device list looks the same rather than
wrapping
> the contents of the function in an if.

Done.

https://codereview.chromium.org/618283003/diff/120001/ui/events/device_data_m...
ui/events/device_data_manager.cc:141: std::equal(devices.begin(), devices.end(),
On 2014/10/04 00:35:12, flackr wrote:
> ditto, !std::equal?

Done.

https://codereview.chromium.org/618283003/diff/120001/ui/events/device_data_m...
ui/events/device_data_manager.cc:142: keyboard_devices_.begin(),
KeyboardEquals)) {
On 2014/10/04 00:35:12, flackr wrote:
> ditto, indent 4 more or to just past '(' above.

Done.

https://codereview.chromium.org/618283003/diff/120001/ui/events/device_data_m...
File ui/events/device_data_manager.h (right):

https://codereview.chromium.org/618283003/diff/120001/ui/events/device_data_m...
ui/events/device_data_manager.h:71: const std::vector<KeyboardDevice>& devices)
OVERRIDE;
On 2014/10/04 00:51:31, flackr wrote:
> Can you override this in DeviceDataManagerX11 to remove blocked devices? This
> way we don't need to consider maximize mode a special case.

Done.

https://codereview.chromium.org/618283003/diff/120001/ui/events/input_device.h
File ui/events/input_device.h (right):

https://codereview.chromium.org/618283003/diff/120001/ui/events/input_device....
ui/events/input_device.h:17: UNKNOWN,  // Device that may or may not be an
external device.
On 2014/10/04 00:35:12, flackr wrote:
> nit: align comment.

Done.

https://codereview.chromium.org/618283003/diff/120001/ui/events/x/hotplug_eve...
File ui/events/x/hotplug_event_handler_x11.cc (right):

https://codereview.chromium.org/618283003/diff/120001/ui/events/x/hotplug_eve...
ui/events/x/hotplug_event_handler_x11.cc:35: // Filters out all devices that
that are not known keyboards. This may return
On 2014/10/04 00:35:12, flackr wrote:
> nit: This function only determines devices that are not known keyboards for
one
> device, not all devices. Perhaps update comment to:
> 
> // Returns true if |name| is the name of a known keyboard device. Note, this
may
> return false negatives.
> 
> See also style guide on descriptive function comments:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm...

Done.

https://codereview.chromium.org/618283003/diff/120001/ui/events/x/hotplug_eve...
ui/events/x/hotplug_event_handler_x11.cc:43: // Filters out all devices that
that are XTEST keyboards.
On 2014/10/04 00:35:12, flackr wrote:
> Ditto.

Done.

https://codereview.chromium.org/618283003/diff/120001/ui/events/x/hotplug_eve...
ui/events/x/hotplug_event_handler_x11.cc:153: continue;
On 2014/10/04 00:35:12, flackr wrote:
> No need for else if after a continue. Perhaps put an empty line afterwards to
> clearly separate from cases where device is added.

Done.

[rsadam, 2014-10-04 19:19:28]

Actually this approach may not necessarily work, since we do not update
observers when we block a device.

I.e 2 devices present at start
Maximize mode enabled, blocks one device
Listeners still think there are 2 devices plugged in.

Will fix this.

[flackr, 2014-10-04 19:29:14]

On 2014/10/04 19:19:28, rsadam wrote:
> Actually this approach may not necessarily work, since we do not update
> observers when we block a device.
> 
> I.e 2 devices present at start
> Maximize mode enabled, blocks one device
> Listeners still think there are 2 devices plugged in.
> 
> Will fix this.

Just need to make blocking and unblocking trigger an update of the device list
:-).

[rsadam, 2014-10-04 20:06:30]

> Just need to make blocking and unblocking trigger an update of the device list
> :-).

Yeah I was trying to figure out how to get the list of devices - completely
derped on the fact that I could simply call touch_screen_devices() /
keyboard_devices().

[pkotwicz, 2014-10-05 18:53:01]

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

[pkotwicz, 2014-10-05 18:53:02]

A couple of nits. Please make sure that this compiles under ozone as well.

https://codereview.chromium.org/618283003/diff/160001/ash/display/display_cha...
File ash/display/display_change_observer_chromeos.h (right):

https://codereview.chromium.org/618283003/diff/160001/ash/display/display_cha...
ash/display/display_change_observer_chromeos.h:52: virtual void
OnKeyboardDeviceConfigurationChanged() OVERRIDE;
Nit: Please rebase. We recently changed the OVERRIDE macro from uppercase to
lowercase

https://codereview.chromium.org/618283003/diff/160001/ui/events/keyboard_devi...
File ui/events/keyboard_device.cc (right):

https://codereview.chromium.org/618283003/diff/160001/ui/events/keyboard_devi...
ui/events/keyboard_device.cc:14: const std::string& name) : InputDevice(id,
type, name) {
Nit: put each parameter on a separate line

https://codereview.chromium.org/618283003/diff/160001/ui/events/touchscreen_d...
File ui/events/touchscreen_device.h (right):

https://codereview.chromium.org/618283003/diff/160001/ui/events/touchscreen_d...
ui/events/touchscreen_device.h:19: const gfx::Size& size);
Nit: According to the Chromium style guide, if all of the parameters of a
function declaration cannot fit on one line, they should each go on a separate
line.
http://www.chromium.org/developers/coding-style

https://codereview.chromium.org/618283003/diff/160001/ui/events/x/device_data...
File ui/events/x/device_data_manager_x11.cc (right):

https://codereview.chromium.org/618283003/diff/160001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:716: const ui::KeyboardDevice keyboard,
DeviceDataManagerX11* manager) {
As a general rule we either pass in a const reference or a parameter which is
neither a reference nor const.

|keyboard| here is passed by copy. So it does not really matter if
ShouldIgnoreKeyboard() mutates it

[rsadam, 2014-10-06 15:40:17]

https://codereview.chromium.org/618283003/diff/160001/ash/display/display_cha...
File ash/display/display_change_observer_chromeos.h (right):

https://codereview.chromium.org/618283003/diff/160001/ash/display/display_cha...
ash/display/display_change_observer_chromeos.h:52: virtual void
OnKeyboardDeviceConfigurationChanged() OVERRIDE;
On 2014/10/05 18:53:02, pkotwicz wrote:
> Nit: Please rebase. We recently changed the OVERRIDE macro from uppercase to
> lowercase

Done.

https://codereview.chromium.org/618283003/diff/160001/ui/events/keyboard_devi...
File ui/events/keyboard_device.cc (right):

https://codereview.chromium.org/618283003/diff/160001/ui/events/keyboard_devi...
ui/events/keyboard_device.cc:14: const std::string& name) : InputDevice(id,
type, name) {
On 2014/10/05 18:53:02, pkotwicz wrote:
> Nit: put each parameter on a separate line

Done.

https://codereview.chromium.org/618283003/diff/160001/ui/events/touchscreen_d...
File ui/events/touchscreen_device.h (right):

https://codereview.chromium.org/618283003/diff/160001/ui/events/touchscreen_d...
ui/events/touchscreen_device.h:19: const gfx::Size& size);
On 2014/10/05 18:53:02, pkotwicz wrote:
> Nit: According to the Chromium style guide, if all of the parameters of a
> function declaration cannot fit on one line, they should each go on a separate
> line.
> http://www.chromium.org/developers/coding-style

Done.

https://codereview.chromium.org/618283003/diff/160001/ui/events/x/device_data...
File ui/events/x/device_data_manager_x11.cc (right):

https://codereview.chromium.org/618283003/diff/160001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:716: const ui::KeyboardDevice keyboard,
DeviceDataManagerX11* manager) {
On 2014/10/05 18:53:02, pkotwicz wrote:
> As a general rule we either pass in a const reference or a parameter which is
> neither a reference nor const.
> 
> |keyboard| here is passed by copy. So it does not really matter if
> ShouldIgnoreKeyboard() mutates it

Done.

[kpschoedel, 2014-10-06 17:16:20]

https://codereview.chromium.org/618283003/diff/160001/ui/events/x/hotplug_eve...
File ui/events/x/hotplug_event_handler_x11.cc (right):

https://codereview.chromium.org/618283003/diff/160001/ui/events/x/hotplug_eve...
ui/events/x/hotplug_event_handler_x11.cc:33: const char kInternalKeyboardName[]
= "AT Translated Set 2 keyboard";
Is this known to be true for all boards?

[rsadam, 2014-10-06 17:29:08]

On 2014/10/06 17:16:20, kpschoedel wrote:
>
https://codereview.chromium.org/618283003/diff/160001/ui/events/x/hotplug_eve...
> File ui/events/x/hotplug_event_handler_x11.cc (right):
> 
>
https://codereview.chromium.org/618283003/diff/160001/ui/events/x/hotplug_eve...
> ui/events/x/hotplug_event_handler_x11.cc:33: const char
kInternalKeyboardName[]
> = "AT Translated Set 2 keyboard";
> Is this known to be true for all boards?

I believe this is standard and therefore true for all boards. I've verified it
manually on 3 devices and we take a similar approach for detecting whether the
keyboard is internal in other areas of the code as well:
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/maximize_mo...

Are there any other sanity checks I should take?

[rsadam, 2014-10-06 17:31:45]

Patchset #11 (id:200001) has been deleted

[rsadam, 2014-10-06 17:31:56]

Patchset #11 (id:220001) has been deleted

[kpschoedel, 2014-10-06 17:52:06]

On 2014/10/06 17:29:08, rsadam wrote: 
> I believe this is standard and therefore true for all boards. I've verified it
> manually on 3 devices and we take a similar approach for detecting whether the
> keyboard is internal in other areas of the code as well:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/maximize_mo...
> 
> Are there any other sanity checks I should take?

Nothing I know of. I'm just concerned that nobody seems to know for certain, so
it's not clear that future hardware will keep doing it... PS/2 codes feel a bit
dated.

[flackr, 2014-10-06 18:31:11]

https://codereview.chromium.org/618283003/diff/160001/ui/events/x/hotplug_eve...
File ui/events/x/hotplug_event_handler_x11.cc (right):

https://codereview.chromium.org/618283003/diff/160001/ui/events/x/hotplug_eve...
ui/events/x/hotplug_event_handler_x11.cc:33: const char kInternalKeyboardName[]
= "AT Translated Set 2 keyboard";
On 2014/10/06 17:16:20, kpschoedel wrote:
> Is this known to be true for all boards?

We may have to revisit this later but I believe all current boards have this
device name (from my sampling of 4 or 5 devices). Maybe add a TODO with bug to
have a better way to detect internal keyboards. We could add a udev rule to
create a symlink for the internal keyboard device when it's added and then this
can easily be made board specific without adding extra code to chromium.

https://codereview.chromium.org/618283003/diff/240001/ui/events/device_data_m...
File ui/events/device_data_manager.cc (right):

https://codereview.chromium.org/618283003/diff/240001/ui/events/device_data_m...
ui/events/device_data_manager.cc:26: }
Can this not be done with a single InputDeviceEquals(const ui::InputDevice& a,
const ui::InputDevice& b)?

https://codereview.chromium.org/618283003/diff/240001/ui/events/input_device.h
File ui/events/input_device.h (right):

https://codereview.chromium.org/618283003/diff/240001/ui/events/input_device....
ui/events/input_device.h:20: // Represents a Keyboard device state.
nit: this is the base class, not a keyboard specifically.

https://codereview.chromium.org/618283003/diff/240001/ui/events/x/device_data...
File ui/events/x/device_data_manager_x11.cc (right):

https://codereview.chromium.org/618283003/diff/240001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:716: ui::KeyboardDevice keyboard,
DeviceDataManagerX11* manager) {
Since this is only used once internally, maybe make this an anonymous method and
pass the blocked_devices_ bitset instead?

[rsadam, 2014-10-06 19:38:01]

Created crbug.com/420728 and added a TODO to follow up on internal keyboards.

https://codereview.chromium.org/618283003/diff/240001/ui/events/device_data_m...
File ui/events/device_data_manager.cc (right):

https://codereview.chromium.org/618283003/diff/240001/ui/events/device_data_m...
ui/events/device_data_manager.cc:26: }
On 2014/10/06 18:31:11, flackr wrote:
> Can this not be done with a single InputDeviceEquals(const ui::InputDevice& a,
> const ui::InputDevice& b)?

Done.

https://codereview.chromium.org/618283003/diff/240001/ui/events/x/device_data...
File ui/events/x/device_data_manager_x11.cc (right):

https://codereview.chromium.org/618283003/diff/240001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:716: ui::KeyboardDevice keyboard,
DeviceDataManagerX11* manager) {
On 2014/10/06 18:31:11, flackr wrote:
> Since this is only used once internally, maybe make this an anonymous method
and
> pass the blocked_devices_ bitset instead?

Done.

[flackr, 2014-10-06 21:14:53]

https://codereview.chromium.org/618283003/diff/260001/ui/events/events.gyp
File ui/events/events.gyp (right):

https://codereview.chromium.org/618283003/diff/260001/ui/events/events.gyp#ne...
ui/events/events.gyp:57: 'keyboard_device.h',
You probably also need to update BUILD.gn. You should be able to build chrome
with gn I think: https://code.google.com/p/chromium/wiki/GNQuickStart

https://codereview.chromium.org/618283003/diff/260001/ui/events/x/device_data...
File ui/events/x/device_data_manager_x11.cc (right):

https://codereview.chromium.org/618283003/diff/260001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:732: blocked_devices_)),
nit: Looks like this can be pushed up to the previous line.

[rsadam, 2014-10-07 05:25:05]

https://codereview.chromium.org/618283003/diff/260001/ui/events/x/device_data...
File ui/events/x/device_data_manager_x11.cc (right):

https://codereview.chromium.org/618283003/diff/260001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:732: blocked_devices_)),
On 2014/10/06 21:14:53, flackr wrote:
> nit: Looks like this can be pushed up to the previous line.

Done.

[rsadam, 2014-10-07 05:25:38]

Done! Verified it compiles with gn.

https://codereview.chromium.org/618283003/diff/260001/ui/events/events.gyp
File ui/events/events.gyp (right):

https://codereview.chromium.org/618283003/diff/260001/ui/events/events.gyp#ne...
ui/events/events.gyp:57: 'keyboard_device.h',
On 2014/10/06 21:14:53, flackr wrote:
> You probably also need to update BUILD.gn. You should be able to build chrome
> with gn I think: https://code.google.com/p/chromium/wiki/GNQuickStart

Done.

[flackr, 2014-10-07 13:30:15]

Looks good, was just thinking about what test coverage would be important and it
looks like it should be easy to test updating the keyboard device list and
notifying observers.

https://codereview.chromium.org/618283003/diff/280001/ui/events/x/device_data...
File ui/events/x/device_data_manager_x11.cc (right):

https://codereview.chromium.org/618283003/diff/280001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:702:
OnKeyboardDevicesUpdated(keyboard_devices());
Can you add a test to ensure enable/disable correctly update the device list and
notify an observer?

[rsadam, 2014-10-07 18:35:25]

Test caught a bug in the re-enable phase. We need to keep track of the device
metadata in order to restore the device. For now this is only keyboards - have
added a TODO to make this more generic.

[rsadam, 2014-10-07 21:59:10]

Patchset #14 (id:300001) has been deleted

[rsadam, 2014-10-07 22:00:35]

rsadam@chromium.org changed reviewers:
- sadrul@chromium.org

[rsadam, 2014-10-07 22:00:36]

-sadrul as a reviewer since he's on vacation for the next 3 weeks.

https://codereview.chromium.org/618283003/diff/280001/ui/events/x/device_data...
File ui/events/x/device_data_manager_x11.cc (right):

https://codereview.chromium.org/618283003/diff/280001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:702:
OnKeyboardDevicesUpdated(keyboard_devices());
On 2014/10/07 13:30:15, flackr wrote:
> Can you add a test to ensure enable/disable correctly update the device list
and
> notify an observer?

Done.

[rsadam, 2014-10-07 22:00:58]

Patchset #14 (id:320001) has been deleted

[rsadam, 2014-10-08 19:03:12]

Patchset #14 (id:340001) has been deleted

[rsadam, 2014-10-08 21:12:55]

On 2014/10/07 18:35:25, rsadam wrote:
> Test caught a bug in the re-enable phase. We need to keep track of the device
> metadata in order to restore the device. For now this is only keyboards - have
> added a TODO to make this more generic.

ping reviewers.

[flackr, 2014-10-08 21:58:11]

https://codereview.chromium.org/618283003/diff/360001/ui/events/x/device_data...
File ui/events/x/device_data_manager_x11.cc (right):

https://codereview.chromium.org/618283003/diff/360001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:705: keyboards.end(),
nit: Don't need to wrap this.

https://codereview.chromium.org/618283003/diff/360001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:721: devices.push_back(it->second);
Hmm, what if a device changes, or is removed while "blocked"? Then it seems this
cached list will be stale. The problem is of course in OnKeyboardDevicesUpdated
you may be getting called from line 711, 722 or you may be getting called from
the hotplug device manager. Maybe 711, 722 need to call straight into the base
class to just update the list so that 746 is always getting the full device
list.

https://codereview.chromium.org/618283003/diff/360001/ui/events/x/device_data...
File ui/events/x/device_data_manager_x11.h (right):

https://codereview.chromium.org/618283003/diff/360001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.h:311: std::map<unsigned int,
ui::KeyboardDevice, std::less<int>> blocked_keyboards_;
You need to specify the comparator for types where operator< is defined.
Also, you should store this as <int, KeyboardDevice> to match the int deviceid
in InputDevice

https://codereview.chromium.org/618283003/diff/360001/ui/events/x/device_data...
File ui/events/x/device_data_manager_x11_unittest.cc (right):

https://codereview.chromium.org/618283003/diff/360001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11_unittest.cc:94: EXPECT_EQ(expected_size,
devices.size());
EXPECT_EQ(1u, devices.size());

[rsadam, 2014-10-09 05:19:03]

https://codereview.chromium.org/618283003/diff/360001/ui/events/x/device_data...
File ui/events/x/device_data_manager_x11.cc (right):

https://codereview.chromium.org/618283003/diff/360001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:705: keyboards.end(),
On 2014/10/08 21:58:11, flackr wrote:
> nit: Don't need to wrap this.

Done.

https://codereview.chromium.org/618283003/diff/360001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:721: devices.push_back(it->second);
On 2014/10/08 21:58:11, flackr wrote:
> Hmm, what if a device changes, or is removed while "blocked"? Then it seems
this
> cached list will be stale. The problem is of course in
OnKeyboardDevicesUpdated
> you may be getting called from line 711, 722 or you may be getting called from
> the hotplug device manager. Maybe 711, 722 need to call straight into the base
> class to just update the list so that 746 is always getting the full device
> list.

Handled that case with a slight variation of what we discuss- 711 and 722 still
call the overridden method, except we update the blocked list at 746, and re-add
the cached device on enable device.

When a device is blocked, it will get filtered since it's in blocked_keyboards_.
When a device is unblocked, it will update the base class to include this
device.

https://codereview.chromium.org/618283003/diff/360001/ui/events/x/device_data...
File ui/events/x/device_data_manager_x11.h (right):

https://codereview.chromium.org/618283003/diff/360001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.h:311: std::map<unsigned int,
ui::KeyboardDevice, std::less<int>> blocked_keyboards_;
On 2014/10/08 21:58:11, flackr wrote:
> You need to specify the comparator for types where operator< is defined.
> Also, you should store this as <int, KeyboardDevice> to match the int deviceid
> in InputDevice

Done.

https://codereview.chromium.org/618283003/diff/360001/ui/events/x/device_data...
File ui/events/x/device_data_manager_x11_unittest.cc (right):

https://codereview.chromium.org/618283003/diff/360001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11_unittest.cc:94: EXPECT_EQ(expected_size,
devices.size());
On 2014/10/08 21:58:11, flackr wrote:
> EXPECT_EQ(1u, devices.size());

Done.

[kpschoedel, 2014-10-10 15:51:52]

On 2014/10/06 17:29:08, rsadam wrote:
> On 2014/10/06 17:16:20, kpschoedel wrote:
> >
>
https://codereview.chromium.org/618283003/diff/160001/ui/events/x/hotplug_eve...
> > File ui/events/x/hotplug_event_handler_x11.cc (right):
> > 
> >
>
https://codereview.chromium.org/618283003/diff/160001/ui/events/x/hotplug_eve...
> > ui/events/x/hotplug_event_handler_x11.cc:33: const char
> kInternalKeyboardName[]
> > = "AT Translated Set 2 keyboard";
> > Is this known to be true for all boards?
> 
> I believe this is standard and therefore true for all boards. [....]

Daisy's internal keyboard is named "cros-ec-i2c". There may be others…

[rsadam, 2014-10-10 16:06:44]

On 2014/10/10 15:51:52, kpschoedel wrote:
> On 2014/10/06 17:29:08, rsadam wrote:
> > On 2014/10/06 17:16:20, kpschoedel wrote:
> > >
> >
>
https://codereview.chromium.org/618283003/diff/160001/ui/events/x/hotplug_eve...
> > > File ui/events/x/hotplug_event_handler_x11.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/618283003/diff/160001/ui/events/x/hotplug_eve...
> > > ui/events/x/hotplug_event_handler_x11.cc:33: const char
> > kInternalKeyboardName[]
> > > = "AT Translated Set 2 keyboard";
> > > Is this known to be true for all boards?
> > 
> > I believe this is standard and therefore true for all boards. [....]
> 
> Daisy's internal keyboard is named "cros-ec-i2c". There may be others…

There seem to be 2 others in total. From chromiumos 90-powerd-id.rules:
SYSTEM=="input", ENV{ID_INPUT_KEYBOARD}=="1",    DRIVERS=="atkbd",       
ENV{POWERD_ROLE}="internal_keyboard"
SUBSYSTEM=="input", ENV{ID_INPUT_KEYBOARD}=="1",    DRIVERS=="cros-ec-keyb",
ENV{POWERD_ROLE}="internal_keyboard"

Would it suffice to filter on "atkbd" and "cros-ec"?

[flackr, 2014-10-10 17:49:31]

https://codereview.chromium.org/618283003/diff/380001/ui/events/x/device_data...
File ui/events/x/device_data_manager_x11.cc (right):

https://codereview.chromium.org/618283003/diff/380001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:694: LOG(ERROR) << "Disable the device:"
<< deviceid;
Remove this, not an error.

https://codereview.chromium.org/618283003/diff/380001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:704: id,
you can also just write blocked_keyboards_[id] = *it;

https://codereview.chromium.org/618283003/diff/380001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:706: OnKeyboardDevicesUpdated(keyboards);
OnKeyboardDevices assumes it is getting the full list of keyboard devices.
Calling it directly with an already filtered list can result in it falsely
unblocking keyboards which were previously removed.

I.e.
DisableDevice(1);
DisableDevice(2); // Calls OnKeyboardDevicesUpdated which thinks device 1
doesn't exist anymore because it's not in keyboards

Same for line 720. This is why I proposed calling into the parent class only.

https://codereview.chromium.org/618283003/diff/380001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:759:
blocked_keyboards_.erase(blocked_iter++);
This is not correct. std::vector::erase invalidates iterators pointing to the
erased position and beyond. In particular you will probably at least skip the
next keyboard if not overstep the end of the list. You need to use the returned
value to step to the next position:
blocked_iter = blocked_keyboards_.erase(blocked_iter);

[rsadam, 2014-10-14 17:32:19]

In the udev rules, atkbd driver corresponds to AT Translated Set 2 Keyboard, and
cros-ec-keyb to cros-ec-*. Given that only these 2 are listed, I think it's
pretty safe to assume that these are the only internal keyboards for now.

https://codereview.chromium.org/618283003/diff/380001/ui/events/x/device_data...
File ui/events/x/device_data_manager_x11.cc (right):

https://codereview.chromium.org/618283003/diff/380001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:694: LOG(ERROR) << "Disable the device:"
<< deviceid;
On 2014/10/10 17:49:31, flackr wrote:
> Remove this, not an error.

Oops. Fixed.

https://codereview.chromium.org/618283003/diff/380001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:704: id,
On 2014/10/10 17:49:31, flackr wrote:
> you can also just write blocked_keyboards_[id] = *it;

As discussed offline, would require copy constructor for the struct so sticking
with original implementation.

https://codereview.chromium.org/618283003/diff/380001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:706: OnKeyboardDevicesUpdated(keyboards);
On 2014/10/10 17:49:30, flackr wrote:
> OnKeyboardDevices assumes it is getting the full list of keyboard devices.
> Calling it directly with an already filtered list can result in it falsely
> unblocking keyboards which were previously removed.
> 
> I.e.
> DisableDevice(1);
> DisableDevice(2); // Calls OnKeyboardDevicesUpdated which thinks device 1
> doesn't exist anymore because it's not in keyboards
> 
> Same for line 720. This is why I proposed calling into the parent class only.

Done.

https://codereview.chromium.org/618283003/diff/380001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:759:
blocked_keyboards_.erase(blocked_iter++);
On 2014/10/10 17:49:31, flackr wrote:
> This is not correct. std::vector::erase invalidates iterators pointing to the
> erased position and beyond. In particular you will probably at least skip the
> next keyboard if not overstep the end of the list. You need to use the
returned
> value to step to the next position:
> blocked_iter = blocked_keyboards_.erase(blocked_iter);

blocked_iter is a std::map::iter, and so as we discussed offline, the existing
approach should be fine.

[flackr, 2014-10-14 17:41:36]

https://codereview.chromium.org/618283003/diff/380001/ui/events/x/device_data...
File ui/events/x/device_data_manager_x11.cc (right):

https://codereview.chromium.org/618283003/diff/380001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:704: id,
On 2014/10/14 17:32:19, rsadam wrote:
> On 2014/10/10 17:49:31, flackr wrote:
> > you can also just write blocked_keyboards_[id] = *it;
> 
> As discussed offline, would require copy constructor for the struct so
sticking
> with original implementation.

Acknowledged, no need to wrap id, *it.

https://codereview.chromium.org/618283003/diff/380001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:706: OnKeyboardDevicesUpdated(keyboards);
On 2014/10/14 17:32:18, rsadam wrote:
> On 2014/10/10 17:49:30, flackr wrote:
> > OnKeyboardDevices assumes it is getting the full list of keyboard devices.
> > Calling it directly with an already filtered list can result in it falsely
> > unblocking keyboards which were previously removed.
> > 
> > I.e.
> > DisableDevice(1);
> > DisableDevice(2); // Calls OnKeyboardDevicesUpdated which thinks device 1
> > doesn't exist anymore because it's not in keyboards
> > 
> > Same for line 720. This is why I proposed calling into the parent class
only.
> 
> Done.

Test?

https://codereview.chromium.org/618283003/diff/380001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:759:
blocked_keyboards_.erase(blocked_iter++);
On 2014/10/14 17:32:19, rsadam wrote:
> On 2014/10/10 17:49:31, flackr wrote:
> > This is not correct. std::vector::erase invalidates iterators pointing to
the
> > erased position and beyond. In particular you will probably at least skip
the
> > next keyboard if not overstep the end of the list. You need to use the
> returned
> > value to step to the next position:
> > blocked_iter = blocked_keyboards_.erase(blocked_iter);
> 
> blocked_iter is a std::map::iter, and so as we discussed offline, the existing
> approach should be fine.

Acknowledged.

https://codereview.chromium.org/618283003/diff/460001/ui/events/x/hotplug_eve...
File ui/events/x/hotplug_event_handler_x11.cc (right):

https://codereview.chromium.org/618283003/diff/460001/ui/events/x/hotplug_eve...
ui/events/x/hotplug_event_handler_x11.cc:35: "cros-ec"
Since "cros-ec" is a prefix but "AT Translated Set 2 keyboard" is not can you
have two consts?

https://codereview.chromium.org/618283003/diff/460001/ui/events/x/hotplug_eve...
ui/events/x/hotplug_event_handler_x11.cc:52: if
(name.find(kInternalKeyboardPrefixes[i]) != std::string::npos)
Only return true if it matches at position 0 (i.e. start of string, an actual
prefix).

[rsadam, 2014-10-14 17:55:02]

https://codereview.chromium.org/618283003/diff/380001/ui/events/x/device_data...
File ui/events/x/device_data_manager_x11.cc (right):

https://codereview.chromium.org/618283003/diff/380001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:706: OnKeyboardDevicesUpdated(keyboards);
On 2014/10/14 17:41:35, flackr wrote:
> On 2014/10/14 17:32:18, rsadam wrote:
> > On 2014/10/10 17:49:30, flackr wrote:
> > > OnKeyboardDevices assumes it is getting the full list of keyboard devices.
> > > Calling it directly with an already filtered list can result in it falsely
> > > unblocking keyboards which were previously removed.
> > > 
> > > I.e.
> > > DisableDevice(1);
> > > DisableDevice(2); // Calls OnKeyboardDevicesUpdated which thinks device 1
> > > doesn't exist anymore because it's not in keyboards
> > > 
> > > Same for line 720. This is why I proposed calling into the parent class
> only.
> > 
> > Done.
> 
> Test?

Done.

https://codereview.chromium.org/618283003/diff/460001/ui/events/x/hotplug_eve...
File ui/events/x/hotplug_event_handler_x11.cc (right):

https://codereview.chromium.org/618283003/diff/460001/ui/events/x/hotplug_eve...
ui/events/x/hotplug_event_handler_x11.cc:35: "cros-ec"
On 2014/10/14 17:41:36, flackr wrote:
> Since "cros-ec" is a prefix but "AT Translated Set 2 keyboard" is not can you
> have two consts?

Done.

https://codereview.chromium.org/618283003/diff/460001/ui/events/x/hotplug_eve...
ui/events/x/hotplug_event_handler_x11.cc:52: if
(name.find(kInternalKeyboardPrefixes[i]) != std::string::npos)
On 2014/10/14 17:41:36, flackr wrote:
> Only return true if it matches at position 0 (i.e. start of string, an actual
> prefix).

Done.

[flackr, 2014-10-14 19:34:27]

lgtm with nits.

https://codereview.chromium.org/618283003/diff/480001/ui/events/x/hotplug_eve...
File ui/events/x/hotplug_event_handler_x11.cc (right):

https://codereview.chromium.org/618283003/diff/480001/ui/events/x/hotplug_eve...
ui/events/x/hotplug_event_handler_x11.cc:33: const char* kInternalKeyboardName =
"AT Translated Set 2 keyboard";
Let's name these constants to match the modules perhaps?
kThisKeyboardsModuleNameName and kCrosEcKeyboardPrefix?

https://codereview.chromium.org/618283003/diff/480001/ui/events/x/hotplug_eve...
ui/events/x/hotplug_event_handler_x11.cc:37: const char*
kInternalKeyboardPrefixes[] = {
nit: Don't use array for a single item.

https://codereview.chromium.org/618283003/diff/480001/ui/events/x/hotplug_eve...
ui/events/x/hotplug_event_handler_x11.cc:57: if
(name.find(kInternalKeyboardPrefixes[i]) == 0)
nit: Doesn't this need to be 0u (since string::find returns a size_t)?
Though you should probably use std::string::compare and just look at the first
strlen(kInternalKeyboardPrefixes[i]) characters to avoid searching the rest of
the string since we only care about if it's at the start.

[rsadam, 2014-10-14 22:24:15]

Addressed nits!

https://codereview.chromium.org/618283003/diff/480001/ui/events/x/hotplug_eve...
File ui/events/x/hotplug_event_handler_x11.cc (right):

https://codereview.chromium.org/618283003/diff/480001/ui/events/x/hotplug_eve...
ui/events/x/hotplug_event_handler_x11.cc:33: const char* kInternalKeyboardName =
"AT Translated Set 2 keyboard";
On 2014/10/14 19:34:26, flackr wrote:
> Let's name these constants to match the modules perhaps?
> kThisKeyboardsModuleNameName and kCrosEcKeyboardPrefix?

Done.

https://codereview.chromium.org/618283003/diff/480001/ui/events/x/hotplug_eve...
ui/events/x/hotplug_event_handler_x11.cc:37: const char*
kInternalKeyboardPrefixes[] = {
On 2014/10/14 19:34:26, flackr wrote:
> nit: Don't use array for a single item.

Done.

https://codereview.chromium.org/618283003/diff/480001/ui/events/x/hotplug_eve...
ui/events/x/hotplug_event_handler_x11.cc:57: if
(name.find(kInternalKeyboardPrefixes[i]) == 0)
On 2014/10/14 19:34:26, flackr wrote:
> nit: Doesn't this need to be 0u (since string::find returns a size_t)?
> Though you should probably use std::string::compare and just look at the first
> strlen(kInternalKeyboardPrefixes[i]) characters to avoid searching the rest of
> the string since we only care about if it's at the start.

Done.

[flackr, 2014-10-15 00:11:18]

https://codereview.chromium.org/618283003/diff/500001/ui/events/x/hotplug_eve...
File ui/events/x/hotplug_event_handler_x11.cc (right):

https://codereview.chromium.org/618283003/diff/500001/ui/events/x/hotplug_eve...
ui/events/x/hotplug_event_handler_x11.cc:54: 0u, strlen(kCrosEcKeyboardPrefix),
kCrosEcKeyboardPrefix) == 0u;
while find returns a size_t, compare returns an int, just == 0

[rsadam, 2014-10-20 15:36:50]

rsadam@chromium.org changed reviewers:
+ oshima@chromium.org

[rsadam, 2014-10-20 15:36:53]

ping kpshoedel.

+oshima for OWNERS

https://codereview.chromium.org/618283003/diff/500001/ui/events/x/hotplug_eve...
File ui/events/x/hotplug_event_handler_x11.cc (right):

https://codereview.chromium.org/618283003/diff/500001/ui/events/x/hotplug_eve...
ui/events/x/hotplug_event_handler_x11.cc:54: 0u, strlen(kCrosEcKeyboardPrefix),
kCrosEcKeyboardPrefix) == 0u;
On 2014/10/15 00:11:18, flackr (OOO until Oct 18) wrote:
> while find returns a size_t, compare returns an int, just == 0

Done.

[kpschoedel, 2014-10-20 16:12:23]

On 2014/10/20 15:36:53, rsadam wrote:
> ping kpshoedel.

Non-owning LGTM.

[rsadam, 2014-10-20 16:29:32]

Clang doesn't seem to like initing empty vectors with {}, changed to
std::vector<KeyboardDevice>();

[rsadam, 2014-10-20 16:34:48]

rsadam@chromium.org changed reviewers:
+ sky@chromium.org

[rsadam, 2014-10-20 16:34:50]

Misread OWNERS file.

+oshima for ash/* 
+sky for ui/*

[oshima, 2014-10-20 19:38:17]

ash/ lgtm

[sky, 2014-10-20 22:18:44]

https://codereview.chromium.org/618283003/diff/560001/ui/events/device_data_m...
File ui/events/device_data_manager.cc (right):

https://codereview.chromium.org/618283003/diff/560001/ui/events/device_data_m...
ui/events/device_data_manager.cc:19: const ui::InputDevice& b) {
Make sure you run git cl format (this looks weird, not sure if git cl produced
this).

https://codereview.chromium.org/618283003/diff/560001/ui/events/device_data_m...
ui/events/device_data_manager.cc:123: if (devices.size() ==
touchscreen_devices_.size() &&
Doesn't std::equals handle the length check?

https://codereview.chromium.org/618283003/diff/560001/ui/events/device_data_m...
ui/events/device_data_manager.cc:130: observers_,
same comment about git cl format here and 143, spacing looks off.

https://codereview.chromium.org/618283003/diff/560001/ui/events/device_data_m...
ui/events/device_data_manager.cc:136: if (devices.size() ==
keyboard_devices_.size() &&
Same comment here about equals.

https://codereview.chromium.org/618283003/diff/560001/ui/events/device_data_m...
File ui/events/device_data_manager.h (right):

https://codereview.chromium.org/618283003/diff/560001/ui/events/device_data_m...
ui/events/device_data_manager.h:34: static const int kMaxDeviceNum = 128;
See style guide, this should be before destructor.

https://codereview.chromium.org/618283003/diff/560001/ui/events/device_hotplu...
File ui/events/device_hotplug_event_observer.h (right):

https://codereview.chromium.org/618283003/diff/560001/ui/events/device_hotplu...
ui/events/device_hotplug_event_observer.h:11: #include
"ui/events/keyboard_device.h"
Can't you forward declare both KeyboardDevice and TouchscreenDevice?

https://codereview.chromium.org/618283003/diff/560001/ui/events/device_hotplu...
ui/events/device_hotplug_event_observer.h:22: // devices.
Document the set of devices may not have actually changed.

https://codereview.chromium.org/618283003/diff/560001/ui/events/input_device.cc
File ui/events/input_device.cc (right):

https://codereview.chromium.org/618283003/diff/560001/ui/events/input_device....
ui/events/input_device.cc:15: int id,
It's not readily obvious that the id is unique between all input types. But that
appears to be the case. Please document it.

https://codereview.chromium.org/618283003/diff/560001/ui/events/input_device.h
File ui/events/input_device.h (right):

https://codereview.chromium.org/618283003/diff/560001/ui/events/input_device....
ui/events/input_device.h:15: INTERNAL,  // Internal device.
Prefix with enum name, eg INPUT_DEVICE_INTERNAL.

https://codereview.chromium.org/618283003/diff/560001/ui/events/input_device....
ui/events/input_device.h:15: INTERNAL,  // Internal device.
It's not clear what internal or external mean in this context.

https://codereview.chromium.org/618283003/diff/560001/ui/events/input_device....
ui/events/input_device.h:25: 
Don't you need a virtual destructor?

https://codereview.chromium.org/618283003/diff/560001/ui/events/keyboard_devi...
File ui/events/keyboard_device.h (right):

https://codereview.chromium.org/618283003/diff/560001/ui/events/keyboard_devi...
ui/events/keyboard_device.h:17: KeyboardDevice(int id, InputDeviceType type,
const std::string& name);
AFAICT you don't need this class. Is there a reason to have it?

https://codereview.chromium.org/618283003/diff/560001/ui/events/ozone/evdev/e...
File ui/events/ozone/evdev/event_factory_evdev.cc (right):

https://codereview.chromium.org/618283003/diff/560001/ui/events/ozone/evdev/e...
ui/events/ozone/evdev/event_factory_evdev.cc:278: "", /* Device name */
"" -> std::string()

https://codereview.chromium.org/618283003/diff/560001/ui/events/x/device_data...
File ui/events/x/device_data_manager_x11.cc (right):

https://codereview.chromium.org/618283003/diff/560001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:114: bool HasId(ui::KeyboardDevice
keyboard, int id) {
Name this better, eg KeybardDeviceHasId. Also, can this take a const
ui::KeyboardDevice?

https://codereview.chromium.org/618283003/diff/560001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:695: int id = static_cast<int>(deviceid);
Why the static cast here? Shouldn't the types match?

https://codereview.chromium.org/618283003/diff/560001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:755: unsigned int id = static_cast<int>
((*blocked_iter).first);
Same comment about cast (and you've got an extra space after >.

https://codereview.chromium.org/618283003/diff/560001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:756: blocked_devices_.set(id, false);
Why do you need to persist this?

https://codereview.chromium.org/618283003/diff/560001/ui/events/x/hotplug_eve...
File ui/events/x/hotplug_event_handler_x11.cc (right):

https://codereview.chromium.org/618283003/diff/560001/ui/events/x/hotplug_eve...
ui/events/x/hotplug_event_handler_x11.cc:42: std::transform(lower.begin(),
lower.end(), lower.begin(), ::tolower);
Are you guaranteed to always get ascii? Seems safer to use base versions.

https://codereview.chromium.org/618283003/diff/560001/ui/events/x/hotplug_eve...
ui/events/x/hotplug_event_handler_x11.cc:166: InputDeviceType type;
nit: move after conditional.

[kpschoedel, 2014-10-20 22:29:01]

https://codereview.chromium.org/618283003/diff/560001/ui/events/keyboard_devi...
File ui/events/keyboard_device.h (right):

https://codereview.chromium.org/618283003/diff/560001/ui/events/keyboard_devi...
ui/events/keyboard_device.h:17: KeyboardDevice(int id, InputDeviceType type,
const std::string& name);
On 2014/10/20 22:18:44, sky wrote:
> AFAICT you don't need this class. Is there a reason to have it?

chromeos::EventRewriter currently maintains its own independent analog of
DeviceDataManager that I'd like to consolidate into here after rsadam@ finishes
this.

[rsadam, 2014-10-21 16:51:13]

Patchset #23 (id:580001) has been deleted

[rsadam, 2014-10-21 16:51:27]

Patchset #23 (id:600001) has been deleted

[rsadam, 2014-10-21 16:53:52]

https://codereview.chromium.org/618283003/diff/240001/ui/events/input_device.h
File ui/events/input_device.h (right):

https://codereview.chromium.org/618283003/diff/240001/ui/events/input_device....
ui/events/input_device.h:20: // Represents a Keyboard device state.
On 2014/10/06 18:31:11, flackr wrote:
> nit: this is the base class, not a keyboard specifically.

Done.

https://codereview.chromium.org/618283003/diff/560001/ui/events/device_data_m...
File ui/events/device_data_manager.cc (right):

https://codereview.chromium.org/618283003/diff/560001/ui/events/device_data_m...
ui/events/device_data_manager.cc:19: const ui::InputDevice& b) {
On 2014/10/20 22:18:43, sky wrote:
> Make sure you run git cl format (this looks weird, not sure if git cl produced
> this).

Done.

https://codereview.chromium.org/618283003/diff/560001/ui/events/device_data_m...
ui/events/device_data_manager.cc:123: if (devices.size() ==
touchscreen_devices_.size() &&
On 2014/10/20 22:18:43, sky wrote:
> Doesn't std::equals handle the length check?

Removing this line causes a segfault. I did some digging
http://www.cplusplus.com/reference/algorithm/equal/ and based on the sample
algorithm there it does indeed seem that no length check is performed.

https://codereview.chromium.org/618283003/diff/560001/ui/events/device_data_m...
ui/events/device_data_manager.cc:130: observers_,
On 2014/10/20 22:18:43, sky wrote:
> same comment about git cl format here and 143, spacing looks off.

Done.

https://codereview.chromium.org/618283003/diff/560001/ui/events/device_data_m...
ui/events/device_data_manager.cc:136: if (devices.size() ==
keyboard_devices_.size() &&
On 2014/10/20 22:18:43, sky wrote:
> Same comment here about equals.

Please see above.

https://codereview.chromium.org/618283003/diff/560001/ui/events/device_data_m...
File ui/events/device_data_manager.h (right):

https://codereview.chromium.org/618283003/diff/560001/ui/events/device_data_m...
ui/events/device_data_manager.h:34: static const int kMaxDeviceNum = 128;
On 2014/10/20 22:18:43, sky wrote:
> See style guide, this should be before destructor.

Done.

https://codereview.chromium.org/618283003/diff/560001/ui/events/device_hotplu...
File ui/events/device_hotplug_event_observer.h (right):

https://codereview.chromium.org/618283003/diff/560001/ui/events/device_hotplu...
ui/events/device_hotplug_event_observer.h:11: #include
"ui/events/keyboard_device.h"
On 2014/10/20 22:18:43, sky wrote:
> Can't you forward declare both KeyboardDevice and TouchscreenDevice?

Done.

https://codereview.chromium.org/618283003/diff/560001/ui/events/device_hotplu...
ui/events/device_hotplug_event_observer.h:22: // devices.
On 2014/10/20 22:18:43, sky wrote:
> Document the set of devices may not have actually changed.

Done.

https://codereview.chromium.org/618283003/diff/560001/ui/events/ozone/evdev/e...
File ui/events/ozone/evdev/event_factory_evdev.cc (right):

https://codereview.chromium.org/618283003/diff/560001/ui/events/ozone/evdev/e...
ui/events/ozone/evdev/event_factory_evdev.cc:278: "", /* Device name */
On 2014/10/20 22:18:44, sky wrote:
> "" -> std::string()

Done.

https://codereview.chromium.org/618283003/diff/560001/ui/events/x/device_data...
File ui/events/x/device_data_manager_x11.cc (right):

https://codereview.chromium.org/618283003/diff/560001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:114: bool HasId(ui::KeyboardDevice
keyboard, int id) {
On 2014/10/20 22:18:44, sky wrote:
> Name this better, eg KeybardDeviceHasId. Also, can this take a const
> ui::KeyboardDevice?

Done.

https://codereview.chromium.org/618283003/diff/560001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:695: int id = static_cast<int>(deviceid);
On 2014/10/20 22:18:44, sky wrote:
> Why the static cast here? Shouldn't the types match?

The input types don't match - the existing logic in DeviceDataManagerX11 uses
unsigned ints, whereas the rest of the code depending on InputDevices expects
the device id to be of type int. Would it make sense to refactor
DeviceDataManager in a future CL to also use ints?

https://codereview.chromium.org/618283003/diff/560001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:755: unsigned int id = static_cast<int>
((*blocked_iter).first);
On 2014/10/20 22:18:44, sky wrote:
> Same comment about cast (and you've got an extra space after >.

Done.

https://codereview.chromium.org/618283003/diff/560001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:756: blocked_devices_.set(id, false);
On 2014/10/20 22:18:44, sky wrote:
> Why do you need to persist this?

This is the case that a device is blocked and then removed. If it is plugged
back in it should not still be blocked.

https://codereview.chromium.org/618283003/diff/560001/ui/events/x/hotplug_eve...
File ui/events/x/hotplug_event_handler_x11.cc (right):

https://codereview.chromium.org/618283003/diff/560001/ui/events/x/hotplug_eve...
ui/events/x/hotplug_event_handler_x11.cc:42: std::transform(lower.begin(),
lower.end(), lower.begin(), ::tolower);
On 2014/10/20 22:18:44, sky wrote:
> Are you guaranteed to always get ascii? Seems safer to use base versions.

Done.

https://codereview.chromium.org/618283003/diff/560001/ui/events/x/hotplug_eve...
ui/events/x/hotplug_event_handler_x11.cc:166: InputDeviceType type;
On 2014/10/20 22:18:44, sky wrote:
> nit: move after conditional.

Done.

[sky, 2014-10-21 18:15:32]

https://codereview.chromium.org/618283003/diff/560001/ui/events/x/device_data...
File ui/events/x/device_data_manager_x11.cc (right):

https://codereview.chromium.org/618283003/diff/560001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:695: int id = static_cast<int>(deviceid);
On 2014/10/21 16:53:52, rsadam wrote:
> On 2014/10/20 22:18:44, sky wrote:
> > Why the static cast here? Shouldn't the types match?
> 
> The input types don't match - the existing logic in DeviceDataManagerX11 uses
> unsigned ints, whereas the rest of the code depending on InputDevices expects
> the device id to be of type int. Would it make sense to refactor
> DeviceDataManager in a future CL to also use ints?

Is there a reason InputDevices needs to be int? Can it be unsigned int? I would
prefer you clean up types first, then do this one.

https://codereview.chromium.org/618283003/diff/560001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:756: blocked_devices_.set(id, false);
On 2014/10/21 16:53:52, rsadam wrote:
> On 2014/10/20 22:18:44, sky wrote:
> > Why do you need to persist this?
> 
> This is the case that a device is blocked and then removed. If it is plugged
> back in it should not still be blocked.

I you guaranteed that the same device gets the same id? Please document this.

[rsadam, 2014-10-21 18:37:49]

> > This is the case that a device is blocked and then removed. If it is plugged
> > back in it should not still be blocked.
> 
> I you guaranteed that the same device gets the same id? Please document this.

No, there is no such guarantee. Based on my testing on Pixel the device ids are
assigned as the first in sequence that are free. The case I was referring to was
that device 7 was blocked, device 7 is unplugged, device 7 is replugged (and
reassigned id 7 since 1 to 6 are in use). Device 7 should not be blocked.

A better example would be if you unplug a blocked device, and plug in a
completely different device. This new device would have the old device's id
(assuming no other devices have been removed so far), and should not be blocked.

[rsadam, 2014-10-21 19:37:22]

https://codereview.chromium.org/618283003/diff/560001/ui/events/x/device_data...
File ui/events/x/device_data_manager_x11.cc (right):

https://codereview.chromium.org/618283003/diff/560001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:695: int id = static_cast<int>(deviceid);
On 2014/10/21 18:15:32, sky wrote:
> On 2014/10/21 16:53:52, rsadam wrote:
> > On 2014/10/20 22:18:44, sky wrote:
> > > Why the static cast here? Shouldn't the types match?
> > 
> > The input types don't match - the existing logic in DeviceDataManagerX11
uses
> > unsigned ints, whereas the rest of the code depending on InputDevices
expects
> > the device id to be of type int. Would it make sense to refactor
> > DeviceDataManager in a future CL to also use ints?
> 
> Is there a reason InputDevices needs to be int? Can it be unsigned int? I
would
> prefer you clean up types first, then do this one.

Done.

https://codereview.chromium.org/618283003/diff/560001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:756: blocked_devices_.set(id, false);
On 2014/10/21 18:15:32, sky wrote:
> On 2014/10/21 16:53:52, rsadam wrote:
> > On 2014/10/20 22:18:44, sky wrote:
> > > Why do you need to persist this?
> > 
> > This is the case that a device is blocked and then removed. If it is plugged
> > back in it should not still be blocked.
> 
> I you guaranteed that the same device gets the same id? Please document this.

(Replied in message above)

[sky, 2014-10-21 21:51:04]

On Tue, Oct 21, 2014 at 11:37 AM,  <rsadam@chromium.org> wrote:
>> > This is the case that a device is blocked and then removed. If it is
>> > plugged
>> > back in it should not still be blocked.
>
>
>> I you guaranteed that the same device gets the same id? Please document
>> this.
>
>
> No, there is no such guarantee. Based on my testing on Pixel the device ids
> are
> assigned as the first in sequence that are free. The case I was referring to
> was
> that device 7 was blocked, device 7 is unplugged, device 7 is replugged (and
> reassigned id 7 since 1 to 6 are in use). Device 7 should not be blocked.
>
> A better example would be if you unplug a blocked device, and plug in a
> completely different device. This new device would have the old device's id
> (assuming no other devices have been removed so far), and should not be
> blocked.

Doesn't this imply you shouldn't attempt to persist blocked state?

  -Scott
>
> https://codereview.chromium.org/618283003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[rsadam, 2014-10-21 21:55:44]

> Doesn't this imply you shouldn't attempt to persist blocked state?

I'm not sure I follow. We're setting blocked to false, i.e resetting it to the
default to the default value.

[rsadam, 2014-10-21 21:56:50]

> Doesn't this imply you shouldn't attempt to persist blocked state?

I'm not sure I follow. We're setting blocked to false, i.e resetting it to the
default value.

[sky, 2014-10-21 23:56:34]

Why does the entry need to be in the blocked_devices_ anymore then?
Can't it be nuked?

On Tue, Oct 21, 2014 at 2:56 PM,  <rsadam@chromium.org> wrote:
>> Doesn't this imply you shouldn't attempt to persist blocked state?
>
>
> I'm not sure I follow. We're setting blocked to false, i.e resetting it to
> the
> default value.
>
>
>
> https://codereview.chromium.org/618283003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[rsadam, 2014-10-22 01:28:55]

On 2014/10/21 23:56:34, sky wrote:
> Why does the entry need to be in the blocked_devices_ anymore then?
> Can't it be nuked?

My intention is to nuke it. blocked_devices_ is a std::bitset, in which case
isn't setting the value to false the equivalent of nuking it?

[sky, 2014-10-22 15:07:19]

My mistake, I thought it was a map. LGTM

https://codereview.chromium.org/618283003/diff/640001/ui/events/x/device_data...
File ui/events/x/device_data_manager_x11.cc (right):

https://codereview.chromium.org/618283003/diff/640001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:746: std::map<unsigned int,
KeyboardDevice>::iterator blocked_iter;
nit: move into for loop.

[rsadam, 2014-10-22 16:21:16]

https://codereview.chromium.org/618283003/diff/640001/ui/events/x/device_data...
File ui/events/x/device_data_manager_x11.cc (right):

https://codereview.chromium.org/618283003/diff/640001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.cc:746: std::map<unsigned int,
KeyboardDevice>::iterator blocked_iter;
On 2014/10/22 15:07:18, sky wrote:
> nit: move into for loop.

Done.

[rsadam, 2014-10-22 16:37:41]

Fixed merge conflict and rebased to ToT.

[rsadam, 2014-10-22 18:30:54]

The CQ bit was checked by rsadam@chromium.org

[commit-bot: I haz the power, 2014-10-22 18:34:06]

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

[commit-bot: I haz the power, 2014-10-22 19:25:39]

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

[commit-bot: I haz the power, 2014-10-22 19:25:41]

Try jobs failed on following builders:
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[rsadam, 2014-10-22 19:26:20]

The CQ bit was checked by rsadam@chromium.org

[commit-bot: I haz the power, 2014-10-22 19:29:00]

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

[commit-bot: I haz the power, 2014-10-22 21:30:29]

Committed patchset #26 (id:680001)

[commit-bot: I haz the power, 2014-10-22 21:31:21]

Patchset 26 (id:??) landed as
https://crrev.com/d761ce02ab03d1a40f053a9c054a5c0473d94906
Cr-Commit-Position: refs/heads/master@{#300764}