Add test to check deprecated accelerator do not have keyboard overlay.

chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.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 "ash/accelerators/accelerator_table.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
@@ skipping 15 matching lines @@
         "didPaint", base::Bind(&TestWebUIMessageHandler::HandleDidPaint,
                                base::Unretained(this)));
   }
 
  private:
   void HandleDidPaint(const base::ListValue*) {}
 
   DISALLOW_COPY_AND_ASSIGN(TestWebUIMessageHandler);
 };
 
-}  // namespace
+content::WebContents* StartKeyboardOverlayUI(Browser* browser) {
+    ui_test_utils::NavigateToURL(browser,

[xiyuan, 2017/04/13 06:56:21]

2-space indent here and other function body lines.

[wutao, 2017/04/13 16:24:03]

Done.

+                                 GURL(chrome::kChromeUIKeyboardOverlayURL));
+    content::WebContents* web_contents =
+        browser->tab_strip_model()->GetActiveWebContents();
+    web_contents->GetWebUI()->AddMessageHandler(
+        base::MakeUnique<TestWebUIMessageHandler>());
+    return web_contents;
+}
 
-using KeyboardOverlayUIBrowserTest = InProcessBrowserTest;
+bool IsDisplayUIScalingEnabled(content::WebContents* web_contents) {
+    bool is_display_ui_scaling_enabled;
+    CHECK(content::ExecuteScriptAndExtractBool(

[xiyuan, 2017/04/13 06:56:21]

CHECK() should be avoided. Can we use EXPECT_TRUE(...)?

[wutao, 2017/04/13 16:24:03]

Done.

+        web_contents,
+        "domAutomationController.send(isDisplayUIScalingEnabled());",
+        &is_display_ui_scaling_enabled));
+    return is_display_ui_scaling_enabled;
+}
 
