Revert "Hide option to remap internal search when no internal KB is connected"

This reverts commit 4c097c39.

Reason for revert: I found a bug in internal keyboard detection. I will reland with fix to be able to merge to M-71.

Original change's description:
> Hide option to remap internal search when no internal KB is connected
> 
> To avoid confusion when there is no internal keyboard connected:
> - We should not show the option to remap the internal "Search"
> key since that key doesn't exist.
> - We should also avoid labeling the options to remap the external
> meta keys as "External Meta" or "External Command", and just say
> "Meta" or "Command" as "External" here is redundant.
> 
> BUG=890522, 890950
> TEST=Expanded existing tests.
> 
> Change-Id: If979b80217298a5f6bb18be478aa2c457859a351
> Reviewed-on: https://chromium-review.googlesource.com/c/1253022
> Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
> Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#599409}

TBR=stevenjb@chromium.org,afakhry@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 890522, 890950
Change-Id: I9cd4273a61c51751a20d8bbffbf7fffbb252246c
Reviewed-on: https://chromium-review.googlesource.com/c/1292816Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601391}

chrome/app/settings_strings.grdp

@@ -3916,15 +3916,9 @@
     <message name="IDS_SETTINGS_KEYBOARD_KEY_EXTERNAL_COMMAND" desc="In Device Settings, the dropdown list item for the external Command key on Apple keyboards.">
       External Command
     </message>
-    <message name="IDS_SETTINGS_KEYBOARD_KEY_COMMAND" desc="In Device Settings, the dropdown list item for the external Command key on Apple keyboards when there's no internal keyboard on the device.">
-      Command
-    </message>
     <message name="IDS_SETTINGS_KEYBOARD_KEY_EXTERNAL_META" desc="In Device Settings, the dropdown list item for the external Meta key (Search key on ChromeOS keyboards, and Windows key on Windows keyboards).">
       External Meta
     </message>
-    <message name="IDS_SETTINGS_KEYBOARD_KEY_META" desc="In Device Settings, the dropdown list item for the external Meta key (Search key on ChromeOS keyboards, and Windows key on Windows keyboards) when there's no internal keyboard on the device.">
-      Meta
-    </message>
     <message name="IDS_SETTINGS_KEYBOARD_SEND_FUNCTION_KEYS" desc="In Device Settings, the checkbox label for interpreting the top-row keys as function keys instead.">
       Treat top-row keys as function keys
     </message>

chrome/browser/resources/settings/device_page/keyboard.html

@@ -13,15 +13,13 @@
 <dom-module id="settings-keyboard">
   <template>
     <style include="settings-shared"></style>
-    <template is="dom-if" if="[[hasInternalKeyboard_]]">
-      <div class="settings-box first" id="internalSearchKey">
-        <div class="start">$i18n{keyboardKeySearch}</div>
-        <settings-dropdown-menu label="$i18n{keyboardKeySearch}"
-            pref="{{prefs.settings.language.xkb_remap_search_key_to}}"
-            menu-options="[[keyMapTargets_]]">
-        </settings-dropdown-menu>
-      </div>
-    </template>
+    <div class="settings-box first">
+      <div class="start">$i18n{keyboardKeySearch}</div>
+      <settings-dropdown-menu label="$i18n{keyboardKeySearch}"
+          pref="{{prefs.settings.language.xkb_remap_search_key_to}}"
+          menu-options="[[keyMapTargets_]]">
+      </settings-dropdown-menu>
+    </div>
     <div class="settings-box">
       <div class="start">$i18n{keyboardKeyCtrl}</div>
       <settings-dropdown-menu label="$i18n{keyboardKeyCtrl}"
@@ -70,11 +68,8 @@
     </template>
     <template is="dom-if" if="[[showExternalMetaKey_]]">
       <div class="settings-box" id="externalMetaKey">
-        <div class="start">
-          [[getExternalMetaKeyLabel_(hasInternalKeyboard_)]]
-        </div>
-        <settings-dropdown-menu
-            label="[[getExternalMetaKeyLabel_(hasInternalKeyboard_)]]"
+        <div class="start">$i18n{keyboardKeyExternalMeta}</div>
+        <settings-dropdown-menu label="$i18n{keyboardKeyExternalMeta}"
             pref="{{prefs.settings.language.remap_external_meta_key_to}}"
             menu-options="[[keyMapTargets_]]">
         </settings-dropdown-menu>
