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

remoting/host/input_injector_win.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "remoting/host/input_injector.h"
 
 #include <stdint.h>
 #include <windows.h>
 
 #include <utility>
@@ skipping 181 matching lines @@
 
    private:
     friend class base::RefCountedThreadSafe<Core>;
     virtual ~Core();
 
     void HandleKey(const KeyEvent& event);
     void HandleText(const TextEvent& event);
     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_;
     std::unique_ptr<TouchInjectorWin> touch_injector_;
 
     DISALLOW_COPY_AND_ASSIGN(Core);
   };
 
   scoped_refptr<Core> core_;
 
@@ skipping 129 matching lines @@
 
   int scancode =
       ui::KeycodeConverter::UsbKeycodeToNativeKeycode(event.usb_keycode());
   VLOG(3) << "Converting USB keycode: " << std::hex << event.usb_keycode()
           << " to scancode: " << scancode << std::dec;
 
   // Ignore events which can't be mapped.
   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);
 }
 
 void InputInjectorWin::Core::HandleText(const TextEvent& event) {
   // HostEventDispatcher should filter events missing the pressed field.
   DCHECK(event.has_text());
 
   base::string16 text = base::UTF8ToUTF16(event.text());
   for (base::string16::const_iterator it = text.begin();
@@ skipping 16 matching lines @@
     if (SendInput(inputs.size(), inputs.data(), sizeof(INPUT)) != inputs.size())
       PLOG(ERROR) << "Failed to inject a mouse event";
   }
 }
 
 void InputInjectorWin::Core::HandleTouch(const TouchEvent& event) {
   DCHECK(touch_injector_);
   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
 std::unique_ptr<InputInjector> InputInjector::Create(
     scoped_refptr<base::SingleThreadTaskRunner> main_task_runner,
     scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner) {
   return base::WrapUnique(
       new InputInjectorWin(main_task_runner, ui_task_runner));
 }
 
 // static
 bool InputInjector::SupportsTouchEvents() {
   return base::win::GetVersion() >= base::win::VERSION_WIN8;
 }
 
 }  // namespace remoting