-IN_PROC_BROWSER_TEST_F(KeyboardOverlayUIBrowserTest,
-                       ShouldHaveKeyboardOverlay) {
-  ui_test_utils::NavigateToURL(browser(),
-                               GURL(chrome::kChromeUIKeyboardOverlayURL));
-  content::WebContents* web_contents =
-      browser()->tab_strip_model()->GetActiveWebContents();
-  web_contents->GetWebUI()->AddMessageHandler(
-      base::MakeUnique<TestWebUIMessageHandler>());
-
-  bool is_display_ui_scaling_enabled;
-  ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
-      web_contents,
-      "domAutomationController.send(isDisplayUIScalingEnabled());",
-      &is_display_ui_scaling_enabled));
-
-  for (size_t i = 0; i < ash::kAcceleratorDataLength; ++i) {
-    const ash::AcceleratorData& entry = ash::kAcceleratorData[i];
-    // Skip some accelerators in this test:
+bool ShouldSkip(const ash::AcceleratorData& accelerator) {
+    // Skip some accelerators in the tests:
     // 1. If the accelerator has no modifier, i.e. ui::EF_NONE, or for "Caps
     // Lock", such as ui::VKEY_MENU and ui::VKEY_LWIN, the logic to show it on
     // the keyboard overlay is not by the mapping of
     // keyboardOverlayData['shortcut'], so it can not be tested by this test.
     // 2. If it has debug modifiers: ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN |
     // ui::EF_SHIFT_DOWN

[xiyuan, 2017/04/13 06:56:21]

Move the comments out of body and make it a comment for the function.

[wutao, 2017/04/13 16:24:03]

Done.

-    if (entry.keycode == ui::VKEY_MENU ||
-        entry.keycode == ui::VKEY_LWIN ||
-        entry.modifiers == ui::EF_NONE ||
-        entry.modifiers ==
+    if (accelerator.keycode == ui::VKEY_MENU ||
+        accelerator.keycode == ui::VKEY_LWIN ||
+        accelerator.modifiers == ui::EF_NONE ||
+        accelerator.modifiers ==
             (ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN | ui::EF_SHIFT_DOWN)) {
-      continue;
+      return true;
     }
+    return false;

[xiyuan, 2017/04/13 06:56:21]

nit: Get rid of the "if" and put conditions in return;

[wutao, 2017/04/13 16:24:03]

Done.

+}
 
-    std::string shortcut;
-    ASSERT_TRUE(content::ExecuteScriptAndExtractString(
+std::string KeyboardCodeToLabel(const ash::AcceleratorData& accelerator,
+                                content::WebContents* web_contents) {
+    std::string label;
+    CHECK(content::ExecuteScriptAndExtractString(

[xiyuan, 2017/04/13 06:56:21]

EXPECT_TRUE ?

[wutao, 2017/04/13 16:24:03]

Done.

         web_contents,
         "domAutomationController.send("
         "  (function(number) {"
         "       if (!!KEYCODE_TO_LABEL[number]) {"
         "             return KEYCODE_TO_LABEL[number];"
         "       } else {"
         "             return 'NONE';"
         "       }"
-        "  })(" + std::to_string(static_cast<unsigned int>(entry.keycode)) + ")"
+        "  })(" +
+              std::to_string(static_cast<unsigned int>(accelerator.keycode)) +
+        "    )"
         ");",
-        &shortcut));
-    if (shortcut == "NONE") {
-      shortcut = base::ToLowerASCII(
-          static_cast<char>(LocatedToNonLocatedKeyboardCode(entry.keycode)));
+        &label));
+    if (label == "NONE") {
+      label = base::ToLowerASCII(static_cast<char>(
+          LocatedToNonLocatedKeyboardCode(accelerator.keycode)));
     }
+    return label;
+}
 
+std::string GenerateShortcutKey(const ash::AcceleratorData& accelerator,
+                                content::WebContents* web_contents) {
+    std::string shortcut = KeyboardCodeToLabel(accelerator, web_contents);
     // The order of the "if" conditions should not be changed because the
     // modifiers are expected to be alphabetical sorted in the generated
     // shortcut.
-    if (entry.modifiers & ui::EF_ALT_DOWN)
+    if (accelerator.modifiers & ui::EF_ALT_DOWN)
       shortcut.append("<>ALT");
-    if (entry.modifiers & ui::EF_CONTROL_DOWN)
+    if (accelerator.modifiers & ui::EF_CONTROL_DOWN)
       shortcut.append("<>CTRL");
-    if (entry.modifiers & ui::EF_COMMAND_DOWN)
+    if (accelerator.modifiers & ui::EF_COMMAND_DOWN)
       shortcut.append("<>SEARCH");
-    if (entry.modifiers & ui::EF_SHIFT_DOWN)
+    if (accelerator.modifiers & ui::EF_SHIFT_DOWN)
       shortcut.append("<>SHIFT");
+    return shortcut;
+}
 
+bool ContainsShortcut(const std::string& shortcut,
+                      content::WebContents* web_contents) {
+    bool contains;
+    CHECK(content::ExecuteScriptAndExtractBool(

[xiyuan, 2017/04/13 06:56:22]

EXPECT_TRUE ?

[wutao, 2017/04/13 16:24:03]

Done.

+        web_contents,
+        "domAutomationController.send("
+        "  !!keyboardOverlayData['shortcut']['" + shortcut + "']"
+        ");",
+        &contains));
+    return contains;
+}
+
+}  // namespace
+
+using KeyboardOverlayUIBrowserTest = InProcessBrowserTest;
+
+IN_PROC_BROWSER_TEST_F(KeyboardOverlayUIBrowserTest,
+                       AcceleratorsShouldHaveKeyboardOverlay) {
+  content::WebContents* web_contents = StartKeyboardOverlayUI(browser());

[xiyuan, 2017/04/13 06:56:22]

nit: content::WebContents* const web_contents

[wutao, 2017/04/13 16:24:03]

Done.

+  bool is_display_ui_scaling_enabled = IsDisplayUIScalingEnabled(web_contents);

[xiyuan, 2017/04/13 06:56:21]

nit: const bool

[wutao, 2017/04/13 16:24:03]

Done.

+  for (size_t i = 0; i < ash::kAcceleratorDataLength; ++i) {
+    const ash::AcceleratorData& entry = ash::kAcceleratorData[i];
+    if (ShouldSkip(entry))
+      continue;
+
+    std::string shortcut = GenerateShortcutKey(entry, web_contents);

[xiyuan, 2017/04/13 06:56:21]

nit: const std::string

[wutao, 2017/04/13 16:24:03]

Done.

     if (!is_display_ui_scaling_enabled) {
       if (shortcut == "-<>CTRL<>SHIFT" || shortcut == "+<>CTRL<>SHIFT" ||
           shortcut == "0<>CTRL<>SHIFT")
         continue;

[xiyuan, 2017/04/13 06:56:21]

nit: wrap with {} since condition is more than 1 line.

[wutao, 2017/04/13 16:24:03]

Done.

     }
 
-    bool contains;
-    ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
-        web_contents,
-        "domAutomationController.send("
-        "  !!keyboardOverlayData['shortcut']['" + shortcut + "']"
-        ");",
-        &contains));
+    bool contains = ContainsShortcut(shortcut, web_contents);

[xiyuan, 2017/04/13 06:56:22]

This can be removed and inlined with ASSERT_TRUE

i.e.
  ASSERT_TRUE(ContainsShortcut(shortcut, web_contents)) << ...

[wutao, 2017/04/13 16:24:03]

Done.

     ASSERT_TRUE(contains) << "Please add the new accelerators to keyboard "

[xiyuan, 2017/04/13 06:56:21]

ASSERT_TRUE -> EXPECT_TRUE so that the test can list all offending accelerator.

[wutao, 2017/04/13 16:24:03]

Done.

                              "overlay. Add one entry '" +
                                  shortcut +
                                  "' in the 'shortcut' section"
                                  " at the bottom of the file of "
                                  "'/chrome/browser/resources/chromeos/"
                                  "keyboard_overlay_data.js'. Please keep it in "
                                  "alphabetical order.";
   }
 }