@@ -82,11 +77,9 @@
     </template>
     <template is="dom-if" if="[[showAppleCommandKey_]]">
       <div class="settings-box" id="externalCommandKey">
-        <div class="start">
-          [[getExternalCommandKeyLabel_(hasInternalKeyboard_)]]
-        </div>
+        <div class="start">$i18n{keyboardKeyExternalCommand}</div>
         <settings-dropdown-menu
-            label="[[getExternalCommandKeyLabel_(hasInternalKeyboard_)]]"
+            label="$i18n{keyboardKeyExternalCommand}"
             pref="{{prefs.settings.language.remap_external_command_key_to}}"
             menu-options="[[keyMapTargets_]]">
         </settings-dropdown-menu>

chrome/browser/resources/settings/device_page/keyboard.js

@@ -39,9 +39,6 @@ Polymer({
     /** @private Whether to show diamond key options. */
     showDiamondKey_: Boolean,
 
-    /** @private Whether this device has an internal keyboard. */
-    hasInternalKeyboard_: Boolean,
-
     /**
      * Whether to show a remapping option for external keyboard's Meta key
      * (Search/Windows keys). This is true only when there's an external
@@ -136,7 +133,6 @@ Polymer({
    * @private
    */
   onShowKeysChange_: function(keyboardParams) {
-    this.hasInternalKeyboard_ = keyboardParams['hasInternalKeyboard'];
     this.showCapsLock_ = keyboardParams['showCapsLock'];
     this.showDiamondKey_ = keyboardParams['showDiamondKey'];
     this.showExternalMetaKey_ = keyboardParams['showExternalMetaKey'];
@@ -151,15 +147,4 @@ Polymer({
   onShowLanguageInputTap_: function() {
     settings.navigateTo(settings.routes.LANGUAGES);
   },
-
-  getExternalMetaKeyLabel_: function(hasInternalKeyboard) {
-    return loadTimeData.getString(
-        hasInternalKeyboard ? 'keyboardKeyExternalMeta' : 'keyboardKeyMeta');
-  },
-
-  getExternalCommandKeyLabel_: function(hasInternalKeyboard) {
-    return loadTimeData.getString(
-        hasInternalKeyboard ? 'keyboardKeyExternalCommand' :
-                              'keyboardKeyCommand');
-  },
 });

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

@@ -20,7 +20,6 @@
 namespace {
 
 struct KeyboardsStateResult {
-  bool has_internal_keyboard = false;
   bool has_external_non_apple_keyboard = false;
   bool has_apple_keyboard = false;
 };
@@ -29,8 +28,6 @@ KeyboardsStateResult GetKeyboardsState() {
   KeyboardsStateResult result;
   for (const ui::InputDevice& keyboard :
        ui::InputDeviceManager::GetInstance()->GetKeyboardDevices()) {
-    result.has_internal_keyboard = (keyboard.type == ui::INPUT_DEVICE_INTERNAL);
-
     const ui::EventRewriterChromeOS::DeviceType type =
         ui::EventRewriterChromeOS::GetDeviceType(keyboard);
     if (type == ui::EventRewriterChromeOS::kDeviceAppleKeyboard) {
@@ -149,8 +146,6 @@ void KeyboardHandler::UpdateShowKeys() {
       base::Value(keyboards_state.has_external_non_apple_keyboard));
   keyboard_params.SetKey("showAppleCommandKey",
                          base::Value(keyboards_state.has_apple_keyboard));
-  keyboard_params.SetKey("hasInternalKeyboard",
-                         base::Value(keyboards_state.has_internal_keyboard));
 
   FireWebUIListener(kShowKeysChangedName, keyboard_params);
 }

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

@@ -47,8 +47,7 @@ class KeyboardHandlerTest : public testing::Test {
   bool GetLastShowKeysChangedMessage(bool* has_caps_lock_out,
                                      bool* has_diamond_key_out,
                                      bool* has_external_meta_key_out,
-                                     bool* has_apple_command_key_out,
-                                     bool* has_internal_search_out)
+                                     bool* has_apple_command_key_out)
       WARN_UNUSED_RESULT {
     for (auto it = web_ui_.call_data().rbegin();
          it != web_ui_.call_data().rend(); ++it) {
@@ -71,7 +70,6 @@ class KeyboardHandlerTest : public testing::Test {
           {"showDiamondKey", has_diamond_key_out},
           {"showExternalMetaKey", has_external_meta_key_out},
           {"showAppleCommandKey", has_apple_command_key_out},
-          {"hasInternalKeyboard", has_internal_search_out},
       };
 
       for (const auto& pair : path_to_out_param) {
@@ -94,7 +92,7 @@ class KeyboardHandlerTest : public testing::Test {
     bool has_caps_lock = false;
     bool ignored = false;
     if (!GetLastShowKeysChangedMessage(&has_caps_lock, &ignored, &ignored,
-                                       &ignored, &ignored)) {
+                                       &ignored)) {
       ADD_FAILURE() << "Didn't get " << KeyboardHandler::kShowKeysChangedName;
       return false;
     }
@@ -108,7 +106,7 @@ class KeyboardHandlerTest : public testing::Test {
     bool has_diamond_key = false;
     bool ignored = false;
     if (!GetLastShowKeysChangedMessage(&ignored, &has_diamond_key, &ignored,
-                                       &ignored, &ignored)) {
+                                       &ignored)) {
       ADD_FAILURE() << "Didn't get " << KeyboardHandler::kShowKeysChangedName;
       return false;
     }
@@ -122,7 +120,7 @@ class KeyboardHandlerTest : public testing::Test {
     bool has_external_meta = false;
     bool ignored = false;
     if (!GetLastShowKeysChangedMessage(&ignored, &ignored, &has_external_meta,
-                                       &ignored, &ignored)) {
+                                       &ignored)) {
       ADD_FAILURE() << "Didn't get " << KeyboardHandler::kShowKeysChangedName;
       return false;
     }
@@ -136,27 +134,13 @@ class KeyboardHandlerTest : public testing::Test {
     bool has_apple_command_key = false;
     bool ignored = false;
     if (!GetLastShowKeysChangedMessage(&ignored, &ignored, &ignored,
-                                       &has_apple_command_key, &ignored)) {
+                                       &has_apple_command_key)) {
       ADD_FAILURE() << "Didn't get " << KeyboardHandler::kShowKeysChangedName;
       return false;
     }
     return has_apple_command_key;
   }
 
-  // Returns true if the last keys-changed message reported that the device has
-  // an internal keyboard and hence an internal Search key remap option.
-  // A failure is added if a message wasn't found.
-  bool HasInternalSearchKey() {
-    bool has_internal_search_key = false;
-    bool ignored = false;
-    if (!GetLastShowKeysChangedMessage(&ignored, &ignored, &ignored, &ignored,
-                                       &has_internal_search_key)) {
-      ADD_FAILURE() << "Didn't get " << KeyboardHandler::kShowKeysChangedName;
-      return false;
-    }
-    return has_internal_search_key;
-  }
-
   ws::InputDeviceClientTestApi input_device_client_test_api_;
   content::TestWebUI web_ui_;
   TestKeyboardHandler handler_;
@@ -170,7 +154,6 @@ TEST_F(KeyboardHandlerTest, DefaultKeys) {
   base::CommandLine::ForCurrentProcess()->AppendSwitch(
       chromeos::switches::kHasChromeOSKeyboard);
   handler_test_api_.Initialize();
-  EXPECT_FALSE(HasInternalSearchKey());
   EXPECT_FALSE(HasCapsLock());
   EXPECT_FALSE(HasDiamondKey());
   EXPECT_FALSE(HasExternalMetaKey());
@@ -181,7 +164,6 @@ TEST_F(KeyboardHandlerTest, NonChromeOSKeyboard) {
   // If kHasChromeOSKeyboard isn't passed, we should assume there's a Caps Lock
   // key.
   handler_test_api_.Initialize();
-  EXPECT_FALSE(HasInternalSearchKey());
   EXPECT_TRUE(HasCapsLock());
   EXPECT_FALSE(HasDiamondKey());
   EXPECT_FALSE(HasExternalMetaKey());
@@ -195,7 +177,6 @@ TEST_F(KeyboardHandlerTest, ExternalKeyboard) {
   input_device_client_test_api_.SetKeyboardDevices(std::vector<ui::InputDevice>{
       {1, ui::INPUT_DEVICE_INTERNAL, "internal keyboard"}});
   handler_test_api_.Initialize();
-  EXPECT_TRUE(HasInternalSearchKey());
   EXPECT_FALSE(HasCapsLock());
   EXPECT_FALSE(HasDiamondKey());
   EXPECT_FALSE(HasExternalMetaKey());
@@ -205,7 +186,6 @@ TEST_F(KeyboardHandlerTest, ExternalKeyboard) {
   // Caps Lock and Meta keys now.
   input_device_client_test_api_.SetKeyboardDevices(std::vector<ui::InputDevice>{
       {2, ui::INPUT_DEVICE_USB, "external keyboard"}});
-  EXPECT_FALSE(HasInternalSearchKey());
   EXPECT_TRUE(HasCapsLock());
   EXPECT_FALSE(HasDiamondKey());
   EXPECT_TRUE(HasExternalMetaKey());
@@ -215,7 +195,6 @@ TEST_F(KeyboardHandlerTest, ExternalKeyboard) {
   // the command key.
   input_device_client_test_api_.SetKeyboardDevices(std::vector<ui::InputDevice>{
       {3, ui::INPUT_DEVICE_USB, "Apple Inc. Apple Keyboard"}});
-  EXPECT_FALSE(HasInternalSearchKey());
   EXPECT_TRUE(HasCapsLock());
   EXPECT_FALSE(HasDiamondKey());
   EXPECT_FALSE(HasExternalMetaKey());
@@ -226,7 +205,6 @@ TEST_F(KeyboardHandlerTest, ExternalKeyboard) {
   input_device_client_test_api_.SetKeyboardDevices(std::vector<ui::InputDevice>{
       {2, ui::INPUT_DEVICE_USB, "external keyboard"},
       {3, ui::INPUT_DEVICE_USB, "Apple Inc. Apple Keyboard"}});
-  EXPECT_FALSE(HasInternalSearchKey());
   EXPECT_TRUE(HasCapsLock());
   EXPECT_FALSE(HasDiamondKey());
   EXPECT_TRUE(HasExternalMetaKey());
@@ -238,7 +216,6 @@ TEST_F(KeyboardHandlerTest, ExternalKeyboard) {
   // https://crbug.com/834594.
   input_device_client_test_api_.SetKeyboardDevices(std::vector<ui::InputDevice>{
       {4, ui::INPUT_DEVICE_USB, "Topre Corporation Realforce 87"}});
-  EXPECT_FALSE(HasInternalSearchKey());
   EXPECT_TRUE(HasCapsLock());
   EXPECT_FALSE(HasDiamondKey());
   EXPECT_TRUE(HasExternalMetaKey());
@@ -246,7 +223,6 @@ TEST_F(KeyboardHandlerTest, ExternalKeyboard) {
 
   // Disconnect the external keyboard and check that the key goes away.
   input_device_client_test_api_.SetKeyboardDevices({});
-  EXPECT_FALSE(HasInternalSearchKey());
   EXPECT_FALSE(HasCapsLock());
   EXPECT_FALSE(HasDiamondKey());
   EXPECT_FALSE(HasExternalMetaKey());

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

@@ -634,7 +634,6 @@ void AddDeviceStrings(content::WebUIDataSource* html_source) {
       {"keyboardKeyCtrl", IDS_SETTINGS_KEYBOARD_KEY_LEFT_CTRL},
       {"keyboardKeyAlt", IDS_SETTINGS_KEYBOARD_KEY_LEFT_ALT},
       {"keyboardKeyCapsLock", IDS_SETTINGS_KEYBOARD_KEY_CAPS_LOCK},
-      {"keyboardKeyCommand", IDS_SETTINGS_KEYBOARD_KEY_COMMAND},
       {"keyboardKeyDiamond", IDS_SETTINGS_KEYBOARD_KEY_DIAMOND},
       {"keyboardKeyEscape", IDS_SETTINGS_KEYBOARD_KEY_ESCAPE},
       {"keyboardKeyBackspace", IDS_SETTINGS_KEYBOARD_KEY_BACKSPACE},
@@ -642,7 +641,6 @@ void AddDeviceStrings(content::WebUIDataSource* html_source) {
       {"keyboardKeyExternalCommand",
        IDS_SETTINGS_KEYBOARD_KEY_EXTERNAL_COMMAND},
       {"keyboardKeyExternalMeta", IDS_SETTINGS_KEYBOARD_KEY_EXTERNAL_META},
-      {"keyboardKeyMeta", IDS_SETTINGS_KEYBOARD_KEY_META},
       {"keyboardSendFunctionKeys", IDS_SETTINGS_KEYBOARD_SEND_FUNCTION_KEYS},
       {"keyboardSendFunctionKeysDescription",
        ui::DeviceUsesKeyboardLayout2()

chrome/test/data/webui/settings/device_page_tests.js

@@ -610,17 +610,15 @@ cr.define('device_page_tests', function() {
             expectFalse(!!keyboardPage.$$('#capsLockKey'));
             expectFalse(!!keyboardPage.$$('#diamondKey'));
 
-            // Pretend the diamond key is available, and no internal keyboard.
+            // Pretend the diamond key is available.
             let keyboardParams = {
               'showCapsLock': false,
               'showDiamondKey': true,
               'showExternalMetaKey': false,
               'showAppleCommandKey': false,
-              'hasInternalKeyboard': false,
             };
             cr.webUIListenerCallback('show-keys-changed', keyboardParams);
             Polymer.dom.flush();
-            expectFalse(!!keyboardPage.$$('#internalSearchKey'));
             expectFalse(!!keyboardPage.$$('#capsLockKey'));
             expectTrue(!!keyboardPage.$$('#diamondKey'));
             expectFalse(!!keyboardPage.$$('#externalMetaKey'));
@@ -630,7 +628,6 @@ cr.define('device_page_tests', function() {
             keyboardParams['showCapsLock'] = true;
             cr.webUIListenerCallback('show-keys-changed', keyboardParams);
             Polymer.dom.flush();
-            expectFalse(!!keyboardPage.$$('#internalSearchKey'));
             expectTrue(!!keyboardPage.$$('#capsLockKey'));
             expectTrue(!!keyboardPage.$$('#diamondKey'));
             expectFalse(!!keyboardPage.$$('#externalMetaKey'));
@@ -640,7 +637,6 @@ cr.define('device_page_tests', function() {
             keyboardParams['showExternalMetaKey'] = true;
             cr.webUIListenerCallback('show-keys-changed', keyboardParams);
             Polymer.dom.flush();
-            expectFalse(!!keyboardPage.$$('#internalSearchKey'));
             expectTrue(!!keyboardPage.$$('#capsLockKey'));
             expectTrue(!!keyboardPage.$$('#diamondKey'));
             expectTrue(!!keyboardPage.$$('#externalMetaKey'));
@@ -650,17 +646,6 @@ cr.define('device_page_tests', function() {
             keyboardParams['showAppleCommandKey'] = true;
             cr.webUIListenerCallback('show-keys-changed', keyboardParams);
             Polymer.dom.flush();
-            expectFalse(!!keyboardPage.$$('#internalSearchKey'));
-            expectTrue(!!keyboardPage.$$('#capsLockKey'));
-            expectTrue(!!keyboardPage.$$('#diamondKey'));
-            expectTrue(!!keyboardPage.$$('#externalMetaKey'));
-            expectTrue(!!keyboardPage.$$('#externalCommandKey'));
-
-            // Add an internal keyboard.
-            keyboardParams['hasInternalKeyboard'] = true;
-            cr.webUIListenerCallback('show-keys-changed', keyboardParams);
-            Polymer.dom.flush();
-            expectTrue(!!keyboardPage.$$('#internalSearchKey'));
             expectTrue(!!keyboardPage.$$('#capsLockKey'));
             expectTrue(!!keyboardPage.$$('#diamondKey'));
             expectTrue(!!keyboardPage.$$('#externalMetaKey'));