Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2815213003:
Add test to check deprecated accelerator do not have keyboard overlay. (Closed)
DescriptionAdd test to check deprecated accelerator do not have keyboard overlay.
When deprecate and replace an Accelerator, one step is to remove
it from the keyboard overlay. Writing a test to ensure it.
BUG=706462
R=xiyuan@chromium.org
TEST=KeyboardOverlayUIBrowserTest.*
Review-Url: https://codereview.chromium.org/2815213003
Cr-Commit-Position: refs/heads/master@{#464456}
Committed: https://chromium.googlesource.com/chromium/src/+/a0bd2c53f52b94f64ac1521e3bdea8da54c870b4
Patch Set 1 #
Total comments: 30
Patch Set 2 : Fix nits in patch 1. #Messages
Total messages: 14 (8 generated)
Hi Xiyuan, I add one more test and refactor the code. Mostly are moving code around. In the new helper functions, no new changes except changing ASSERT_TRUE to CHECK. Hope it is not too hard to review because the moving. Thanks to have the review. Tao
Could you re-format the CL description to make the subject line < 72 chars ? https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc (right): https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:37: ui_test_utils::NavigateToURL(browser, 2-space indent here and other function body lines. https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:48: CHECK(content::ExecuteScriptAndExtractBool( CHECK() should be avoided. Can we use EXPECT_TRUE(...)? https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:62: // ui::EF_SHIFT_DOWN Move the comments out of body and make it a comment for the function. https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:70: return false; nit: Get rid of the "if" and put conditions in return; https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:76: CHECK(content::ExecuteScriptAndExtractString( EXPECT_TRUE ? https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:117: CHECK(content::ExecuteScriptAndExtractBool( EXPECT_TRUE ? https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:132: content::WebContents* web_contents = StartKeyboardOverlayUI(browser()); nit: content::WebContents* const web_contents https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:133: bool is_display_ui_scaling_enabled = IsDisplayUIScalingEnabled(web_contents); nit: const bool https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:139: std::string shortcut = GenerateShortcutKey(entry, web_contents); nit: const std::string https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:143: continue; nit: wrap with {} since condition is more than 1 line. https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:146: bool contains = ContainsShortcut(shortcut, web_contents); This can be removed and inlined with ASSERT_TRUE i.e. ASSERT_TRUE(ContainsShortcut(shortcut, web_contents)) << ... https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:147: ASSERT_TRUE(contains) << "Please add the new accelerators to keyboard " ASSERT_TRUE -> EXPECT_TRUE so that the test can list all offending accelerator. https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:164: continue; wrong indent. https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:167: bool contains = ContainsShortcut(shortcut, web_contents); nit: Get rid of |shortcut| and |contains| and inline the call. https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:168: ASSERT_FALSE(contains) << "Please remove the deprecated accelerator '" + ASSERT_FALSE -> EXPECT_FALSE to list all bad ones.
Description was changed from ========== Refactor KeyboardOverlayUIBrowserTest and add a test to check deprecated accelerators should not have keyboard overlay. When deprecate and replace an Accelerator, one step is to remove it from the keyboard overlay. Writing a test to ensure it. BUG=706462 R=xiyuan@chromium.org TEST=KeyboardOverlayUIBrowserTest.* ========== to ========== Add test to check deprecated accelerator do not have keyboard overlay. When deprecate and replace an Accelerator, one step is to remove it from the keyboard overlay. Writing a test to ensure it. BUG=706462 R=xiyuan@chromium.org TEST=KeyboardOverlayUIBrowserTest.* ==========
Hi Xiyuan, I fixed the nits in patch 1. Please have another look. Thank you, Tao https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc (right): https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:37: ui_test_utils::NavigateToURL(browser, On 2017/04/13 06:56:21, xiyuan wrote: > 2-space indent here and other function body lines. Done. https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:48: CHECK(content::ExecuteScriptAndExtractBool( On 2017/04/13 06:56:21, xiyuan wrote: > CHECK() should be avoided. Can we use EXPECT_TRUE(...)? Done. https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:62: // ui::EF_SHIFT_DOWN On 2017/04/13 06:56:21, xiyuan wrote: > Move the comments out of body and make it a comment for the function. Done. https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:70: return false; On 2017/04/13 06:56:21, xiyuan wrote: > nit: Get rid of the "if" and put conditions in return; Done. https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:76: CHECK(content::ExecuteScriptAndExtractString( On 2017/04/13 06:56:21, xiyuan wrote: > EXPECT_TRUE ? Done. https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:117: CHECK(content::ExecuteScriptAndExtractBool( On 2017/04/13 06:56:22, xiyuan wrote: > EXPECT_TRUE ? Done. https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:132: content::WebContents* web_contents = StartKeyboardOverlayUI(browser()); On 2017/04/13 06:56:22, xiyuan wrote: > nit: content::WebContents* const web_contents Done. https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:133: bool is_display_ui_scaling_enabled = IsDisplayUIScalingEnabled(web_contents); On 2017/04/13 06:56:21, xiyuan wrote: > nit: const bool Done. https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:139: std::string shortcut = GenerateShortcutKey(entry, web_contents); On 2017/04/13 06:56:21, xiyuan wrote: > nit: const std::string Done. https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:143: continue; On 2017/04/13 06:56:21, xiyuan wrote: > nit: wrap with {} since condition is more than 1 line. Done. https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:146: bool contains = ContainsShortcut(shortcut, web_contents); On 2017/04/13 06:56:22, xiyuan wrote: > This can be removed and inlined with ASSERT_TRUE > > i.e. > ASSERT_TRUE(ContainsShortcut(shortcut, web_contents)) << ... Done. https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:147: ASSERT_TRUE(contains) << "Please add the new accelerators to keyboard " On 2017/04/13 06:56:21, xiyuan wrote: > ASSERT_TRUE -> EXPECT_TRUE so that the test can list all offending accelerator. Done. https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:164: continue; On 2017/04/13 06:56:21, xiyuan wrote: > wrong indent. Done. https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:167: bool contains = ContainsShortcut(shortcut, web_contents); On 2017/04/13 06:56:21, xiyuan wrote: > nit: Get rid of |shortcut| and |contains| and inline the call. I need |shortcut| to print out some info for the offending accelerator. Removed |contains|. https://codereview.chromium.org/2815213003/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:168: ASSERT_FALSE(contains) << "Please remove the deprecated accelerator '" + On 2017/04/13 06:56:22, xiyuan wrote: > ASSERT_FALSE -> EXPECT_FALSE to list all bad ones. Done.
lgtm
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by wutao@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492104347283680,
"parent_rev": "40e16386f32e441ddc317243955caa74c97b2870", "commit_rev":
"a0bd2c53f52b94f64ac1521e3bdea8da54c870b4"}
Message was sent while issue was closed.
Description was changed from ========== Add test to check deprecated accelerator do not have keyboard overlay. When deprecate and replace an Accelerator, one step is to remove it from the keyboard overlay. Writing a test to ensure it. BUG=706462 R=xiyuan@chromium.org TEST=KeyboardOverlayUIBrowserTest.* ========== to ========== Add test to check deprecated accelerator do not have keyboard overlay. When deprecate and replace an Accelerator, one step is to remove it from the keyboard overlay. Writing a test to ensure it. BUG=706462 R=xiyuan@chromium.org TEST=KeyboardOverlayUIBrowserTest.* Review-Url: https://codereview.chromium.org/2815213003 Cr-Commit-Position: refs/heads/master@{#464456} Committed: https://chromium.googlesource.com/chromium/src/+/a0bd2c53f52b94f64ac1521e3bde... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/a0bd2c53f52b94f64ac1521e3bde... |
