chromeos: Display Caps Lock setting for non-CrOS keyboards.

Let the user remap the Caps Lock key when
--has-chromeos-keyboard isn't provided (i.e. the internal
keyboard has a Caps Lock key in place of Search). Formerly,
the setting was only shown when an external keyboard was
connected.

Also add unit tests for KeyboardHandler and delete some
unused variables. Unrelatedly, drop an accidentally-repeated
call in PowerHandlerTest.

BUG=742613

Change-Id: I56d7ccb0bd5e054b9e815fb4d8d457c0729ada0c
Reviewed-on: https://chromium-review.googlesource.com/572209Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Commit-Queue: Dan Erat <derat@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487298}

chrome/browser/chromeos/BUILD.gn

@@ -1887,6 +1887,7 @@ source_set("unit_tests") {
     "../ui/webui/chromeos/login/l10n_util_unittest.cc",
     "../ui/webui/chromeos/login/oobe_display_chooser_unittest.cc",
     "../ui/webui/chromeos/login/signin_userlist_unittest.cc",
+    "../ui/webui/settings/chromeos/device_keyboard_handler_unittest.cc",
     "../ui/webui/settings/chromeos/device_power_handler_unittest.cc",
     "../ui/webui/settings/chromeos/easy_unlock_settings_handler_unittest.cc",
     "//components/drive/change_list_loader_unittest.cc",

chrome/browser/chromeos/login/chrome_restart_request.cc

@@ -214,6 +214,7 @@ void DeriveCommandLine(const GURL& start_url,
     chromeos::switches::kEnterpriseDisableArc,
     chromeos::switches::kEnterpriseEnableForcedReEnrollment,
     chromeos::switches::kHasChromeOSDiamondKey,
+    chromeos::switches::kHasChromeOSKeyboard,
     chromeos::switches::kLoginProfile,
     chromeos::switches::kNaturalScrollDefault,
     chromeos::switches::kShowMdLogin,

chrome/browser/ui/webui/settings/chromeos/device_keyboard_handler.cc

@@ -9,7 +9,6 @@
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/values.h"
-#include "chrome/browser/profiles/profile.h"
 #include "chromeos/chromeos_switches.h"
 #include "content/public/browser/web_ui.h"
 #include "ui/events/devices/input_device_manager.h"
@@ -31,12 +30,17 @@ bool HasExternalKeyboard() {
 namespace chromeos {
 namespace settings {
 
-KeyboardHandler::KeyboardHandler(content::WebUI* webui)
-    : profile_(Profile::FromWebUI(webui)), observer_(this) {}
+const char KeyboardHandler::kShowKeysChangedName[] = "show-keys-changed";
 
-KeyboardHandler::~KeyboardHandler() {
+void KeyboardHandler::TestAPI::Initialize() {
+  base::ListValue args;
+  handler_->HandleInitialize(&args);
 }
 
+KeyboardHandler::KeyboardHandler() : observer_(this) {}
+
+KeyboardHandler::~KeyboardHandler() = default;
+
 void KeyboardHandler::RegisterMessages() {
   web_ui()->RegisterMessageCallback(
       "initializeKeyboardSettings",
@@ -71,11 +75,16 @@ void KeyboardHandler::HandleShowKeyboardShortcutsOverlay(
 }
 
 void KeyboardHandler::UpdateShowKeys() {
-  const base::Value has_caps_lock(HasExternalKeyboard());
+  // kHasChromeOSKeyboard will be unset on Chromebooks that have standalone Caps
+  // Lock keys.
+  const base::Value has_caps_lock(
+      HasExternalKeyboard() ||
+      !base::CommandLine::ForCurrentProcess()->HasSwitch(
+          chromeos::switches::kHasChromeOSKeyboard));
   const base::Value has_diamond_key(
       base::CommandLine::ForCurrentProcess()->HasSwitch(
           chromeos::switches::kHasChromeOSDiamondKey));
-  FireWebUIListener("show-keys-changed", has_caps_lock, has_diamond_key);
+  FireWebUIListener(kShowKeysChangedName, has_caps_lock, has_diamond_key);
 }
 
 }  // namespace settings

chrome/browser/ui/webui/settings/chromeos/device_keyboard_handler.h

@@ -14,16 +14,10 @@ namespace base {
 class ListValue;
 }
 
-namespace content {
-class WebUI;
-}
-
 namespace ui {
 class InputDeviceManager;
 }
 
-class Profile;
-
 namespace chromeos {
 namespace settings {
 
@@ -32,7 +26,24 @@ class KeyboardHandler
     : public ::settings::SettingsPageUIHandler,
       public ui::InputDeviceEventObserver {
  public:
-  explicit KeyboardHandler(content::WebUI* webui);
+  // Name of the message sent to WebUI when the keys that should be shown
+  // change.
+  static const char kShowKeysChangedName[];
+
+  // Class used by tests to interact with KeyboardHandler internals.
+  class TestAPI {
+   public:
+    explicit TestAPI(KeyboardHandler* handler) { handler_ = handler; }
+
+    // Simulates a request from WebUI to initialize the page.
+    void Initialize();
+
+   private:
+    KeyboardHandler* handler_;  // Not owned.
+    DISALLOW_COPY_AND_ASSIGN(TestAPI);
+  };
+
+  KeyboardHandler();
   ~KeyboardHandler() override;
 
   // SettingsPageUIHandler implementation.
@@ -54,8 +65,6 @@ class KeyboardHandler
   // system status.
   void UpdateShowKeys();
 
-  Profile* profile_;  // Weak pointer.
-
   ScopedObserver<ui::InputDeviceManager, KeyboardHandler> observer_;
 
   DISALLOW_COPY_AND_ASSIGN(KeyboardHandler);

chrome/browser/ui/webui/settings/chromeos/device_keyboard_handler_unittest.cc

+// Copyright 2017 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 "chrome/browser/ui/webui/settings/chromeos/device_keyboard_handler.h"
+
+#include <memory>
+
+#include "base/command_line.h"
+#include "base/logging.h"
+#include "base/macros.h"
+#include "base/memory/ptr_util.h"
+#include "base/observer_list.h"
+#include "chromeos/chromeos_switches.h"
+#include "content/public/test/test_web_ui.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/events/devices/input_device.h"
+#include "ui/events/test/device_data_manager_test_api.h"
+
+namespace chromeos {
+namespace settings {
+
+namespace {
+
+class TestKeyboardHandler : public KeyboardHandler {
+ public:
+  // Pull WebUIMessageHandler::set_web_ui() into public so tests can call it.
+  using KeyboardHandler::set_web_ui;
+};
+
+}  // namespace
+
+class KeyboardHandlerTest : public testing::Test {
+ public:
+  KeyboardHandlerTest() : handler_test_api_(&handler_) {
+    handler_.set_web_ui(&web_ui_);
+    handler_.RegisterMessages();
+    handler_.AllowJavascriptForTesting();
+
+    // Make sure that we start out without any keyboards reported.
+    device_test_api_.SetKeyboardDevices(std::vector<ui::InputDevice>());
+  }
+
+ protected:
+  // Updates out-params from the last message sent to WebUI about a change to
+  // which keys should be shown. False is returned if the message was invalid or
+  // not found.
+  bool GetLastShowKeysChangedMessage(bool* has_caps_lock_out,
+                                     bool* has_diamond_key_out)
+      WARN_UNUSED_RESULT {
+    for (auto it = web_ui_.call_data().rbegin();
+         it != web_ui_.call_data().rend(); ++it) {
+      const content::TestWebUI::CallData* data = it->get();
+      std::string name;
+      if (data->function_name() != "cr.webUIListenerCallback" ||
+          !data->arg1()->GetAsString(&name) ||
+          name != KeyboardHandler::kShowKeysChangedName) {
+        continue;
+      }
+      return data->arg2()->GetAsBoolean(has_caps_lock_out) &&
+             data->arg3()->GetAsBoolean(has_diamond_key_out);
+    }
+    return false;
+  }
+
+  // Returns true if the last keys-changed message reported that a Caps Lock key
+  // is present and false otherwise. A failure is added if a message wasn't
+  // found.
+  bool HasCapsLock() {
+    bool has_caps_lock = false, has_diamond_key = false;
+    if (!GetLastShowKeysChangedMessage(&has_caps_lock, &has_diamond_key)) {
+      ADD_FAILURE() << "Didn't get " << KeyboardHandler::kShowKeysChangedName;
+      return false;
+    }
+    return has_caps_lock;
+  }
+
+  // Returns true if the last keys-changed message reported that a "diamond" key
+  // is present and false otherwise. A failure is added if a message wasn't
+  // found.
+  bool HasDiamondKey() {
+    bool has_caps_lock = false, has_diamond_key = false;
+    if (!GetLastShowKeysChangedMessage(&has_caps_lock, &has_diamond_key)) {
+      ADD_FAILURE() << "Didn't get " << KeyboardHandler::kShowKeysChangedName;
+      return false;
+    }
+    return has_diamond_key;
+  }
+
+  ui::test::DeviceDataManagerTestAPI device_test_api_;
+  content::TestWebUI web_ui_;
+  TestKeyboardHandler handler_;
+  KeyboardHandler::TestAPI handler_test_api_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(KeyboardHandlerTest);
+};
+
+TEST_F(KeyboardHandlerTest, DefaultKeys) {
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      chromeos::switches::kHasChromeOSKeyboard);
+  handler_test_api_.Initialize();
+  EXPECT_FALSE(HasCapsLock());
+  EXPECT_FALSE(HasDiamondKey());
+}
+
+TEST_F(KeyboardHandlerTest, NonChromeOSKeyboard) {
+  // If kHasChromeOSKeyboard isn't passed, we should assume there's a Caps Lock
+  // key.
+  handler_test_api_.Initialize();
+  EXPECT_TRUE(HasCapsLock());
+  EXPECT_FALSE(HasDiamondKey());
+}
+
+TEST_F(KeyboardHandlerTest, ExternalKeyboard) {
+  // An internal keyboard shouldn't change the defaults.
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      chromeos::switches::kHasChromeOSKeyboard);
+  device_test_api_.SetKeyboardDevices(std::vector<ui::InputDevice>{
+      {1, ui::INPUT_DEVICE_INTERNAL, "internal keyboard"}});
+  handler_test_api_.Initialize();
+  EXPECT_FALSE(HasCapsLock());
+  EXPECT_FALSE(HasDiamondKey());
+
+  // Simulate an external keyboard being connected. We should assume there's a
+  // Caps Lock key now.
+  device_test_api_.SetKeyboardDevices(std::vector<ui::InputDevice>{
+      {1, ui::INPUT_DEVICE_EXTERNAL, "external keyboard"}});
+  device_test_api_.NotifyObserversKeyboardDeviceConfigurationChanged();
+  EXPECT_TRUE(HasCapsLock());
+  EXPECT_FALSE(HasDiamondKey());
+
+  // Disconnect the external keyboard and check that the key goes away.
+  device_test_api_.SetKeyboardDevices(std::vector<ui::InputDevice>());
+  device_test_api_.NotifyObserversKeyboardDeviceConfigurationChanged();
+  EXPECT_FALSE(HasCapsLock());
+  EXPECT_FALSE(HasDiamondKey());
+}
+
+TEST_F(KeyboardHandlerTest, DiamondKey) {
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      chromeos::switches::kHasChromeOSKeyboard);
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      chromeos::switches::kHasChromeOSDiamondKey);
+  handler_test_api_.Initialize();
+  EXPECT_FALSE(HasCapsLock());
+  EXPECT_TRUE(HasDiamondKey());
+}
+
+}  // namespace settings
+}  // namespace chromeos

chrome/browser/ui/webui/settings/chromeos/device_power_handler_unittest.cc

@@ -70,7 +70,6 @@ class PowerHandlerTest : public testing::Test {
       const content::TestWebUI::CallData* data = it->get();
       std::string name;
       const base::DictionaryValue* dict = nullptr;
-      data->arg1()->GetAsString(&name);
       if (data->function_name() != "cr.webUIListenerCallback" ||
           !data->arg1()->GetAsString(&name) ||
           name != PowerHandler::kPowerManagementSettingsChangedName) {

chrome/browser/ui/webui/settings/md_settings_ui.cc

@@ -165,7 +165,7 @@ MdSettingsUI::MdSettingsUI(content::WebUI* web_ui, const GURL& url)
   AddSettingsPageUIHandler(
       base::MakeUnique<chromeos::settings::FingerprintHandler>(profile));
   AddSettingsPageUIHandler(
-      base::MakeUnique<chromeos::settings::KeyboardHandler>(web_ui));
+      base::MakeUnique<chromeos::settings::KeyboardHandler>());
   AddSettingsPageUIHandler(
       base::MakeUnique<chromeos::settings::PointerHandler>());
   AddSettingsPageUIHandler(

chromeos/chromeos_switches.cc

@@ -374,6 +374,13 @@ const char kGuestWallpaperSmall[] = "guest-wallpaper-small";
 // for any type of user. Should be used only for testing.
 const char kForceHappinessTrackingSystem[] = "force-happiness-tracking-system";
 
+// If set, the system is a Chromebook with a "standard Chrome OS keyboard",
+// which generally means one with a Search key in the standard Caps Lock
+// location above the Left Shift key. It should be unset for Chromebooks with
+// both Search and Caps Lock keys (e.g. stout) and for devices like Chromeboxes
+// that only use external keyboards.
+const char kHasChromeOSKeyboard[] = "has-chromeos-keyboard";
+
 // If true, the Chromebook has a keyboard with a diamond key.
 const char kHasChromeOSDiamondKey[] = "has-chromeos-diamond-key";
 

chromeos/chromeos_switches.h

@@ -114,6 +114,7 @@ CHROMEOS_EXPORT extern const char kGuestWallpaperLarge[];
 CHROMEOS_EXPORT extern const char kGuestWallpaperSmall[];
 CHROMEOS_EXPORT extern const char kForceHappinessTrackingSystem[];
 CHROMEOS_EXPORT extern const char kHasChromeOSDiamondKey[];
+CHROMEOS_EXPORT extern const char kHasChromeOSKeyboard[];
 CHROMEOS_EXPORT extern const char kHomedir[];
 CHROMEOS_EXPORT extern const char kHostPairingOobe[];
 CHROMEOS_EXPORT extern const char kIgnoreUserProfileMappingForTests[];

ui/events/test/device_data_manager_test_api.h

@@ -6,6 +6,7 @@
 #define UI_EVENTS_TEST_DEVICE_DATA_MANAGER_TEST_API_H_
 
 #include <memory>
+#include <vector>
 
 #include "base/macros.h"
 #include "ui/events/devices/events_devices_export.h"
@@ -14,6 +15,8 @@ namespace ui {
 
 class DeviceDataManager;
 enum class StylusState;
+struct InputDevice;
+struct TouchscreenDevice;
 
 namespace test {
 
@@ -34,6 +37,13 @@ class DeviceDataManagerTestAPI {
   void NotifyObserversStylusStateChanged(StylusState stylus_state);
   void OnDeviceListsComplete();
 
+  // Methods for updating DeviceDataManager's device lists. Notify* methods must
+  // be invoked separately to notify observers after making changes.
+  void SetTouchscreenDevices(const std::vector<TouchscreenDevice>& devices);
+  void SetKeyboardDevices(const std::vector<InputDevice>& devices);
+  void SetMouseDevices(const std::vector<InputDevice>& devices);
+  void SetTouchpadDevices(const std::vector<InputDevice>& devices);
+
  private:
   DISALLOW_COPY_AND_ASSIGN(DeviceDataManagerTestAPI);
 };

ui/events/test/device_data_manager_test_api_impl.cc

@@ -53,5 +53,25 @@ void DeviceDataManagerTestAPI::OnDeviceListsComplete() {
   DeviceDataManager::GetInstance()->OnDeviceListsComplete();
 }
 
+void DeviceDataManagerTestAPI::SetTouchscreenDevices(
+    const std::vector<TouchscreenDevice>& devices) {
+  DeviceDataManager::GetInstance()->touchscreen_devices_ = devices;
+}
+
+void DeviceDataManagerTestAPI::SetKeyboardDevices(
+    const std::vector<InputDevice>& devices) {
+  DeviceDataManager::GetInstance()->keyboard_devices_ = devices;
+}
+
+void DeviceDataManagerTestAPI::SetMouseDevices(
+    const std::vector<InputDevice>& devices) {
+  DeviceDataManager::GetInstance()->mouse_devices_ = devices;
+}
+
+void DeviceDataManagerTestAPI::SetTouchpadDevices(
+    const std::vector<InputDevice>& devices) {
+  DeviceDataManager::GetInstance()->touchpad_devices_ = devices;
+}
+
 }  // namespace test
 }  // namespace ui

ui/events/test/device_data_manager_test_api_stub.cc

@@ -48,5 +48,25 @@ void DeviceDataManagerTestAPI::OnDeviceListsComplete() {
   NOTREACHED();
 }
 
+void DeviceDataManagerTestAPI::SetTouchscreenDevices(
+    const std::vector<TouchscreenDevice>& devices) {
+  NOTREACHED();
+}
+
+void DeviceDataManagerTestAPI::SetKeyboardDevices(
+    const std::vector<InputDevice>& devices) {
+  NOTREACHED();
+}
+
+void DeviceDataManagerTestAPI::SetMouseDevices(
+    const std::vector<InputDevice>& devices) {
+  NOTREACHED();
+}
+
+void DeviceDataManagerTestAPI::SetTouchpadDevices(
+    const std::vector<InputDevice>& devices) {
+  NOTREACHED();
+}
+
 }  // namespace test
 }  // namespace ui