Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2763483002:
Fix Caps Lock bug (Closed)
DescriptionFix Caps Lock bug
Move the handling of Caps Lock key event to accelerator controller and trigger toggling on key-release. Remove the handling code of Caps Lock for X11 from InputMethodChromeOS::DispatchKeyEvent, because it is now handled only in accelerator controller. This should also work for both x11+ozone environment and x11 environment on desktop.
BUG=700705
Review-Url: https://codereview.chromium.org/2763483002
Cr-Commit-Position: refs/heads/master@{#464499}
Committed: https://chromium.googlesource.com/chromium/src/+/863b0d4dbe21b493e2dfa59c6d52e02a1af73565
Patch Set 1 #Patch Set 2 : q #
Total comments: 1
Patch Set 3 : On branch mychange Your branch is up-to-date with 'origin/master'. #Patch Set 4 : Fix continous toggling of Caps Lock key. #Patch Set 5 : Change unit test #Patch Set 6 : Add unit test for Caps Lock in accelerator. #
Total comments: 6
Patch Set 7 : Applying the fix #
Total comments: 7
Patch Set 8 : Remove Caps Lock handling for X11 case #Patch Set 9 : Failure due to file directory change, apply the fix #
Messages
Total messages: 71 (44 generated)
Description was changed from ========== Fix Caps Lock bug Toggle Caps Lock only if its previous key event is ET_KEY_RELEASED. Avoid continuous toggles when the key is hold down. On branch mychange Your branch is up-to-date with 'origin/master'. Changes to be committed: modified: chrome/browser/chromeos/events/event_rewriter.cc BUG=700705 ========== to ========== Fix Caps Lock bug Toggle Caps Lock only if its previous key event is ET_KEY_RELEASED. Avoid continuous toggles when the key is hold down. On branch mychange Your branch is up-to-date with 'origin/master'. Changes to be committed: modified: chrome/browser/chromeos/events/event_rewriter.cc BUG=700705 ==========
weidongg@chromium.org changed reviewers: + afakhry@chromium.org
The CQ bit was checked by weidongg@chromium.org
The CQ bit was unchecked by weidongg@chromium.org
The CQ bit was checked by weidongg@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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by afakhry@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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
I also kicked off a CQ-dryrun to see if any tests would catch any bugs. https://codereview.chromium.org/2763483002/diff/20001/ui/events/event.cc File ui/events/event.cc (right): https://codereview.chromium.org/2763483002/diff/20001/ui/events/event.cc#newc... ui/events/event.cc:1163: ~ui::EF_CAPS_LOCK_ON) && This doesn't look right. Clearing the CAPS LOCK flag on both sides of the == to make them equal when they're not seems weird to me. Let's talk about this in person as to why you need this.
I removed the handling part in RewriteModifierKeys and added that into accelerator controller. It works well in my machine.
The CQ bit was checked by weidongg@chromium.org to run a CQ dry run
The CQ bit was checked by weidongg@chromium.org
The CQ bit was unchecked by weidongg@chromium.org
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 checked by weidongg@chromium.org
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by weidongg@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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/03/29 05:02:18, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) You need to modify the following tests: EventRewriterTest.TestRewriteModifiersRemapToCapsLock EventRewriterTest.TestRewriteCapsLock Also add reviewers from the OWNERS of the files you are modifying.
The CQ bit was checked by weidongg@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...
weidongg@chromium.org changed reviewers: + oshima@chromium.org
oshima@chromium.org: Please review changes in Caps Lock key related files.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Just uploaded a new patch.
Description was changed from ========== Fix Caps Lock bug Toggle Caps Lock only if its previous key event is ET_KEY_RELEASED. Avoid continuous toggles when the key is hold down. On branch mychange Your branch is up-to-date with 'origin/master'. Changes to be committed: modified: chrome/browser/chromeos/events/event_rewriter.cc BUG=700705 ========== to ========== Fix Caps Lock bug Move the handling of Caps Lock key event to accelerator_controller and trigger toggling on key-release. BUG=700705 ==========
The CQ bit was checked by afakhry@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...
I triggered a CQ dry-run. I think the newly added test here: https://codereview.chromium.org/2802583002/ to catch new accelerators that are not search-based will fail. In this case we would want to update the test too as part of this CL. https://codereview.chromium.org/2763483002/diff/100001/ash/accelerators/accel... File ash/accelerators/accelerator_controller_unittest.cc (right): https://codereview.chromium.org/2763483002/diff/100001/ash/accelerators/accel... ash/accelerators/accelerator_controller_unittest.cc:1027: EXPECT_FALSE(ProcessInController(press_caps_lock)); Can you check here the status of the caps lock hasn't changed? Just to make sure it triggers only on release. https://codereview.chromium.org/2763483002/diff/100001/ash/common/accelerator... File ash/common/accelerators/accelerator_controller.cc (right): https://codereview.chromium.org/2763483002/diff/100001/ash/common/accelerator... ash/common/accelerators/accelerator_controller.cc:589: return ime && ime->GetImeKeyboard(); Can we move the check for |ime| is not null up to be the first thing? It's better than repeating it several times. https://codereview.chromium.org/2763483002/diff/100001/ash/common/accelerator... File ash/common/accelerators/accelerator_table.cc (right): https://codereview.chromium.org/2763483002/diff/100001/ash/common/accelerator... ash/common/accelerators/accelerator_table.cc:95: // was rewrote by event rewriter if Caps Lock is mapped from another rewrote --> rewritten. However, you don't need this comment. It's kind of known already that we can map other keys to caps lock.
https://codereview.chromium.org/2763483002/diff/100001/ash/accelerators/accel... File ash/accelerators/accelerator_controller_unittest.cc (right): https://codereview.chromium.org/2763483002/diff/100001/ash/accelerators/accel... ash/accelerators/accelerator_controller_unittest.cc:1027: EXPECT_FALSE(ProcessInController(press_caps_lock)); On 2017/04/07 23:31:42, afakhry wrote: > Can you check here the status of the caps lock hasn't changed? Just to make sure > it triggers only on release. Done. https://codereview.chromium.org/2763483002/diff/100001/ash/common/accelerator... File ash/common/accelerators/accelerator_controller.cc (right): https://codereview.chromium.org/2763483002/diff/100001/ash/common/accelerator... ash/common/accelerators/accelerator_controller.cc:589: return ime && ime->GetImeKeyboard(); On 2017/04/07 23:31:42, afakhry wrote: > Can we move the check for |ime| is not null up to be the first thing? It's > better than repeating it several times. Sure, you are right. https://codereview.chromium.org/2763483002/diff/100001/ash/common/accelerator... File ash/common/accelerators/accelerator_table.cc (right): https://codereview.chromium.org/2763483002/diff/100001/ash/common/accelerator... ash/common/accelerators/accelerator_table.cc:95: // was rewrote by event rewriter if Caps Lock is mapped from another On 2017/04/07 23:31:42, afakhry wrote: > rewrote --> rewritten. > > However, you don't need this comment. It's kind of known already that we can map > other keys to caps lock. Deleted.
@oshima friendly ping.
https://codereview.chromium.org/2763483002/diff/120001/ash/common/accelerator... File ash/common/accelerators/accelerator_controller.cc (right): https://codereview.chromium.org/2763483002/diff/120001/ash/common/accelerator... ash/common/accelerators/accelerator_controller.cc:555: return false; when ime is null? https://codereview.chromium.org/2763483002/diff/120001/ash/common/accelerator... File ash/common/accelerators/accelerator_table_unittest.cc (right): https://codereview.chromium.org/2763483002/diff/120001/ash/common/accelerator... ash/common/accelerators/accelerator_table_unittest.cc:18: // The number of non-Search-based accelerators as of 2017-04-06. update the comment https://codereview.chromium.org/2763483002/diff/120001/ui/chromeos/events/eve... File ui/chromeos/events/event_rewriter_chromeos.cc (left): https://codereview.chromium.org/2763483002/diff/120001/ui/chromeos/events/eve... ui/chromeos/events/event_rewriter_chromeos.cc:816: #endif Did you consult with the author of this code? Please include the resolution in the bug description as well as comment in the code. (e.g. This doesn't work on X11 because .....)
kpschoedel@chromium.org changed reviewers: + kpschoedel@chromium.org
https://codereview.chromium.org/2763483002/diff/120001/ui/chromeos/events/eve... File ui/chromeos/events/event_rewriter_chromeos.cc (left): https://codereview.chromium.org/2763483002/diff/120001/ui/chromeos/events/eve... ui/chromeos/events/event_rewriter_chromeos.cc:816: #endif (original author) I believe this can be removed because all ChromeOS platforms now use Ozone/Freon and the X11 case is no longer used.
https://codereview.chromium.org/2763483002/diff/120001/ui/chromeos/events/eve... File ui/chromeos/events/event_rewriter_chromeos.cc (left): https://codereview.chromium.org/2763483002/diff/120001/ui/chromeos/events/eve... ui/chromeos/events/event_rewriter_chromeos.cc:816: #endif On 2017/04/11 21:56:00, kpschoedel wrote: > (original author) I believe this can be removed because all ChromeOS platforms > now use Ozone/Freon and the X11 case is no longer used. Many ChromeOS UI devs are still using linux+x11 desktop environment to develop and test UI, so it's good to mention that this won't work on x11 somewhere.
https://codereview.chromium.org/2763483002/diff/120001/ash/common/accelerator... File ash/common/accelerators/accelerator_controller.cc (right): https://codereview.chromium.org/2763483002/diff/120001/ash/common/accelerator... ash/common/accelerators/accelerator_controller.cc:555: return false; On 2017/04/11 19:04:47, oshima wrote: > when ime is null? |ime| is checked below at |return| multiple times. Moving the check for |ime| is not null up to be the first thing seems to be better than repeating it several times. https://codereview.chromium.org/2763483002/diff/120001/ui/chromeos/events/eve... File ui/chromeos/events/event_rewriter_chromeos.cc (left): https://codereview.chromium.org/2763483002/diff/120001/ui/chromeos/events/eve... ui/chromeos/events/event_rewriter_chromeos.cc:816: #endif On 2017/04/11 22:24:54, oshima wrote: > On 2017/04/11 21:56:00, kpschoedel wrote: > > (original author) I believe this can be removed because all ChromeOS platforms > > now use Ozone/Freon and the X11 case is no longer used. > > Many ChromeOS UI devs are still using linux+x11 desktop environment to develop > and test UI, so it's good to mention that this won't work on x11 somewhere. I am sorry, I am still confused about these terms. X11 is displaying server only used for build running on desktop? Ozone is abstraction layer for low level input and it is only used in chromebook device? What is linux+x11 desktop environment? Is this like a virtual machine that runs chrome build?
Description was changed from ========== Fix Caps Lock bug Move the handling of Caps Lock key event to accelerator_controller and trigger toggling on key-release. BUG=700705 ========== to ========== Fix Caps Lock bug Move the handling of Caps Lock key event to accelerator controller and trigger toggling on key-release. Since we now handle the toggling in accelerator controller, we can remove the handling of Caps Lock from InputMethodChromeOS::DispatchKeyEvent which was written for X11 case. BUG=700705 ==========
weidongg@chromium.org changed reviewers: + shuchen@chromium.org
shuchen@chromium.org: Please review changes in ui/base/ime/input_method_chromeos.cc
lgtm for ui/base/ime/...
lgtm. Please wait for Oshima's approval.
On 2017/04/11 22:24:54, oshima wrote: > https://codereview.chromium.org/2763483002/diff/120001/ui/chromeos/events/eve... > File ui/chromeos/events/event_rewriter_chromeos.cc (left): > > https://codereview.chromium.org/2763483002/diff/120001/ui/chromeos/events/eve... > ui/chromeos/events/event_rewriter_chromeos.cc:816: #endif > On 2017/04/11 21:56:00, kpschoedel wrote: > > (original author) I believe this can be removed because all ChromeOS platforms > > now use Ozone/Freon and the X11 case is no longer used. > > Many ChromeOS UI devs are still using linux+x11 desktop environment to develop > and test UI, so it's good to mention that this won't work on x11 somewhere. By the way, will this work on x11+ozone environment?
On 2017/04/13 01:17:17, oshima wrote: > On 2017/04/11 22:24:54, oshima wrote: > > > https://codereview.chromium.org/2763483002/diff/120001/ui/chromeos/events/eve... > > File ui/chromeos/events/event_rewriter_chromeos.cc (left): > > > > > https://codereview.chromium.org/2763483002/diff/120001/ui/chromeos/events/eve... > > ui/chromeos/events/event_rewriter_chromeos.cc:816: #endif > > On 2017/04/11 21:56:00, kpschoedel wrote: > > > (original author) I believe this can be removed because all ChromeOS > platforms > > > now use Ozone/Freon and the X11 case is no longer used. > > > > Many ChromeOS UI devs are still using linux+x11 desktop environment to develop > > and test UI, so it's good to mention that this won't work on x11 somewhere. > > By the way, will this work on x11+ozone environment? Before this patch, the caps lock toggling happens both on key-down (handled by x11) and key-up (accelerator controller) in x11+ozone environment. With this patch, caps lock toggling is only handled by accelerator now. So it works well for x11+ozone environment.
Description was changed from ========== Fix Caps Lock bug Move the handling of Caps Lock key event to accelerator controller and trigger toggling on key-release. Since we now handle the toggling in accelerator controller, we can remove the handling of Caps Lock from InputMethodChromeOS::DispatchKeyEvent which was written for X11 case. BUG=700705 ========== to ========== Fix Caps Lock bug Move the handling of Caps Lock key event to accelerator controller and trigger toggling on key-release. Remove the handling code of Caps Lock for X11 from InputMethodChromeOS::DispatchKeyEvent, because it is now handled only in accelerator controller. This should also work for both x11+ozone environment and x11 environment on desktop. BUG=700705 ==========
Updated the description.
lgtm
The CQ bit was checked by weidongg@chromium.org
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
Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by weidongg@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 weidongg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from afakhry@chromium.org, shuchen@chromium.org, oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2763483002/#ps160001 (title: "Failure due to file directory change, apply the fix")
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": 160001, "attempt_start_ts": 1492110366458570,
"parent_rev": "06114d78b56a1f73eeede3514c06bcb0aa33a942", "commit_rev":
"863b0d4dbe21b493e2dfa59c6d52e02a1af73565"}
Message was sent while issue was closed.
Description was changed from ========== Fix Caps Lock bug Move the handling of Caps Lock key event to accelerator controller and trigger toggling on key-release. Remove the handling code of Caps Lock for X11 from InputMethodChromeOS::DispatchKeyEvent, because it is now handled only in accelerator controller. This should also work for both x11+ozone environment and x11 environment on desktop. BUG=700705 ========== to ========== Fix Caps Lock bug Move the handling of Caps Lock key event to accelerator controller and trigger toggling on key-release. Remove the handling code of Caps Lock for X11 from InputMethodChromeOS::DispatchKeyEvent, because it is now handled only in accelerator controller. This should also work for both x11+ozone environment and x11 environment on desktop. BUG=700705 Review-Url: https://codereview.chromium.org/2763483002 Cr-Commit-Position: refs/heads/master@{#464499} Committed: https://chromium.googlesource.com/chromium/src/+/863b0d4dbe21b493e2dfa59c6d52... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/863b0d4dbe21b493e2dfa59c6d52... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