+
+IN_PROC_BROWSER_TEST_F(KeyboardOverlayUIBrowserTest,
+                       DeprecatedAcceleratorsShouldNotHaveKeyboardOverlay) {
+  content::WebContents* web_contents = StartKeyboardOverlayUI(browser());
+  for (size_t i = 0; i < ash::kDeprecatedAcceleratorsLength; ++i) {
+    const ash::AcceleratorData& entry = ash::kDeprecatedAccelerators[i];
+    if (ShouldSkip(entry))
+        continue;

[xiyuan, 2017/04/13 06:56:21]

wrong indent.

[wutao, 2017/04/13 16:24:03]

Done.

+
+    std::string shortcut = GenerateShortcutKey(entry, web_contents);
+    bool contains = ContainsShortcut(shortcut, web_contents);

[xiyuan, 2017/04/13 06:56:21]

nit: Get rid of |shortcut| and |contains| and inline the call.

[wutao, 2017/04/13 16:24:02]

I need |shortcut| to print out some info for the offending accelerator.
Removed |contains|.

+    ASSERT_FALSE(contains) << "Please remove the deprecated accelerator '" +

[xiyuan, 2017/04/13 06:56:22]

ASSERT_FALSE -> EXPECT_FALSE to list all bad ones.

[wutao, 2017/04/13 16:24:03]

Done.

+                                 shortcut +
+                                 "' from the 'shortcut' section"
+                                 " at the bottom of the file of "
+                                 "'/chrome/browser/resources/chromeos/"
+                                 "keyboard_overlay_data.js'.";
+  }
+}