Reapply Caps Lock and Num Lock to core keyboard whenever new X input device is added

Reapply Caps Lock and Num Lock to core keyboard whenever new X input device is added.

- Let Chrome maintain Num Lock & Caps Lock state. Monitor the X device hierarchy, and, upon detecting any newly attached keyboards, reapply the current Num Lock and Caps Lock state to the master/core keyboard, which will then propagate down to all slaves (including the newly attached one).
- Similarly, whenever Chrome detects a Num Lock and/or Caps Lock state change originating from the server, it parrots the change back down to X.  This fixes the XKB asymmetric indicator state propagation.

See also: http://crosbug.com/p/6245#c18

BUG=chromium-os:22562
BUG=crosbug.com/p/5795
BUG=crosbug.com/p/6245
TEST=On Mario and Alex, connect a USB hub, and connect two keyboards, say A and B, to the hub. Press NumLock on A, confirm that the NumLock LED on B also changes. Do the same for CapsLock.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=108841

[Zachary Kuznia, 2011-10-21 04:15:31]

lgtm

[Yusuke Sato, 2011-11-04 09:36:14]

djkurtz:
Fixed the CL following your (offline) suggestion. Please take another look.

[Daniel Kurtz, 2011-11-04 11:49:18]

Looks really good!

Just some potential optimizations to think about...

http://codereview.chromium.org/8356040/diff/6001/chrome/browser/chromeos/inpu...
File chrome/browser/chromeos/input_method/xkeyboard.cc (right):

http://codereview.chromium.org/8356040/diff/6001/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/xkeyboard.cc:437:
SetCapsLockEnabled(current_caps_lock_status_);
Save yourself a round trip to the server by applying both states together in a
single call to XkbLockModifiers().

http://codereview.chromium.org/8356040/diff/6001/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system_key_event_listener.cc (right):

http://codereview.chromium.org/8356040/diff/6001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system_key_event_listener.cc:296: const bool
new_caps_lock_state =
Do this in one pass:  First extract the two states, check if either has changed,
and, if so, update the current state of both and echo the new state of both back
down to the server in a single X command.

http://codereview.chromium.org/8356040/diff/6001/chrome/browser/chromeos/xinp...
File chrome/browser/chromeos/xinput_hierarchy_changed_event_listener.cc (right):

http://codereview.chromium.org/8356040/diff/6001/chrome/browser/chromeos/xinp...
chrome/browser/chromeos/xinput_hierarchy_changed_event_listener.cc:52:
(info->flags & XISlaveAdded)) {
I think this searches the entire hierarchy on every HierarchyChangeEvent
(add/attach/detach/remove).  Also, we really only care about Keyboards, so, you
can try just watching for when a new SlaveKeyboard is Attached to a Master.

So, can you try this, instead:

void HHCE() {
  XHI* info = &event->info[i];
  if (!(event->flags & XISlaveAttached))
    return;
  for (int i = 0; i < event->num_info; ++i) {
    XIHI* info = event->info[i];
    if (info->flags & XISlaveAttached &&
        info->use == XISlaveKeyboard) {
      ...
    }
  }
}

[Yusuke Sato, 2011-11-07 04:31:28]

http://codereview.chromium.org/8356040/diff/6001/chrome/browser/chromeos/inpu...
File chrome/browser/chromeos/input_method/xkeyboard.cc (right):

http://codereview.chromium.org/8356040/diff/6001/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/xkeyboard.cc:437:
SetCapsLockEnabled(current_caps_lock_status_);
On 2011/11/04 11:49:18, Daniel Kurtz wrote:
> Save yourself a round trip to the server by applying both states together in a
> single call to XkbLockModifiers().

Done.

http://codereview.chromium.org/8356040/diff/6001/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system_key_event_listener.cc (right):

http://codereview.chromium.org/8356040/diff/6001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system_key_event_listener.cc:296: const bool
new_caps_lock_state =
On 2011/11/04 11:49:18, Daniel Kurtz wrote:
> Do this in one pass:  First extract the two states, check if either has
changed,
> and, if so, update the current state of both and echo the new state of both
back
> down to the server in a single X command.

Done.

http://codereview.chromium.org/8356040/diff/6001/chrome/browser/chromeos/xinp...
File chrome/browser/chromeos/xinput_hierarchy_changed_event_listener.cc (right):

http://codereview.chromium.org/8356040/diff/6001/chrome/browser/chromeos/xinp...
chrome/browser/chromeos/xinput_hierarchy_changed_event_listener.cc:52:
(info->flags & XISlaveAdded)) {
On 2011/11/04 11:49:18, Daniel Kurtz wrote:
> I think this searches the entire hierarchy on every HierarchyChangeEvent
> (add/attach/detach/remove).  Also, we really only care about Keyboards, so,
you
> can try just watching for when a new SlaveKeyboard is Attached to a Master.
> 
> So, can you try this, instead:
> 
> void HHCE() {
>   XHI* info = &event->info[i];
>   if (!(event->flags & XISlaveAttached))
>     return;
>   for (int i = 0; i < event->num_info; ++i) {
>     XIHI* info = event->info[i];
>     if (info->flags & XISlaveAttached &&
>         info->use == XISlaveKeyboard) {
>       ...
>     }
>   }
> }
> 

Do you mean 'if (!(event->flags & XISlaveAdded))'? Assuming the answer is yes,
it seems that X does not send an XISlaveAdded+XISlaveKeyboard event to Chrome
when an external keyboard is plugged.

So, let me go with this:

   if (!(event->flags & XISlaveAdded))
     return;
   for (int i = 0; i < event->num_info; ++i) {
     XIHI* info = event->info[i];
     if (info->flags & XISlaveAttached &&
         info->use == XIFloatingSlave) {

[Yusuke Sato, 2011-11-07 05:06:44]

Correction:

    if (!(event->flags & XISlaveAdded))
      return;
    for (int i = 0; i < event->num_info; ++i) {
      XIHI* info = event->info[i];
      if (info->flags & XISlaveAdded &&    // I meant to say 'Added', NOT
'Attached'. Patch Set #5 uses 'Added' and it's working fine.
          info->use == XIFloatingSlave) {

[Daniel Kurtz, 2011-11-07 06:30:01]

LGTM.

It is unfortunate that X doesn't seem to be sending Attached events for
SlaveKeyboards.   Let's leave further investigation to another CL.

[commit-bot: I haz the power, 2011-11-07 07:13:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusukes@chromium.org/8356040/18001

[commit-bot: I haz the power, 2011-11-07 08:55:52]

Change committed as 108841