Fix the toggle caps lock accelerator not always working.

Fix the toggle caps lock accelerator not always working.

There are four ways we can trigger this accelerator, out of which only the
first used to work:
1. Press Alt, Press Search, Release Search, Release Alt.
2. Press Search, Press Alt, Release Search, Release Alt.
3. Press Alt, Press Search, Release Alt, Release Search.
4. Press Search, Press Alt, Release Alt, Release Search.

BUG=686976
TEST=ash_unittests --gtest_filter=ToggleCapsLockTest.ToggleCapsLockAccelerators

Review-Url: https://codereview.chromium.org/2686833005
Cr-Commit-Position: refs/heads/master@{#449190}
Committed: https://chromium.googlesource.com/chromium/src/+/b93cbcadc5af7f922dd3ae2e379d39e06e7c4f93

[afakhry, 2017-02-08 21:26:13]

Description was changed from

==========
Fix the toggle caps lock accelerator not always working.

There are four ways we can trigger this accelerator, out of which only the
first used to work:
1. Press Alt, Press Search, Release Search, Release Alt.
2. Press Search, Press Alt, Release Search, Release Alt.
3. Press Alt, Press Search, Release Alt, Release Search.
4. Press Search, Press Alt, Release Alt, Release Search.

BUG=686976
TEST=ash_unittests
--gtest_filter=AcceleratorControllerTest.ToggleCapsLockAccelerators
==========

to

==========
Fix the toggle caps lock accelerator not always working.

There are four ways we can trigger this accelerator, out of which only the
first used to work:
1. Press Alt, Press Search, Release Search, Release Alt.
2. Press Search, Press Alt, Release Search, Release Alt.
3. Press Alt, Press Search, Release Alt, Release Search.
4. Press Search, Press Alt, Release Alt, Release Search.

BUG=686976
TEST=ash_unittests --gtest_filter=ToggleCapsLockTest.ToggleCapsLockAccelerators
==========

[afakhry, 2017-02-08 21:26:20]

Patchset #1 (id:1) has been deleted

[afakhry, 2017-02-08 21:29:19]

afakhry@chromium.org changed reviewers:
+ jamescook@chromium.org

[afakhry, 2017-02-08 21:29:20]

James, could you please take a look? Thanks.

[afakhry, 2017-02-08 21:29:32]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 21:30:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-08 22:33:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 22:33:23]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-02-08 22:39:08]

LGTM with nits.

Also, do you need to comment on crbug.com/166495 or crbug.com/223325 ?

https://codereview.chromium.org/2686833005/diff/20001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller_unittest.cc (right):

https://codereview.chromium.org/2686833005/diff/20001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_unittest.cc:1168:
TEST_F(ToggleCapsLockTest, ToggleCapsLockAccelerators) {
nit: Could the test itself live in the anonymous namespace? Or does it use
friends that need to be non-anonymous?

https://codereview.chromium.org/2686833005/diff/20001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_unittest.cc:1181:
EXPECT_FALSE(input_method_manager->GetImeKeyboard()->CapsLockIsEnabled());
nit: I don't think you need to check that caps-lock is disabled after you
explicitly disabled it. (and below)

https://codereview.chromium.org/2686833005/diff/20001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_unittest.cc:1194:
ProcessInController(ui::Accelerator(ui::VKEY_MENU, ui::EF_COMMAND_DOWN)));
optional: I wonder if this test would read better with local vars like
press_alt_search and press_search_alt. It might make the line wrapping better,
and would help readers who don't have the command-means-alt mapping firmly in
their head.

https://codereview.chromium.org/2686833005/diff/20001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_unittest.cc:1207: }
Nice test. I like how you documented each block.

https://codereview.chromium.org/2686833005/diff/20001/ash/common/accelerators...
File ash/common/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/2686833005/diff/20001/ash/common/accelerators...
ash/common/accelerators/accelerator_table.cc:88: {false, ui::VKEY_MENU,
ui::EF_COMMAND_DOWN, TOGGLE_CAPS_LOCK},
Could you add a comment here? This is super-confusing because EF_COMMAND_DOWN is
search but VKEY_MENU is sort-of Alt. Or add a comment in
ui/events/keycodes/keyboard_codes_posix.h or both.

[afakhry, 2017-02-09 00:28:30]

https://codereview.chromium.org/2686833005/diff/20001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller_unittest.cc (right):

https://codereview.chromium.org/2686833005/diff/20001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_unittest.cc:1168:
TEST_F(ToggleCapsLockTest, ToggleCapsLockAccelerators) {
On 2017/02/08 22:39:08, James Cook wrote:
> nit: Could the test itself live in the anonymous namespace? Or does it use
> friends that need to be non-anonymous?

The test can be placed in the anonymous namespace, but why is it better to do
so, especially that most other tests are not?

Done anyways.

https://codereview.chromium.org/2686833005/diff/20001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_unittest.cc:1181:
EXPECT_FALSE(input_method_manager->GetImeKeyboard()->CapsLockIsEnabled());
On 2017/02/08 22:39:08, James Cook wrote:
> nit: I don't think you need to check that caps-lock is disabled after you
> explicitly disabled it. (and below)

Me being extra cautious! Done.

https://codereview.chromium.org/2686833005/diff/20001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_unittest.cc:1194:
ProcessInController(ui::Accelerator(ui::VKEY_MENU, ui::EF_COMMAND_DOWN)));
On 2017/02/08 22:39:08, James Cook wrote:
> optional: I wonder if this test would read better with local vars like
> press_alt_search and press_search_alt. It might make the line wrapping better,
> and would help readers who don't have the command-means-alt mapping firmly in
> their head.

