Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(23)

Issue 2881293002: Remove duplicate Caps Lock handling (Closed)

Created:
3 years, 7 months ago by weidongg
Modified:
3 years, 6 months ago
Reviewers:
rkjnsn, Jamie
CC:
chromium-reviews, chromoting-reviews_chromium.org, afakhry
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Remove duplicate Caps Lock handling in CRD In Chrome Remote Desktop, original code tries to match the client's caps lock state with host's (Chrome OS) by toggling Caps Lock twice. But caps lock state switch is triggered in different ways in different systems. For example, Chrome OS triggers both ON and OFF on key release while Linux triggers ON on key press and OFF on key release. Let's assume client is Linux while host is Chrome OS. The caps lock state of both systems is initialized to OFF. If you tap caps lock key in client twice, the caps lock state at the time the key event is generated (not the time the key event takes effect) is: Press Release Press Release Client OFF ON ON ON Host OFF OFF ON ON If the caps lock state of client is OFF while that of host is ON, the state table will be: Press Release Press Release Client OFF ON ON ON Host ON ON OFF OFF So, if we trigger state toggling whenever state mismatches in client and host, then we may toggle duplicately. So the solution is to trigger toggling on key press when state mismatches. Also, we should not trigger toggling for key that is possibly mapped to caps lock in event rewriter. Otherwise, the toggling could occur duplicately. BUG=719996 TEST=NONE Review-Url: https://codereview.chromium.org/2881293002 Cr-Commit-Position: refs/heads/master@{#477110} Committed: https://chromium.googlesource.com/chromium/src/+/64dd0e1fab7201713af95e7293e428074ef8536c

Patch Set 1 #

Patch Set 2 : Remove duplicate Caps Lock handling in CRD #

Patch Set 3 : Exclude all keycode possibly mapped to caps lock #

Total comments: 2

Patch Set 4 : Include reference. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+32 lines, -8 lines) Patch
M remoting/host/input_injector_chromeos.cc View 1 2 3 4 chunks +32 lines, -8 lines 0 comments Download

Messages

Total messages: 37 (17 generated)
weidongg
3 years, 7 months ago (2017-05-15 23:58:16 UTC) #4
Jamie
I'll let Erik take a first pass at this, since he's more familiar with the ...
3 years, 7 months ago (2017-05-16 00:03:34 UTC) #6
rkjnsn
On 2017/05/16 00:03:34, Jamie wrote: > I'll let Erik take a first pass at this, ...
3 years, 7 months ago (2017-05-16 00:22:47 UTC) #7
weidongg
I updated the CL description. This new patch set fixes the problem for Caps Lock. ...
3 years, 7 months ago (2017-05-17 20:42:31 UTC) #11
rkjnsn
On 2017/05/17 20:42:31, weidongg wrote: > I updated the CL description. This new patch set ...
3 years, 6 months ago (2017-05-31 23:36:50 UTC) #12
rkjnsn
On 2017/05/31 23:36:50, rkjnsn wrote: > On 2017/05/17 20:42:31, weidongg wrote: > > I updated ...
3 years, 6 months ago (2017-05-31 23:49:54 UTC) #13
weidongg
On 2017/05/31 23:49:54, rkjnsn wrote: > On 2017/05/31 23:36:50, rkjnsn wrote: > > On 2017/05/17 ...
3 years, 6 months ago (2017-06-01 00:31:43 UTC) #14
rkjnsn
On 2017/06/01 00:31:43, weidongg wrote: > On 2017/05/31 23:49:54, rkjnsn wrote: > > On 2017/05/31 ...
3 years, 6 months ago (2017-06-01 01:33:43 UTC) #15
weidongg
On 2017/06/01 01:33:43, rkjnsn wrote: > On 2017/06/01 00:31:43, weidongg wrote: > > On 2017/05/31 ...
3 years, 6 months ago (2017-06-01 03:24:28 UTC) #16
Jamie
On 2017/06/01 03:24:28, weidongg wrote: > On 2017/06/01 01:33:43, rkjnsn wrote: > > On 2017/06/01 ...
3 years, 6 months ago (2017-06-01 16:24:36 UTC) #17
weidongg
On 2017/06/01 16:24:36, Jamie wrote: > On 2017/06/01 03:24:28, weidongg wrote: > > On 2017/06/01 ...
3 years, 6 months ago (2017-06-01 21:54:07 UTC) #21
rkjnsn
This approach seems reasonable. https://codereview.chromium.org/2881293002/diff/40001/remoting/host/input_injector_chromeos.cc File remoting/host/input_injector_chromeos.cc (right): https://codereview.chromium.org/2881293002/diff/40001/remoting/host/input_injector_chromeos.cc#newcode54 remoting/host/input_injector_chromeos.cc:54: // rewriter. Can you include ...
3 years, 6 months ago (2017-06-05 17:49:23 UTC) #26
weidongg
https://codereview.chromium.org/2881293002/diff/40001/remoting/host/input_injector_chromeos.cc File remoting/host/input_injector_chromeos.cc (right): https://codereview.chromium.org/2881293002/diff/40001/remoting/host/input_injector_chromeos.cc#newcode54 remoting/host/input_injector_chromeos.cc:54: // rewriter. On 2017/06/05 17:49:23, rkjnsn wrote: > Can ...
3 years, 6 months ago (2017-06-05 20:06:57 UTC) #27
Jamie
On 2017/06/05 20:06:57, weidongg wrote: > https://codereview.chromium.org/2881293002/diff/40001/remoting/host/input_injector_chromeos.cc > File remoting/host/input_injector_chromeos.cc (right): > > https://codereview.chromium.org/2881293002/diff/40001/remoting/host/input_injector_chromeos.cc#newcode54 > ...
3 years, 6 months ago (2017-06-05 20:24:39 UTC) #28
weidongg
On 2017/06/05 20:24:39, Jamie wrote: > On 2017/06/05 20:06:57, weidongg wrote: > > > https://codereview.chromium.org/2881293002/diff/40001/remoting/host/input_injector_chromeos.cc ...
3 years, 6 months ago (2017-06-05 21:49:27 UTC) #29
rkjnsn
On 2017/06/05 20:24:39, Jamie wrote: > Also, looking at that code, it seems that only ...
3 years, 6 months ago (2017-06-05 21:59:04 UTC) #30
Jamie
lgtm
3 years, 6 months ago (2017-06-05 22:04:55 UTC) #31
weidongg
Thanks for the review.
3 years, 6 months ago (2017-06-05 22:05:42 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2881293002/60001
3 years, 6 months ago (2017-06-05 22:06:33 UTC) #34
commit-bot: I haz the power
3 years, 6 months ago (2017-06-05 22:49:02 UTC) #37
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/64dd0e1fab7201713af95e7293e4...

Powered by Google App Engine
This is Rietveld 408576698