Fix the toggle caps lock accelerator not always working.

ash/accelerators/accelerator_controller_unittest.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 "ash/common/accelerators/accelerator_controller.h"
 
 #include "ash/common/accelerators/accelerator_table.h"
 #include "ash/common/accessibility_delegate.h"
 #include "ash/common/accessibility_types.h"
 #include "ash/common/ash_switches.h"
@@ skipping 20 matching lines @@
 #include "ash/wm/window_util.h"
 #include "base/command_line.h"
 #include "base/test/user_action_tester.cc"
 #include "services/ui/public/interfaces/window_manager_constants.mojom.h"
 #include "ui/app_list/presenter/app_list.h"
 #include "ui/app_list/presenter/test/test_app_list_presenter.h"
 #include "ui/aura/client/aura_constants.h"
 #include "ui/aura/test/test_window_delegate.h"
 #include "ui/aura/test/test_windows.h"
 #include "ui/aura/window.h"
+#include "ui/base/ime/chromeos/fake_ime_keyboard.h"
+#include "ui/base/ime/chromeos/ime_keyboard.h"
+#include "ui/base/ime/chromeos/input_method_manager.h"
+#include "ui/base/ime/chromeos/mock_input_method_manager.h"
 #include "ui/display/manager/display_manager.h"
 #include "ui/display/screen.h"
 #include "ui/events/event.h"
 #include "ui/events/event_processor.h"
 #include "ui/events/test/event_generator.h"
 #include "ui/message_center/message_center.h"
 #include "ui/views/widget/widget.h"
 
 #if defined(USE_X11)
 #include <X11/Xlib.h>
@@ skipping 1063 matching lines @@
   EXPECT_FALSE(
       GetController()->IsPreferred(ui::Accelerator(ui::VKEY_TAB, ui::EF_NONE)));
   EXPECT_FALSE(
       GetController()->IsReserved(ui::Accelerator(ui::VKEY_A, ui::EF_NONE)));
   EXPECT_FALSE(
       GetController()->IsPreferred(ui::Accelerator(ui::VKEY_A, ui::EF_NONE)));
 }
 
 namespace {
 
+class TestInputMethodManager
+    : public chromeos::input_method::MockInputMethodManager {
+ public:
+  TestInputMethodManager() = default;
+  ~TestInputMethodManager() override = default;
+
+  // MockInputMethodManager:
+  chromeos::input_method::ImeKeyboard* GetImeKeyboard() override {
+    return &keyboard_;
+  }
+
+ private:
+  chromeos::input_method::FakeImeKeyboard keyboard_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestInputMethodManager);
+};
+
+class ToggleCapsLockTest : public AcceleratorControllerTest {
+ public:
+  ToggleCapsLockTest() = default;
+  ~ToggleCapsLockTest() override = default;
+
+  void SetUp() override {
+    AcceleratorControllerTest::SetUp();
+    chromeos::input_method::InputMethodManager::Initialize(
+        new TestInputMethodManager);
+  }
+
+  void TearDown() override {
+    chromeos::input_method::InputMethodManager::Shutdown();
+    AcceleratorControllerTest::TearDown();
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ToggleCapsLockTest);
+};
+
+}  // namespace
+
+// Tests the four combinations of the TOGGLE_CAPS_LOCK accelerator.
+TEST_F(ToggleCapsLockTest, ToggleCapsLockAccelerators) {

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

nit: Could the test itself live in the anonymous namespace? Or does it use
friends that need to be non-anonymous?

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

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.

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

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.

+  chromeos::input_method::InputMethodManager* input_method_manager =
+      chromeos::input_method::InputMethodManager::Get();
+  ASSERT_TRUE(input_method_manager);
+  EXPECT_FALSE(input_method_manager->GetImeKeyboard()->CapsLockIsEnabled());
+
+  // 1. Press Alt, Press Search, Release Search, Release Alt.
+  EXPECT_FALSE(
+      ProcessInController(ui::Accelerator(ui::VKEY_MENU, ui::EF_COMMAND_DOWN)));
+  EXPECT_TRUE(
+      ProcessInController(ReleaseAccelerator(ui::VKEY_LWIN, ui::EF_ALT_DOWN)));
+  EXPECT_TRUE(input_method_manager->GetImeKeyboard()->CapsLockIsEnabled());
+  input_method_manager->GetImeKeyboard()->SetCapsLockEnabled(false);
+  EXPECT_FALSE(input_method_manager->GetImeKeyboard()->CapsLockIsEnabled());

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

nit: I don't think you need to check that caps-lock is disabled after you
explicitly disabled it. (and below)

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

Me being extra cautious! Done.

+
+  // 2. Press Search, Press Alt, Release Search, Release Alt.
+  EXPECT_FALSE(
+      ProcessInController(ui::Accelerator(ui::VKEY_LWIN, ui::EF_ALT_DOWN)));
+  EXPECT_TRUE(
+      ProcessInController(ReleaseAccelerator(ui::VKEY_LWIN, ui::EF_ALT_DOWN)));
+  EXPECT_TRUE(input_method_manager->GetImeKeyboard()->CapsLockIsEnabled());
+  input_method_manager->GetImeKeyboard()->SetCapsLockEnabled(false);
+  EXPECT_FALSE(input_method_manager->GetImeKeyboard()->CapsLockIsEnabled());
+
+  // 3. Press Alt, Press Search, Release Alt, Release Search.
+  EXPECT_FALSE(
+      ProcessInController(ui::Accelerator(ui::VKEY_MENU, ui::EF_COMMAND_DOWN)));

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

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.

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

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

+  EXPECT_TRUE(ProcessInController(
+      ReleaseAccelerator(ui::VKEY_MENU, ui::EF_COMMAND_DOWN)));
+  EXPECT_TRUE(input_method_manager->GetImeKeyboard()->CapsLockIsEnabled());
+  input_method_manager->GetImeKeyboard()->SetCapsLockEnabled(false);
+  EXPECT_FALSE(input_method_manager->GetImeKeyboard()->CapsLockIsEnabled());
+
+  // 4. Press Search, Press Alt, Release Alt, Release Search.
+  EXPECT_FALSE(
+      ProcessInController(ui::Accelerator(ui::VKEY_LWIN, ui::EF_ALT_DOWN)));
+  EXPECT_TRUE(ProcessInController(
+      ReleaseAccelerator(ui::VKEY_MENU, ui::EF_COMMAND_DOWN)));
+  EXPECT_TRUE(input_method_manager->GetImeKeyboard()->CapsLockIsEnabled());
+}

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

Nice test. I like how you documented each block.

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

Thanks.

+
+namespace {
+
 class PreferredReservedAcceleratorsTest : public test::AshTestBase {
  public:
   PreferredReservedAcceleratorsTest() {}
   ~PreferredReservedAcceleratorsTest() override {}
 
   // test::AshTestBase:
   void SetUp() override {
     AshTestBase::SetUp();
     Shell::GetInstance()->lock_state_controller()->set_animator_for_test(
         new test::TestSessionStateAnimator);
@@ skipping 308 matching lines @@
     // Expect no notifications from the new accelerators.
     EXPECT_TRUE(IsMessageCenterEmpty());
 
     // If the action is LOCK_SCREEN, we must reset the state by unlocking the
     // screen before we proceed testing the rest of accelerators.
     ResetStateIfNeeded();
   }
 }
 
 }  // namespace ash