Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(4382)

Issues Search
    My Issues | Starred     Open | Closed | All

Unified Diff: chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc

Issue 2815213003: Add test to check deprecated accelerator do not have keyboard overlay. (Closed)
Patch Set: Created 3 years, 8 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « no previous file | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc
diff --git a/chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc b/chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc
index 99eedb3ccfacedf44f3a6eb2530d9fdf0b049d44..93a2fe107f70ded592a1c59979cd4702aacbe17e 100644
--- a/chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc
+++ b/chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc
@@ -33,44 +33,47 @@ class TestWebUIMessageHandler : public content::WebUIMessageHandler {
DISALLOW_COPY_AND_ASSIGN(TestWebUIMessageHandler);
};
-} // namespace
-
-using KeyboardOverlayUIBrowserTest = InProcessBrowserTest;
+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;
+}
-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));
+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;
+}
- 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 commen
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 ret
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) {"
@@ -79,39 +82,68 @@ IN_PROC_BROWSER_TEST_F(KeyboardOverlayUIBrowserTest,
" } 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;
+}
- if (!is_display_ui_scaling_enabled) {
- if (shortcut == "-<>CTRL<>SHIFT" || shortcut == "+<>CTRL<>SHIFT" ||
- shortcut == "0<>CTRL<>SHIFT")
- continue;
- }
-
+bool ContainsShortcut(const std::string& shortcut,
+ content::WebContents* web_contents) {
bool contains;
- ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
+ 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 l
wutao 2017/04/13 16:24:03 Done.
+ }
+
+ bool contains = ContainsShortcut(shortcut, web_contents);
xiyuan 2017/04/13 06:56:22 This can be removed and inlined with ASSERT_TRUE
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 li
wutao 2017/04/13 16:24:03 Done.
"overlay. Add one entry '" +
shortcut +
@@ -122,3 +154,22 @@ IN_PROC_BROWSER_TEST_F(KeyboardOverlayUIBrowserTest,
"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 inli
wutao 2017/04/13 16:24:02 I need |shortcut| to print out some info for the o
+ 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'.";
+ }
+}
« no previous file with comments | « no previous file | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698