When receiving a key event on Windows, set host lock state to match client's

remoting/host/input_injector_win.cc

Index: remoting/host/input_injector_win.cc
diff --git a/remoting/host/input_injector_win.cc b/remoting/host/input_injector_win.cc
index 9354eda63932baf3730db215a64314e34a1fe823..dca60a893633249f55e76d1a01c9f9c58384f71a 100644
--- a/remoting/host/input_injector_win.cc
+++ b/remoting/host/input_injector_win.cc
@@ -199,6 +199,11 @@ 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

[joedow, 2017/04/27 19:43:43]

nit: add period to end of comment

[rkjnsn, 2017/04/28 17:06:27]

Done.

+    void IsLockKey(int scancode);

[joedow, 2017/04/27 19:43:42]

nit: add newline here

[rkjnsn, 2017/04/28 17:06:27]

Done.

+    // 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_;
@@ -348,6 +353,10 @@ void InputInjectorWin::Core::HandleKey(const KeyEvent& event) {
   if (scancode == ui::KeycodeConverter::InvalidNativeKeycode())
     return;
 
+  if (event.has_lock_states() && !IsLockKey(scancode)) {

[joedow, 2017/04/27 19:43:43]

When testing this change, it looks like the lock state is totally broken.

What I see happen is the lock state is correctly applied when sending the key
(e.g. caps lock) but then when I press another key (e.g. 'A'), I see this code
run and toggle the caps lock state back to the old state before the key is
injected.

[rkjnsn, 2017/04/28 17:06:27]

Looks like I had | where I should have had &.

+    SetLockStates(event.lock_states());
+  }
+
   uint32_t flags = KEYEVENTF_SCANCODE | (event.pressed() ? 0 : KEYEVENTF_KEYUP);
   SendKeyboardInput(flags, scancode);
 }
@@ -384,6 +393,33 @@ void InputInjectorWin::Core::HandleTouch(const TouchEvent& event) {
   touch_injector_->InjectTouchEvent(event);
 }
 
+void 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];

[joedow, 2017/04/27 19:43:43]

You can remove the memset call and use this for initialization (and save a
line):
INPUT input[2] = {0};

[rkjnsn, 2017/04/28 17:06:27]

Ah, right. I took and modified the code from SendKeyboardInput, which uses
memset, but of course value initializing should work. I changed it to `INPUT
input[2] = {}`, as I believe that is more in line with typical C++ style than `=
{0}`.

[joedow, 2017/04/28 17:16:26]

Either way is fine with me :)

+  memset(&input, 0, sizeof(input));
+  input[0].type = INPUT_KEYBOARD;
+  input[1].type = INPUT_KEYBOARD;
+  input[1].ki.dwFlags = KEYEVENTF_KEYUP;
+  if (!!(states | protocol::KeyEvent::LOCK_STATES_CAPSLOCK) !=
+      !!(GetKeyState(VK_CAPITAL) & 1)) {

[Jamie, 2017/04/27 19:06:32]

I find !! to be not very readable. A better approach would be to assign
variables |expected_caps_lock| and |actual_caps_lock| (resp. num_lock) so the
code is self-documenting.

[joedow, 2017/04/27 19:43:43]

+1, double-bang boolification is less readable in this case since you are
comparing to an expected state.  Either use the method Jamie suggested or use an
anonymous helper function (w/o the !!).

[rkjnsn, 2017/04/28 17:06:27]

Would changing `!!(states & protocol::KeyEvent::LOCK_STATES_CAPSLOCK` to
`bool{states & protocol::KeyEvent::LOCK_STATES_CAPSLOCK}` be more readable?

[joedow, 2017/04/28 17:16:26]

My preference is to extract the work needed to determine the state from the
comparison.  The additional operators break the flow.

I think the ideal would be:
current_state = stuff
expected_state = more stuff
if (current_state != expected_state) { DoOtherStuff(); }

Reducing the cognitive overhead when parsing the conditions would make it more
maintainable.

If you want to pull out the logic and use brace initialization (with readable
var names), that should clean it up nicely.

[rkjnsn, 2017/04/28 17:33:12]

Fun fact: C++11 apparently disallowed non-constant narrowing conversions in
brace initializers.

+    input[0].ki.wVk = VK_CAPITAL;
+    input[1].ki.wVk = VK_CAPITAL;
+    SendInput(arraysize(input), input, sizeof(INPUT));
+  }
+  if (!!(states | protocol::KeyEvent::LOCK_STATES_NUMLOCK) !=
+      !!(GetKeyState(VK_NUMLOCK) & 1)) {
+    input[0].ki.wVk = VK_NUMLOCK;
+    input[1].ki.wVk = VK_NUMLOCK;
+    SendInput(arraysize(input), input, sizeof(INPUT));
+  }
+}
+
 }  // namespace
 
 // static