Add discrete caps/num lock fields to event protocol

Previously, there was a single field for all lock states. This was
problematic, because some platforms only support caps lock. These
platforms were thus forced to include bogus state for num lock. Making
them separate fields allows these platforms to specify only the
caps-lock state.

Bug: 760731
Change-Id: Id72bbb09e1a8ed218cf77496cfd76cf1df8c97c0
Reviewed-on: https://chromium-review.googlesource.com/692752
Commit-Queue: Erik Jensen <rkjnsn@chromium.org>
Reviewed-by: Jamie Walch <jamiewalch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505725}

remoting/client/input/normalizing_input_filter_cros.cc

@@ -97,9 +97,13 @@ NormalizingInputFilterCros::NormalizingInputFilterCros(
 NormalizingInputFilterCros::~NormalizingInputFilterCros() {}
 
 void NormalizingInputFilterCros::InjectKeyEvent(
-    const protocol::KeyEvent& event) {
-  DCHECK(event.has_usb_keycode());
-  DCHECK(event.has_pressed());
+    const protocol::KeyEvent& event_arg) {
+  DCHECK(event_arg.has_usb_keycode());
+  DCHECK(event_arg.has_pressed());
+
+  // ChromeOS doesn't have a concept of num lock, so unset the field.
+  protocol::KeyEvent event(event_arg);
+  event.clear_num_lock_state();
 
   if (event.pressed()) {
     ProcessKeyDown(event);

remoting/client/input/normalizing_input_filter_mac.cc

@@ -20,8 +20,12 @@ NormalizingInputFilterMac::NormalizingInputFilterMac(
 NormalizingInputFilterMac::~NormalizingInputFilterMac() {}
 
 void NormalizingInputFilterMac::InjectKeyEvent(
-    const protocol::KeyEvent& event) {
-  DCHECK(event.has_usb_keycode());
+    const protocol::KeyEvent& event_arg) {
+  DCHECK(event_arg.has_usb_keycode());
+
+  // Mac OS X doesn't have a concept of num lock, so unset the field.
+  protocol::KeyEvent event(event_arg);
+  event.clear_num_lock_state();
 
   ui::DomCode dom_code = static_cast<ui::DomCode>(event.usb_keycode());
 

remoting/client/plugin/pepper_input_handler.cc

@@ -70,17 +70,25 @@ protocol::TouchEvent MakeTouchEvent(const pp::TouchInputEvent& pp_touch_event) {
 }
 
 // Builds the Chromotocol lock states flags for the PPAPI |event|.
-uint32_t MakeLockStates(const pp::InputEvent& event) {
-  uint32_t modifiers = event.GetModifiers();
-  uint32_t lock_states = 0;
+void SetLockStates(protocol::KeyEvent* key_event,
+                   const pp::InputEvent& pp_key_event) {
+  uint32_t modifiers = pp_key_event.GetModifiers();
+  bool caps_lock = (modifiers & PP_INPUTEVENT_MODIFIER_CAPSLOCKKEY) != 0;
+  bool num_lock = (modifiers & PP_INPUTEVENT_MODIFIER_NUMLOCKKEY) != 0;
 
-  if (modifiers & PP_INPUTEVENT_MODIFIER_CAPSLOCKKEY)
-    lock_states |= protocol::KeyEvent::LOCK_STATES_CAPSLOCK;
+  // Set the new discrete lock state fields
+  key_event->set_caps_lock_state(caps_lock);
+  key_event->set_num_lock_state(num_lock);
 
-  if (modifiers & PP_INPUTEVENT_MODIFIER_NUMLOCKKEY)
+  // Also set the legacy lock_states field to support older hosts.
+  uint32_t lock_states = 0;
+  if (caps_lock) {
+    lock_states |= protocol::KeyEvent::LOCK_STATES_CAPSLOCK;
+  }
+  if (num_lock) {
     lock_states |= protocol::KeyEvent::LOCK_STATES_NUMLOCK;
-
-  return lock_states;
+  }
+  key_event->set_lock_states(lock_states);
 }
 
 // Builds a protocol::KeyEvent from the supplied PPAPI event.
@@ -100,7 +108,7 @@ protocol::KeyEvent MakeKeyEvent(const pp::KeyboardInputEvent& pp_key_event) {
   key_event.set_usb_keycode(
       ui::KeycodeConverter::CodeStringToUsbKeycode(dom_code));
   key_event.set_pressed(pp_key_event.GetType() == PP_INPUTEVENT_TYPE_KEYDOWN);
-  key_event.set_lock_states(MakeLockStates(pp_key_event));
+  SetLockStates(&key_event, pp_key_event);
   return key_event;
 }
 

remoting/host/input_injector_chromeos.cc

@@ -45,9 +45,8 @@ ui::EventFlags MouseButtonToUIFlags(MouseEvent::MouseButton button) {
   }
 }
 
-bool shouldSetLockStates(ui::DomCode dom_code, bool key_pressed) {
-  if (!key_pressed)
-    return false;
+// Check if the given key could be mapped to caps lock
+bool IsLockKey(ui::DomCode dom_code) {
   switch (dom_code) {
     // Ignores all the keys that could possibly be mapped to Caps Lock in event
     // rewriter. Please refer to ui::EventRewriterChromeOS::RewriteModifierKeys.
@@ -61,12 +60,19 @@ bool shouldSetLockStates(ui::DomCode dom_code, bool key_pressed) {
     case ui::DomCode::ALT_RIGHT:
     case ui::DomCode::ESCAPE:
     case ui::DomCode::BACKSPACE:
-      return false;
-    default:
       return true;
+    default:
+      return false;
   }
 }
 
+// If caps_lock is specified, sets local keyboard state to match.
+void SetCapsLockState(bool caps_lock) {
+  chromeos::input_method::InputMethodManager* ime =
+      chromeos::input_method::InputMethodManager::Get();
+  ime->GetImeKeyboard()->SetCapsLockEnabled(caps_lock);
+}
+
 }  // namespace
 
 // This class is run exclusively on the UI thread of the browser process.
@@ -82,7 +88,6 @@ class InputInjectorChromeos::Core {
   void Start(std::unique_ptr<protocol::ClipboardStub> client_clipboard);
 
  private:
-  // Sets the caps lock state to match states
   void SetLockStates(uint32_t states);
 
   std::unique_ptr<ui::SystemInputInjector> delegate_;
@@ -115,9 +120,13 @@ void InputInjectorChromeos::Core::InjectKeyEvent(const KeyEvent& event) {
   ui::DomCode dom_code =
       ui::KeycodeConverter::UsbKeycodeToDomCode(event.usb_keycode());
 
-  if (event.has_lock_states() &&
-      shouldSetLockStates(dom_code, event.pressed())) {
-    SetLockStates(event.lock_states());
+  if (event.pressed() && !IsLockKey(dom_code)) {
+    if (event.has_caps_lock_state()) {
+      SetCapsLockState(event.caps_lock_state());
+    } else if (event.has_lock_states()) {
+      SetCapsLockState((event.lock_states() &
+                        protocol::KeyEvent::LOCK_STATES_CAPSLOCK) != 0);
+    }
   }
 
   // Ignore events which can't be mapped.
@@ -158,13 +167,6 @@ void InputInjectorChromeos::Core::Start(
   point_transformer_.reset(new PointTransformer());
 }
 
-void InputInjectorChromeos::Core::SetLockStates(uint32_t states) {
-  chromeos::input_method::InputMethodManager* ime =
-      chromeos::input_method::InputMethodManager::Get();
-  ime->GetImeKeyboard()->SetCapsLockEnabled(
-      states & protocol::KeyEvent::LOCK_STATES_CAPSLOCK);
-}
-
 InputInjectorChromeos::InputInjectorChromeos(
     scoped_refptr<base::SingleThreadTaskRunner> task_runner,
     ui::SystemInputInjectorFactory* system_input_injector_factory)

remoting/host/input_injector_mac.cc

@@ -233,8 +233,11 @@ void InputInjectorMac::Core::InjectKeyEvent(const KeyEvent& event) {
   // In addition to the modifier keys pressed right now, we also need to set
   // AlphaShift if caps lock was active at the client (Mac ignores NumLock).
   uint64_t flags = left_modifiers_ | right_modifiers_;
-  if (event.lock_states() & protocol::KeyEvent::LOCK_STATES_CAPSLOCK)
+  if ((event.has_caps_lock_state() && event.caps_lock_state()) ||
+      (event.has_lock_states() &&
+       (event.lock_states() & protocol::KeyEvent::LOCK_STATES_CAPSLOCK) != 0)) {
     flags |= kCGEventFlagMaskAlphaShift;
+  }
 
   CreateAndPostKeyEvent(keycode, event.pressed(), flags, base::string16());
 }

remoting/host/input_injector_win.cc

@@ -16,6 +16,7 @@
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
+#include "base/optional.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
@@ -149,6 +150,44 @@ void ParseMouseWheelEvent(const MouseEvent& event, std::vector<INPUT>* output) {
   }
 }
 
+// Check if the given scan code is caps lock or num lock.
+bool IsLockKey(int scancode) {
+  UINT virtual_key = MapVirtualKey(scancode, MAPVK_VSC_TO_VK);
+  return virtual_key == VK_CAPITAL || virtual_key == VK_NUMLOCK;
+}
+
+// Sets the keyboard lock states to those provided.
+void SetLockStates(base::Optional<bool> caps_lock,
+                   base::Optional<bool> num_lock) {
+  // Can't use SendKeyboardInput because we need to send virtual key codes, not
+  // scan codes.
+  INPUT input[2] = {};
+  input[0].type = INPUT_KEYBOARD;
+  input[1].type = INPUT_KEYBOARD;
+  input[1].ki.dwFlags = KEYEVENTF_KEYUP;
+
+  if (caps_lock) {
+    bool client_capslock_state = *caps_lock;
+    bool host_capslock_state = (GetKeyState(VK_CAPITAL) & 1) != 0;
+    if (client_capslock_state != host_capslock_state) {
+      input[0].ki.wVk = VK_CAPITAL;
+      input[1].ki.wVk = VK_CAPITAL;
+      SendInput(arraysize(input), input, sizeof(INPUT));
+    }
+  }
+
+  // Sets the keyboard lock states to those provided.
+  if (num_lock) {
+    bool client_numlock_state = *num_lock;
+    bool host_numlock_state = (GetKeyState(VK_NUMLOCK) & 1) != 0;
+    if (client_numlock_state != host_numlock_state) {
+      input[0].ki.wVk = VK_NUMLOCK;
+      input[1].ki.wVk = VK_NUMLOCK;
+      SendInput(arraysize(input), input, sizeof(INPUT));
+    }
+  }
+}
+
 // A class to generate events on Windows.
 class InputInjectorWin : public InputInjector {
  public:
@@ -199,12 +238,6 @@ class InputInjectorWin : public InputInjector {
     void HandleMouse(const MouseEvent& event);
     void HandleTouch(const TouchEvent& event);
 
-    // Check if the given scan code is caps lock or num lock.
-    bool IsLockKey(int scancode);
-
-    // Sets the keyboard lock states to those provided.
-    void SetLockStates(uint32_t states);
-
     scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;
     scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner_;
     std::unique_ptr<Clipboard> clipboard_;
@@ -354,8 +387,27 @@ void InputInjectorWin::Core::HandleKey(const KeyEvent& event) {
   if (scancode == ui::KeycodeConverter::InvalidNativeKeycode())
     return;
 
-  if (event.has_lock_states() && event.pressed() && !IsLockKey(scancode)) {
-    SetLockStates(event.lock_states());
+  if (event.pressed() && !IsLockKey(scancode)) {
+    base::Optional<bool> caps_lock;
+    base::Optional<bool> num_lock;
+
+    // For caps lock, check both the new caps_lock field and the old lock_states
+    // field.
+    if (event.has_caps_lock_state()) {
+      caps_lock = event.caps_lock_state();
+    } else if (event.has_lock_states()) {
+      caps_lock =
+          (event.lock_states() & protocol::KeyEvent::LOCK_STATES_CAPSLOCK) != 0;
+    }
+
+    // Not all clients have a concept of num lock. Since there's no way to
+    // distinguish these clients using the legacy lock_states field, only update
+    // if the new num_lock field is specified.
+    if (event.has_num_lock_state()) {
+      num_lock = event.num_lock_state();
+    }
+
+    SetLockStates(caps_lock, num_lock);
   }
 
   uint32_t flags = KEYEVENTF_SCANCODE | (event.pressed() ? 0 : KEYEVENTF_KEYUP);
@@ -394,31 +446,6 @@ void InputInjectorWin::Core::HandleTouch(const TouchEvent& event) {
   touch_injector_->InjectTouchEvent(event);
 }
 
-bool InputInjectorWin::Core::IsLockKey(int scancode) {
-  UINT virtual_key = MapVirtualKey(scancode, MAPVK_VSC_TO_VK);
-  return virtual_key == VK_CAPITAL || virtual_key == VK_NUMLOCK;
-}
-
-void InputInjectorWin::Core::SetLockStates(uint32_t states) {
-  // Can't use SendKeyboardInput because we need to send virtual key codes, not
-  // scan codes.
-  INPUT input[2] = {};
-  input[0].type = INPUT_KEYBOARD;
-  input[1].type = INPUT_KEYBOARD;
-  input[1].ki.dwFlags = KEYEVENTF_KEYUP;
-
-  bool client_capslock_state =
-      (states & protocol::KeyEvent::LOCK_STATES_CAPSLOCK) != 0;
-  bool host_capslock_state = (GetKeyState(VK_CAPITAL) & 1) != 0;
-  if (client_capslock_state != host_capslock_state) {
-    input[0].ki.wVk = VK_CAPITAL;
-    input[1].ki.wVk = VK_CAPITAL;
-    SendInput(arraysize(input), input, sizeof(INPUT));
-  }
-  // TODO(jamiewalch): Reinstate NumLock synchronization when the protocol
-  //     supports the client reporting it as "unknown".
-}
-
 }  // namespace
 
 // static

remoting/host/input_injector_x11.cc

@@ -21,6 +21,7 @@
 #include "base/location.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
+#include "base/optional.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/utf_string_conversion_utils.h"
 #include "build/build_config.h"
@@ -126,7 +127,8 @@ class InputInjectorX11 : public InputInjector {
     bool IsLockKey(KeyCode keycode);
 
     // Sets the keyboard lock states to those provided.
-    void SetLockStates(uint32_t states);
+    void SetLockStates(base::Optional<bool> caps_lock,
+                       base::Optional<bool> num_lock);
 
     void InjectScrollWheelClicks(int button, int count);
     // Compensates for global button mappings and resets the XTest device
@@ -283,8 +285,27 @@ void InputInjectorX11::Core::InjectKeyEvent(const KeyEvent& event) {
       XTestFakeKeyEvent(display_, keycode, False, CurrentTime);
     }
 
-    if (event.has_lock_states() && !IsLockKey(keycode)) {
-      SetLockStates(event.lock_states());
+    if (!IsLockKey(keycode)) {
+      base::Optional<bool> caps_lock;
+      base::Optional<bool> num_lock;
+
+      // For caps lock, check both the new caps_lock field and the old
+      // lock_states field.
+      if (event.has_caps_lock_state()) {
+        caps_lock = event.caps_lock_state();
+      } else if (event.has_lock_states()) {
+        caps_lock = (event.lock_states() &
+                     protocol::KeyEvent::LOCK_STATES_CAPSLOCK) != 0;
+      }
+
+      // Not all clients have a concept of num lock. Since there's no way to
+      // distinguish these clients using the legacy lock_states field, only
+      // update if the new num_lock field is specified.
+      if (event.has_num_lock_state()) {
+        num_lock = event.num_lock_state();
+      }
+
+      SetLockStates(caps_lock, num_lock);
     }
 
     if (pressed_keys_.empty()) {
@@ -370,15 +391,32 @@ bool InputInjectorX11::Core::IsLockKey(KeyCode keycode) {
   }
 }
 
-void InputInjectorX11::Core::SetLockStates(uint32_t states) {
-  // TODO(jamiewalch): Reinstate NumLock synchronization when the protocol
-  //     supports the client reporting it as "unknown".
+void InputInjectorX11::Core::SetLockStates(base::Optional<bool> caps_lock,
+                                           base::Optional<bool> num_lock) {
+  // The lock bits associated with each lock key.
   unsigned int caps_lock_mask = XkbKeysymToModifiers(display_, XK_Caps_Lock);
-  unsigned int lock_values = 0;
-  if (states & protocol::KeyEvent::LOCK_STATES_CAPSLOCK) {
-    lock_values |= caps_lock_mask;
+  unsigned int num_lock_mask = XkbKeysymToModifiers(display_, XK_Num_Lock);
+
+  unsigned int update_mask = 0;  // The lock bits we want to update
+  unsigned int lock_values = 0;  // The value of those bits
+
+  if (caps_lock) {
+    update_mask |= caps_lock_mask;
+    if (*caps_lock) {
+      lock_values |= caps_lock_mask;
+    }
+  }
+
+  if (num_lock) {
+    update_mask |= num_lock_mask;
+    if (*num_lock) {
+      lock_values |= num_lock_mask;
+    }
+  }
+
+  if (update_mask) {
+    XkbLockModifiers(display_, XkbUseCoreKbd, update_mask, lock_values);
   }
-  XkbLockModifiers(display_, XkbUseCoreKbd, caps_lock_mask, lock_values);
 }
 
 void InputInjectorX11::Core::InjectScrollWheelClicks(int button, int count) {

remoting/proto/event.proto

@@ -27,8 +27,14 @@ message KeyEvent {
   // The lower 16-bits are the USB Usage ID (which identifies the actual key).
   optional uint32 usb_keycode = 3;
 
-  // The keyboard lock states.
+  // Legacy keyboard lock states. Prefer the discrete entries below.
   optional uint32 lock_states = 4 [default = 0];
+
+  // Keyboard lock states. The field should be specified only if the state can
+  // be reliably determined by the client. E.g., OS X does not have num lock, so
+  // only caps_lock should be provided by a client running on OS X.
+  optional bool caps_lock_state = 5;
+  optional bool num_lock_state = 6;
 }
 
 // Text input event for input method different from physical keyboards,