Great suggestion, actually it helped me fixed the order of the tests to match
the comments. Done.

https://codereview.chromium.org/2686833005/diff/20001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_unittest.cc:1207: }
On 2017/02/08 22:39:08, James Cook wrote:
> Nice test. I like how you documented each block.

Thanks.

https://codereview.chromium.org/2686833005/diff/20001/ash/common/accelerators...
File ash/common/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/2686833005/diff/20001/ash/common/accelerators...
ash/common/accelerators/accelerator_table.cc:88: {false, ui::VKEY_MENU,
ui::EF_COMMAND_DOWN, TOGGLE_CAPS_LOCK},
On 2017/02/08 22:39:08, James Cook wrote:
> Could you add a comment here? This is super-confusing because EF_COMMAND_DOWN
is
> search but VKEY_MENU is sort-of Alt. Or add a comment in
> ui/events/keycodes/keyboard_codes_posix.h or both.

Yes, it is confusing. VKEY_MENU is Alt as a key_code, but when used as a
modifier it becomes EF_ALT_DOWN. Similarly for Search, as a key_code it's
VKEY_LWIN, and as a modifier it's EF_COMMAND_DOWN.


Please take a look at the comments I added.

[James Cook, 2017-02-09 01:22:21]

still LGTM

https://codereview.chromium.org/2686833005/diff/20001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller_unittest.cc (right):

https://codereview.chromium.org/2686833005/diff/20001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_unittest.cc:1168:
TEST_F(ToggleCapsLockTest, ToggleCapsLockAccelerators) {
On 2017/02/09 00:28:30, afakhry wrote:
> On 2017/02/08 22:39:08, James Cook wrote:
> > nit: Could the test itself live in the anonymous namespace? Or does it use
> > friends that need to be non-anonymous?
> 
> The test can be placed in the anonymous namespace, but why is it better to do
> so, especially that most other tests are not?
> 
> Done anyways.

Putting it in an anonymous namespace reduces the amount of work the linker has
to do when resolving symbols, so linking ash_unittests might be a little faster.
I wish we did it for more tests. But you can't do it if you use FRIEND_TEST.

https://codereview.chromium.org/2686833005/diff/20001/ash/common/accelerators...
File ash/common/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/2686833005/diff/20001/ash/common/accelerators...
ash/common/accelerators/accelerator_table.cc:88: {false, ui::VKEY_MENU,
ui::EF_COMMAND_DOWN, TOGGLE_CAPS_LOCK},
On 2017/02/09 00:28:30, afakhry wrote:
> On 2017/02/08 22:39:08, James Cook wrote:
> > Could you add a comment here? This is super-confusing because
EF_COMMAND_DOWN
> is
> > search but VKEY_MENU is sort-of Alt. Or add a comment in
> > ui/events/keycodes/keyboard_codes_posix.h or both.
> 
> Yes, it is confusing. VKEY_MENU is Alt as a key_code, but when used as a
> modifier it becomes EF_ALT_DOWN. Similarly for Search, as a key_code it's
> VKEY_LWIN, and as a modifier it's EF_COMMAND_DOWN.
> 
> 
> Please take a look at the comments I added.

Nice. Well written.

[afakhry, 2017-02-09 01:27:57]

The CQ bit was checked by afakhry@chromium.org

[commit-bot: I haz the power, 2017-02-09 01:28:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 02:14:19]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486603677627120,
"parent_rev": "a8c0d87fb51b053a25e3cebfde5ccbd94a24f10c", "commit_rev":
"b93cbcadc5af7f922dd3ae2e379d39e06e7c4f93"}

[commit-bot: I haz the power, 2017-02-09 02:14:57]

Description was changed from

==========
Fix the toggle caps lock accelerator not always working.

There are four ways we can trigger this accelerator, out of which only the
first used to work:
1. Press Alt, Press Search, Release Search, Release Alt.
2. Press Search, Press Alt, Release Search, Release Alt.
3. Press Alt, Press Search, Release Alt, Release Search.
4. Press Search, Press Alt, Release Alt, Release Search.

BUG=686976
TEST=ash_unittests --gtest_filter=ToggleCapsLockTest.ToggleCapsLockAccelerators
==========

to

==========
Fix the toggle caps lock accelerator not always working.

There are four ways we can trigger this accelerator, out of which only the
first used to work:
1. Press Alt, Press Search, Release Search, Release Alt.
2. Press Search, Press Alt, Release Search, Release Alt.
3. Press Alt, Press Search, Release Alt, Release Search.
4. Press Search, Press Alt, Release Alt, Release Search.

BUG=686976
TEST=ash_unittests --gtest_filter=ToggleCapsLockTest.ToggleCapsLockAccelerators

Review-Url: https://codereview.chromium.org/2686833005
Cr-Commit-Position: refs/heads/master@{#449190}
Committed:
https://chromium.googlesource.com/chromium/src/+/b93cbcadc5af7f922dd3ae2e379d...
==========

[commit-bot: I haz the power, 2017-02-09 02:14:58]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/b93cbcadc5af7f922dd3ae2e379d...