Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2843213002:
When receiving a key event on Windows, set host lock state to match client's (Closed)
DescriptionWhen receiving a key event on Windows, set host lock state to match client's
BUG=176436
Review-Url: https://codereview.chromium.org/2843213002
Cr-Commit-Position: refs/heads/master@{#468442}
Committed: https://chromium.googlesource.com/chromium/src/+/cdd685a13fa2abf0fee9f2041cffcccaaf3477e4
Patch Set 1 #
Total comments: 1
Patch Set 2 : Don't try to toggle lock states if current keypres is a lock key. #
Total comments: 14
Patch Set 3 : Address feedback #
Total comments: 4
Patch Set 4 : Fix C4800 #Messages
Total messages: 25 (14 generated)
The CQ bit was checked by rkjnsn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rkjnsn@chromium.org changed reviewers: + joedow@chromium.org
Joe, Can you you review (and possibly test) these changes? Thanks. https://codereview.chromium.org/2843213002/diff/1/remoting/host/input_injecto... File remoting/host/input_injector_win.cc (right): https://codereview.chromium.org/2843213002/diff/1/remoting/host/input_injecto... remoting/host/input_injector_win.cc:414: } This builds, but I'm not able to test it, as I don't have a Windows machine. It is based on my understanding of the documentation (https://msdn.microsoft.com/en-us/library/ms646310.aspx) and matches examples I have seen online.
The second iteration doesn't build as 'IsLockKey()' is marked void but returns a value. I assume you want it to return a bool so that is what I am going to use for testing.
jamiewalch@chromium.org changed reviewers: + jamiewalch@chromium.org
LGTM with nits. I think it's fine to wait until this lands before testing the official installer. https://codereview.chromium.org/2843213002/diff/20001/remoting/host/input_inj... File remoting/host/input_injector_win.cc (right): https://codereview.chromium.org/2843213002/diff/20001/remoting/host/input_inj... remoting/host/input_injector_win.cc:410: !!(GetKeyState(VK_CAPITAL) & 1)) { 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.
It doesn't look like the lock states are properly managed so I think we should fix it before checking in. I can provide a machine to test on or gather logging if needed. https://codereview.chromium.org/2843213002/diff/20001/remoting/host/input_inj... File remoting/host/input_injector_win.cc (right): https://codereview.chromium.org/2843213002/diff/20001/remoting/host/input_inj... remoting/host/input_injector_win.cc:202: // Check if the given scan code is caps lock or num lock nit: add period to end of comment https://codereview.chromium.org/2843213002/diff/20001/remoting/host/input_inj... remoting/host/input_injector_win.cc:203: void IsLockKey(int scancode); nit: add newline here https://codereview.chromium.org/2843213002/diff/20001/remoting/host/input_inj... remoting/host/input_injector_win.cc:356: if (event.has_lock_states() && !IsLockKey(scancode)) { 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. https://codereview.chromium.org/2843213002/diff/20001/remoting/host/input_inj... remoting/host/input_injector_win.cc:404: INPUT input[2]; You can remove the memset call and use this for initialization (and save a line): INPUT input[2] = {0}; https://codereview.chromium.org/2843213002/diff/20001/remoting/host/input_inj... remoting/host/input_injector_win.cc:410: !!(GetKeyState(VK_CAPITAL) & 1)) { On 2017/04/27 19:06:32, Jamie wrote: > 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. +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 !!).
https://codereview.chromium.org/2843213002/diff/20001/remoting/host/input_inj... File remoting/host/input_injector_win.cc (right): https://codereview.chromium.org/2843213002/diff/20001/remoting/host/input_inj... remoting/host/input_injector_win.cc:202: // Check if the given scan code is caps lock or num lock On 2017/04/27 19:43:43, joedow wrote: > nit: add period to end of comment Done. https://codereview.chromium.org/2843213002/diff/20001/remoting/host/input_inj... remoting/host/input_injector_win.cc:203: void IsLockKey(int scancode); On 2017/04/27 19:43:42, joedow wrote: > nit: add newline here Done. https://codereview.chromium.org/2843213002/diff/20001/remoting/host/input_inj... remoting/host/input_injector_win.cc:356: if (event.has_lock_states() && !IsLockKey(scancode)) { On 2017/04/27 19:43:43, joedow wrote: > 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. Looks like I had | where I should have had &. https://codereview.chromium.org/2843213002/diff/20001/remoting/host/input_inj... remoting/host/input_injector_win.cc:404: INPUT input[2]; On 2017/04/27 19:43:43, joedow wrote: > You can remove the memset call and use this for initialization (and save a > line): > INPUT input[2] = {0}; 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}`. https://codereview.chromium.org/2843213002/diff/20001/remoting/host/input_inj... remoting/host/input_injector_win.cc:410: !!(GetKeyState(VK_CAPITAL) & 1)) { On 2017/04/27 19:43:43, joedow wrote: > On 2017/04/27 19:06:32, Jamie wrote: > > 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. > > +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 !!). Would changing `!!(states & protocol::KeyEvent::LOCK_STATES_CAPSLOCK` to `bool{states & protocol::KeyEvent::LOCK_STATES_CAPSLOCK}` be more readable?
https://codereview.chromium.org/2843213002/diff/20001/remoting/host/input_inj... File remoting/host/input_injector_win.cc (right): https://codereview.chromium.org/2843213002/diff/20001/remoting/host/input_inj... remoting/host/input_injector_win.cc:404: INPUT input[2]; On 2017/04/28 17:06:27, rkjnsn wrote: > On 2017/04/27 19:43:43, joedow wrote: > > You can remove the memset call and use this for initialization (and save a > > line): > > INPUT input[2] = {0}; > > > 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}`. Either way is fine with me :) https://codereview.chromium.org/2843213002/diff/20001/remoting/host/input_inj... remoting/host/input_injector_win.cc:410: !!(GetKeyState(VK_CAPITAL) & 1)) { On 2017/04/28 17:06:27, rkjnsn wrote: > On 2017/04/27 19:43:43, joedow wrote: > > On 2017/04/27 19:06:32, Jamie wrote: > > > 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. > > > > +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 !!). > > Would changing `!!(states & protocol::KeyEvent::LOCK_STATES_CAPSLOCK` to > `bool{states & protocol::KeyEvent::LOCK_STATES_CAPSLOCK}` be more readable? 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.
Uploaded updated version. Don't know if it works, but it should compile. https://codereview.chromium.org/2843213002/diff/20001/remoting/host/input_inj... File remoting/host/input_injector_win.cc (right): https://codereview.chromium.org/2843213002/diff/20001/remoting/host/input_inj... remoting/host/input_injector_win.cc:410: !!(GetKeyState(VK_CAPITAL) & 1)) { On 2017/04/28 17:16:26, joedow wrote: > On 2017/04/28 17:06:27, rkjnsn wrote: > > On 2017/04/27 19:43:43, joedow wrote: > > > On 2017/04/27 19:06:32, Jamie wrote: > > > > 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. > > > > > > +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 !!). > > > > Would changing `!!(states & protocol::KeyEvent::LOCK_STATES_CAPSLOCK` to > > `bool{states & protocol::KeyEvent::LOCK_STATES_CAPSLOCK}` be more readable? > > 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. Fun fact: C++11 apparently disallowed non-constant narrowing conversions in brace initializers.
lgtm This iteration works great (once the build failures were fixed). https://codereview.chromium.org/2843213002/diff/20002/remoting/host/input_inj... File remoting/host/input_injector_win.cc (right): https://codereview.chromium.org/2843213002/diff/20002/remoting/host/input_inj... remoting/host/input_injector_win.cc:411: states & protocol::KeyEvent::LOCK_STATES_CAPSLOCK; I get a build error, this works though: bool client_capslock_state = (states & protocol::KeyEvent::LOCK_STATES_CAPSLOCK) != 0; One note is that using the try-bots would have helped catch this sooner. https://codereview.chromium.org/2843213002/diff/20002/remoting/host/input_inj... remoting/host/input_injector_win.cc:419: bool client_numlock_state = states & protocol::KeyEvent::LOCK_STATES_NUMLOCK; Another build error: bool client_numlock_state = (states & protocol::KeyEvent::LOCK_STATES_NUMLOCK) != 0;
Fixed. https://codereview.chromium.org/2843213002/diff/20002/remoting/host/input_inj... File remoting/host/input_injector_win.cc (right): https://codereview.chromium.org/2843213002/diff/20002/remoting/host/input_inj... remoting/host/input_injector_win.cc:411: states & protocol::KeyEvent::LOCK_STATES_CAPSLOCK; On 2017/05/01 15:14:28, joedow wrote: > I get a build error, this works though: > bool client_capslock_state = > (states & protocol::KeyEvent::LOCK_STATES_CAPSLOCK) != 0; > > One note is that using the try-bots would have helped catch this sooner. I ran the code through Clang before uploading, but it looks like this is a VC++-only "performance warning". (As an aside, I think the warning itself is a little dubious, as the performance implication of boolification is negligible in almost all cases. It'd make more sense if it were a style warning, but it's not, as evidenced by the fact that the warning doesn't trigger on the next line.) Anyway, fixed. I changed the following line, too, to be consistent. https://codereview.chromium.org/2843213002/diff/20002/remoting/host/input_inj... remoting/host/input_injector_win.cc:419: bool client_numlock_state = states & protocol::KeyEvent::LOCK_STATES_NUMLOCK; On 2017/05/01 15:14:28, joedow wrote: > Another build error: > bool client_numlock_state = > (states & protocol::KeyEvent::LOCK_STATES_NUMLOCK) != 0; Changed along with following line.
The CQ bit was checked by rkjnsn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rkjnsn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamiewalch@chromium.org, joedow@chromium.org Link to the patchset: https://codereview.chromium.org/2843213002/#ps50001 (title: "Fix C4800")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 50001, "attempt_start_ts": 1493675992473390,
"parent_rev": "4a961b8eca27af4108358a30753197754316524b", "commit_rev":
"cdd685a13fa2abf0fee9f2041cffcccaaf3477e4"}
Message was sent while issue was closed.
Description was changed from ========== When receiving a key event on Windows, set host lock state to match client's BUG=176436 ========== to ========== When receiving a key event on Windows, set host lock state to match client's BUG=176436 Review-Url: https://codereview.chromium.org/2843213002 Cr-Commit-Position: refs/heads/master@{#468442} Committed: https://chromium.googlesource.com/chromium/src/+/cdd685a13fa2abf0fee9f2041cff... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:50001) as https://chromium.googlesource.com/chromium/src/+/cdd685a13fa2abf0fee9f2041cff... |
