Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2814003002:
Add test to check all current accelerators having keyboard overlay. (Closed)
DescriptionAdd test to check all current accelerators having keyboard overlay.
Add test to check all current accelerators having keyboard overlay.
BUG=706462
TEST=KeyboradOverlayUIBrowserTest.ShouldHaveKeyboardOverlay
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2814003002
Cr-Commit-Position: refs/heads/master@{#464273}
Committed: https://chromium.googlesource.com/chromium/src/+/f3d080598d0e371fb134baaf3f4439b6dd2d8a98
Patch Set 1 #
Total comments: 32
Patch Set 2 : Fix for the comments in patch 1. #
Total comments: 6
Patch Set 3 : Fix nits in patch 2. #
Total comments: 6
Patch Set 4 : Fix for the comments in patch 3. #
Total comments: 14
Patch Set 5 : Fix nits in patch 4. #Patch Set 6 : Fix nits in patch 5. #Patch Set 7 : Forget to save a change. #
Messages
Total messages: 26 (14 generated)
Description was changed from ========== Add test to check all current accelerators having keyboard overlay. Add test to check all current accelerators having keyboard overlay. BUG=706462 TEST=KeyboradOverlayUIBrowserTest.ShouldHaveKeyboardOverlay ========== to ========== Add test to check all current accelerators having keyboard overlay. Add test to check all current accelerators having keyboard overlay. BUG=706462 TEST=KeyboradOverlayUIBrowserTest.ShouldHaveKeyboardOverlay CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
wutao@chromium.org changed reviewers: + afakhry@chromium.org, thestig@chromium.org, xiyuan@chromium.org
Hi thestig@: Please review changes in chrome/app/chromeos_strings.grdp chrome/test/BUILD.gn Hi xiyuan@: Please review changes in chrome/browser/resources/chromeos/keyboard_overlay.js chrome/browser/resources/chromeos/keyboard_overlay_data.js chrome/browser/ui/webui/chromeos/keyboard_overlay_ui.cc chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc The git cl format changed the ID table in the keyboard_overlay_ui.cc. Thanks, Tao
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.
https://codereview.chromium.org/2814003002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/keyboard_overlay.js (right): https://codereview.chromium.org/2814003002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/keyboard_overlay.js:48: // This mapping is maintained for Keyboard_overlay_ui_browsertest. nit: either use lower case file name or use camel case test name. i.e. use Keyboard_overlay_ui_browsertest or KeyboradOverlayUIBrowserTest https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/keyboard_overlay_ui.cc (right): https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui.cc:279: {"keyboardOverlayZoomScreenOut", IDS_KEYBOARD_OVERLAY_ZOOM_SCREEN_OUT}, This is impossible to review. I assume you did the right thing. If we need to things like this, upload a patch for "git cl format" first and upload another patch with real changes. https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc (right): https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:5: #if defined(USE_ASH) chromeos assumes USE_ASH now so no need to check this. And if we need to use a #if defined, it should be in a separate section apart from other includes. See https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:34: class KeyboradOverlayUIBrowserTest : public InProcessBrowserTest { It seems like you can just do using KeyboradOverlayUIBrowserTest = InProcessBrowserTest; since you do not need to override anything for your test. https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:50: web_contents->GetWebUI()->AddMessageHandler(base::WrapUnique(test_handler)); What is the purpose of |test_handler|? It seems not used. https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:52: #if defined(USE_ASH) get rid of this https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:53: for (size_t j = 0; j < ash::kAcceleratorDataLength; ++j) { for (const auto& entry : ash::kAcceleratorData) { or if we want to use a loop with a var, start with "i" instead of "j" https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:58: ui::EF_SHIFT_DOWN)) What this "if" do ? Somehow it does not make a lot of sense to me. If it is intentional, can you add a comment to document what it does ? https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:59: continue; nit: wrap with {} since conditions occupy more than one line. https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:62: unsigned int code = (unsigned int)keyboard_code; use static_cast<>. Casting with () is not allowed. https://google.github.io/styleguide/cppguide.html#Casting https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:75: ")" nit: I would fold line 74 and 75 onto line 73 to make it easier to read. Also it seems |keyboard_code|, |code| are just introduced for casting. Can we get rid of them and do it inline here? i.e. std::to_string(static_cast<unsigned int>(entry.keycode)) https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:76: " );", nit: align ");" with the start of "domAutomationController.send(" i.e. "domAutomationController.send(" .... ");", https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:85: if (entry.modifiers & ui::EF_ALT_DOWN) nit: document the order of the "if" should not be changed because the modifiers are expected to be alpha sorted in the generated shortcut. https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:97: "domAutomationController.send(isDisplayUIScalingEnabled());", This would not change per accelerator. Could you move it out of the loop to call it only once? https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:109: " (function() {" nit: the "function" seems not necessary. Can we just send result directly i.e. "domAutomationController.send(" " !!keyboardOverlayData['shortcut']['" + shortcut + "']" ");" https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:114: " );", nit: align with "domAutomationController.send("
Hi Xiyuan, Please have a look of the fix. The git cl format still wants to mess up with the script in the test. :) Thank you, Tao https://codereview.chromium.org/2814003002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/keyboard_overlay.js (right): https://codereview.chromium.org/2814003002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/keyboard_overlay.js:48: // This mapping is maintained for Keyboard_overlay_ui_browsertest. On 2017/04/12 16:15:46, xiyuan wrote: > nit: either use lower case file name or use camel case test name. > > i.e. > use Keyboard_overlay_ui_browsertest or KeyboradOverlayUIBrowserTest Changed to lowercase keyboard_overlay_ui_browsertest. https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/keyboard_overlay_ui.cc (right): https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui.cc:279: {"keyboardOverlayZoomScreenOut", IDS_KEYBOARD_OVERLAY_ZOOM_SCREEN_OUT}, On 2017/04/12 16:15:46, xiyuan wrote: > This is impossible to review. I assume you did the right thing. If we need to > things like this, upload a patch for "git cl format" first and upload another > patch with real changes. Thanks, I added the following entries with IDs: IDS_KEYBOARD_OVERLAY_ROTATE_WINDOW IDS_KEYBOARD_OVERLAY_SHOW_STYLUS_TOOLS IDS_KEYBOARD_OVERLAY_TOUCH_HUD_MODE_CHANGE IDS_KEYBOARD_OVERLAY_UNPIN https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc (right): https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:5: #if defined(USE_ASH) On 2017/04/12 16:15:46, xiyuan wrote: > chromeos assumes USE_ASH now so no need to check this. And if we need to use a > #if defined, it should be in a separate section apart from other includes. > > See > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Removed #if section. https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:34: class KeyboradOverlayUIBrowserTest : public InProcessBrowserTest { On 2017/04/12 16:15:46, xiyuan wrote: > It seems like you can just do > > using KeyboradOverlayUIBrowserTest = InProcessBrowserTest; > > since you do not need to override anything for your test. Done. https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:50: web_contents->GetWebUI()->AddMessageHandler(base::WrapUnique(test_handler)); On 2017/04/12 16:15:47, xiyuan wrote: > What is the purpose of |test_handler|? It seems not used. Need TestWebUIMessageHandler to handle message "didPaint". I do not need a variable for this, so I will remove this variable. https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:52: #if defined(USE_ASH) On 2017/04/12 16:15:46, xiyuan wrote: > get rid of this Done. https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:53: for (size_t j = 0; j < ash::kAcceleratorDataLength; ++j) { On 2017/04/12 16:15:46, xiyuan wrote: > for (const auto& entry : ash::kAcceleratorData) { > > or if we want to use a loop with a var, start with "i" instead of "j" Cannot use incomplete type as a range, so I will change j to i. https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:58: ui::EF_SHIFT_DOWN)) On 2017/04/12 16:15:46, xiyuan wrote: > What this "if" do ? Somehow it does not make a lot of sense to me. > > If it is intentional, can you add a comment to document what it does ? Some accelerator is handled in the code, not by the keyboardOverlayData['shortcut'] mapping, so cannot be tested in this test. We can consider write another one. I will add a comment, but please suggest other better way. Please see: https://bugs.chromium.org/p/chromium/issues/detail?id=706462#c10 https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:59: continue; On 2017/04/12 16:15:46, xiyuan wrote: > nit: wrap with {} since conditions occupy more than one line. Done. https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:62: unsigned int code = (unsigned int)keyboard_code; On 2017/04/12 16:15:47, xiyuan wrote: > use static_cast<>. Casting with () is not allowed. > > https://google.github.io/styleguide/cppguide.html#Casting Removed. https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:75: ")" On 2017/04/12 16:15:47, xiyuan wrote: > nit: I would fold line 74 and 75 onto line 73 to make it easier to read. > > Also it seems |keyboard_code|, |code| are just introduced for casting. Can we > get rid of them and do it inline here? > > i.e. > > std::to_string(static_cast<unsigned int>(entry.keycode)) git cl format changed this and warn me if I change to a better format :) Removed |keyboard_code|, |code|. https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:76: " );", On 2017/04/12 16:15:46, xiyuan wrote: > nit: align ");" with the start of "domAutomationController.send(" > > i.e. > > "domAutomationController.send(" > .... > ");", Done. https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:85: if (entry.modifiers & ui::EF_ALT_DOWN) On 2017/04/12 16:15:46, xiyuan wrote: > nit: document the order of the "if" should not be changed because the modifiers > are expected to be alpha sorted in the generated shortcut. Done. https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:97: "domAutomationController.send(isDisplayUIScalingEnabled());", On 2017/04/12 16:15:46, xiyuan wrote: > This would not change per accelerator. Could you move it out of the loop to call > it only once? Good catch! Done. https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:109: " (function() {" On 2017/04/12 16:15:46, xiyuan wrote: > nit: the "function" seems not necessary. Can we just send result directly > > i.e. > "domAutomationController.send(" > " !!keyboardOverlayData['shortcut']['" + shortcut + "']" > ");" :) Try to make it formal, but yes, I can do it. https://codereview.chromium.org/2814003002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:114: " );", On 2017/04/12 16:15:47, xiyuan wrote: > nit: align with "domAutomationController.send(" Done.
https://codereview.chromium.org/2814003002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc (right): https://codereview.chromium.org/2814003002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:41: base::WrapUnique(new TestWebUIMessageHandler)); nit: base::MakeUnique<TestWebUIMessageHandler>() https://codereview.chromium.org/2814003002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:62: ui::EF_SHIFT_DOWN)) { Thanks for the comment. The "if" is still hard to understand even with the comment. Can we use "==" and avoid "^" when we can to make it easier to read? e.g. const int kDebugModifiers = ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN | ui::EF_SHIFT_DOWN; if (entry.keycode == ui::VKE_MENU || entry.keycode == ui::VKEY_LWIN || entry.modifiers == ui::EF_NONE || entry.modifiers == kDebugModifiers) { ... } https://codereview.chromium.org/2814003002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:80: shortcut = static_cast<char>(DomCodeToUsLayoutCharacter( Can we just do static_cast<char>(LocatedToNonLocatedKeyboardCode(entry.keycode)) ? To DomCode then back, then cast seems unnecessary.
Hi xiyuan, I fixed the code in patch 2. Thanks for the review. Tao https://codereview.chromium.org/2814003002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc (right): https://codereview.chromium.org/2814003002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:41: base::WrapUnique(new TestWebUIMessageHandler)); On 2017/04/12 19:59:45, xiyuan wrote: > nit: base::MakeUnique<TestWebUIMessageHandler>() Done. I must copy it from an old code. https://codereview.chromium.org/2814003002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:62: ui::EF_SHIFT_DOWN)) { On 2017/04/12 19:59:45, xiyuan wrote: > Thanks for the comment. > > The "if" is still hard to understand even with the comment. Can we use "==" and > avoid "^" when we can to make it easier to read? > > e.g. > > const int kDebugModifiers = ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN | > ui::EF_SHIFT_DOWN; > if (entry.keycode == ui::VKE_MENU || > entry.keycode == ui::VKEY_LWIN || > entry.modifiers == ui::EF_NONE || > entry.modifiers == kDebugModifiers) { > ... > } Changed to "==". Since ash::kDebugModifiers is not exported, and used once here, the last condition changed to: entry.modifiers == (ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN | ui::EF_SHIFT_DOWN) https://codereview.chromium.org/2814003002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:80: shortcut = static_cast<char>(DomCodeToUsLayoutCharacter( On 2017/04/12 19:59:45, xiyuan wrote: > Can we just do > static_cast<char>(LocatedToNonLocatedKeyboardCode(entry.keycode)) > > ? > > To DomCode then back, then cast seems unnecessary. That is great! Saved many unnecessary calls. Just need to convert it to lowercase.
Mostly good. https://codereview.chromium.org/2814003002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc (right): https://codereview.chromium.org/2814003002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:8: #include "chrome/browser/ui/webui/web_ui_test_handler.h" not used? https://codereview.chromium.org/2814003002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:21: MOCK_METHOD1(HandleDidPaint, void(const base::ListValue*)); Put an empty implementation instead of using gmock since the test does not care about how this method is called or whether it is called. https://codereview.chromium.org/2814003002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:83: std::transform(shortcut.begin(), shortcut.end(), shortcut.begin(), This is only needed when we cast from a KeyboardCode in line 81, right? If so, move it into the "if" above. Also, use base::ToLowerASCII() instead of std::transform.
Hi xiyuan@, Should have fixed everything. Please have another look. Thank you, Tao https://codereview.chromium.org/2814003002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc (right): https://codereview.chromium.org/2814003002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:8: #include "chrome/browser/ui/webui/web_ui_test_handler.h" On 2017/04/12 21:55:50, xiyuan wrote: > not used? This one should be replaced by #include "content/public/browser/web_ui_message_handler.h" https://codereview.chromium.org/2814003002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:21: MOCK_METHOD1(HandleDidPaint, void(const base::ListValue*)); On 2017/04/12 21:55:50, xiyuan wrote: > Put an empty implementation instead of using gmock since the test does not care > about how this method is called or whether it is called. Done. https://codereview.chromium.org/2814003002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:83: std::transform(shortcut.begin(), shortcut.end(), shortcut.begin(), On 2017/04/12 21:55:50, xiyuan wrote: > This is only needed when we cast from a KeyboardCode in line 81, right? If so, > move it into the "if" above. > > Also, use base::ToLowerASCII() instead of std::transform. Done.
lgtm Thank you for being patient with me. https://codereview.chromium.org/2814003002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc (right): https://codereview.chromium.org/2814003002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:19: public: nit: ctor and dtor e.g. TestWebUIMessageHandler() = default; ~TestWebUIMessageHandler() override = default; https://codereview.chromium.org/2814003002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:22: void RegisterMessages() override { nit: add a comment for interface impl // content::WebUIMessageHandler: https://codereview.chromium.org/2814003002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:27: }; nit: Add a private section. Move HandleDidPaint there and add a DISALLOW_COPY_AND_ASSIGN macro. i.e. private: void HandleDidPaint(const base::ListValue*) {} DISALLOW_COPY_AND_ASSIGN(TestWebUIMessageHandler);
lgtm https://codereview.chromium.org/2814003002/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/keyboard_overlay.js (right): https://codereview.chromium.org/2814003002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/keyboard_overlay.js:48: // This mapping is maintained for keyboard_overlay_ui_browsertest. Rather, "for KeyboardOverlayUIBrowserTest." https://codereview.chromium.org/2814003002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc (right): https://codereview.chromium.org/2814003002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:19: public: On 2017/04/12 22:34:23, xiyuan wrote: > nit: ctor and dtor > e.g. > TestWebUIMessageHandler() = default; > ~TestWebUIMessageHandler() override = default; Yes, add them. I'm not 100% certain, but I believe some Clang tools use the fact that the dtor has the override keyword as a trigger to check all the other overrides are in fact, overriding something. https://codereview.chromium.org/2814003002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:31: using KeyboradOverlayUIBrowserTest = InProcessBrowserTest; Typo https://codereview.chromium.org/2814003002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:49: const ash::AcceleratorData entry = ash::kAcceleratorData[i]; Use a const ref.
Done. Thanks to teach me all the details. The example I copied from must be an old example not following all the format. https://codereview.chromium.org/2814003002/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/keyboard_overlay.js (right): https://codereview.chromium.org/2814003002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/keyboard_overlay.js:48: // This mapping is maintained for keyboard_overlay_ui_browsertest. On 2017/04/12 22:40:10, Lei Zhang wrote: > Rather, "for KeyboardOverlayUIBrowserTest." Done. https://codereview.chromium.org/2814003002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc (right): https://codereview.chromium.org/2814003002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:19: public: On 2017/04/12 22:34:23, xiyuan wrote: > nit: ctor and dtor > e.g. > TestWebUIMessageHandler() = default; > ~TestWebUIMessageHandler() override = default; Done. https://codereview.chromium.org/2814003002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:19: public: On 2017/04/12 22:40:10, Lei Zhang wrote: > On 2017/04/12 22:34:23, xiyuan wrote: > > nit: ctor and dtor > > e.g. > > TestWebUIMessageHandler() = default; > > ~TestWebUIMessageHandler() override = default; > > Yes, add them. I'm not 100% certain, but I believe some Clang tools use the fact > that the dtor has the override keyword as a trigger to check all the other > overrides are in fact, overriding something. Acknowledged. https://codereview.chromium.org/2814003002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:22: void RegisterMessages() override { On 2017/04/12 22:34:23, xiyuan wrote: > nit: add a comment for interface impl > // content::WebUIMessageHandler: Done. https://codereview.chromium.org/2814003002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:27: }; On 2017/04/12 22:34:23, xiyuan wrote: > nit: Add a private section. Move HandleDidPaint there and add a > DISALLOW_COPY_AND_ASSIGN macro. > > i.e. > > private: > void HandleDidPaint(const base::ListValue*) {} > > DISALLOW_COPY_AND_ASSIGN(TestWebUIMessageHandler); Done. https://codereview.chromium.org/2814003002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:31: using KeyboradOverlayUIBrowserTest = InProcessBrowserTest; On 2017/04/12 22:40:10, Lei Zhang wrote: > Typo Thank you, fixed. https://codereview.chromium.org/2814003002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc:49: const ash::AcceleratorData entry = ash::kAcceleratorData[i]; On 2017/04/12 22:40:10, Lei Zhang wrote: > Use a const ref. Done.
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
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/2814003002/#ps120001 (title: "Forget to save a change.")
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": 120001, "attempt_start_ts": 1492045718490210,
"parent_rev": "2a486dc748205105abde688b4775f19703b272c6", "commit_rev":
"f3d080598d0e371fb134baaf3f4439b6dd2d8a98"}
Message was sent while issue was closed.
Description was changed from ========== Add test to check all current accelerators having keyboard overlay. Add test to check all current accelerators having keyboard overlay. BUG=706462 TEST=KeyboradOverlayUIBrowserTest.ShouldHaveKeyboardOverlay CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add test to check all current accelerators having keyboard overlay. Add test to check all current accelerators having keyboard overlay. BUG=706462 TEST=KeyboradOverlayUIBrowserTest.ShouldHaveKeyboardOverlay CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2814003002 Cr-Commit-Position: refs/heads/master@{#464273} Committed: https://chromium.googlesource.com/chromium/src/+/f3d080598d0e371fb134baaf3f44... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/f3d080598d0e371fb134baaf3f44... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
