Add test to check all current accelerators having 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.
+
+#if defined(USE_ASH)

[xiyuan, 2017/04/12 16:15:46]

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...

[wutao, 2017/04/12 18:06:08]

Removed #if section.

+#include "ash/accelerators/accelerator_table.h"  // nogncheck
+#endif
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/webui/web_ui_test_handler.h"
+#include "chrome/common/url_constants.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/test/browser_test_utils.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "ui/events/keycodes/keyboard_code_conversion.h"
+
+namespace {
+
+class TestWebUIMessageHandler : public content::WebUIMessageHandler {
+ public:
+  MOCK_METHOD1(HandleDidPaint, void(const base::ListValue*));
+
+  void RegisterMessages() override {
+    web_ui()->RegisterMessageCallback(
+        "didPaint", base::Bind(&TestWebUIMessageHandler::HandleDidPaint,
+                               base::Unretained(this)));
+  }
+};
+
+}  // namespace
+
+class KeyboradOverlayUIBrowserTest : public InProcessBrowserTest {

[xiyuan, 2017/04/12 16:15:46]

It seems like you can just do

using KeyboradOverlayUIBrowserTest = InProcessBrowserTest;

since you do not need to override anything for your test.

[wutao, 2017/04/12 18:06:08]

Done.

+ public:
+  KeyboradOverlayUIBrowserTest() {}
+
+  void SetUpOnMainThread() override {
+    InProcessBrowserTest::SetUpOnMainThread();
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(KeyboradOverlayUIBrowserTest,
+                       ShouldHaveKeyboardOverlay) {
+  ui_test_utils::NavigateToURL(browser(),
+                               GURL(chrome::kChromeUIKeyboardOverlayURL));
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  content::WebUIMessageHandler* test_handler = new TestWebUIMessageHandler;
+  web_contents->GetWebUI()->AddMessageHandler(base::WrapUnique(test_handler));

[xiyuan, 2017/04/12 16:15:47]

What is the purpose of |test_handler|? It seems not used.

[wutao, 2017/04/12 18:06:08]

Need TestWebUIMessageHandler to handle message "didPaint". I do not need a
variable for this, so I will remove this variable.

+
+#if defined(USE_ASH)

[xiyuan, 2017/04/12 16:15:46]

get rid of this

[wutao, 2017/04/12 18:06:08]

Done.

+  for (size_t j = 0; j < ash::kAcceleratorDataLength; ++j) {

[xiyuan, 2017/04/12 16:15:46]

for (const auto& entry : ash::kAcceleratorData) {

or if we want to use a loop with a var, start with "i" instead of "j"

[wutao, 2017/04/12 18:06:09]

Cannot use incomplete type as a range, so I will change j to i.

+    const ash::AcceleratorData entry = ash::kAcceleratorData[j];
+    if (!(entry.keycode ^ ui::VKEY_MENU) || !(entry.keycode ^ ui::VKEY_LWIN) ||
+        !(entry.modifiers ^ ui::EF_NONE) ||
+        !(entry.modifiers ^ ui::EF_CONTROL_DOWN ^ ui::EF_ALT_DOWN ^
+          ui::EF_SHIFT_DOWN))

[xiyuan, 2017/04/12 16:15:46]

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 ? 

[wutao, 2017/04/12 18:06:08]

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

+      continue;

[xiyuan, 2017/04/12 16:15:46]

nit: wrap with {} since conditions occupy more than one line.

[wutao, 2017/04/12 18:06:08]

Done.

+
+    ui::KeyboardCode keyboard_code = entry.keycode;
+    unsigned int code = (unsigned int)keyboard_code;

[xiyuan, 2017/04/12 16:15:47]

use static_cast<>. Casting with () is not allowed.

https://google.github.io/styleguide/cppguide.html#Casting

[wutao, 2017/04/12 18:06:08]

Removed.

+    std::string shortcut;
+    ASSERT_TRUE(content::ExecuteScriptAndExtractString(
+        web_contents,
+        "domAutomationController.send("
+        "  (function(number) {"
+        "       if (!!KEYCODE_TO_LABEL[number]) {"
+        "             return KEYCODE_TO_LABEL[number];"
+        "       } else {"
+        "             return 'NONE';"
+        "       }"
+        "    })(" +
+            std::to_string(code) +
+            ")"

[xiyuan, 2017/04/12 16:15:47]

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))

[wutao, 2017/04/12 18:06:08]

git cl format changed this and warn me if I change to a better format :)
Removed |keyboard_code|, |code|.

+            "  );",

[xiyuan, 2017/04/12 16:15:46]

nit: align ");" with the start of "domAutomationController.send("

i.e.

  "domAutomationController.send("
  ....
  ");",

[wutao, 2017/04/12 18:06:08]

Done.

+        &shortcut));
+    if (shortcut == "NONE") {
+      shortcut = DomCodeToUsLayoutCharacter(
+          UsLayoutKeyboardCodeToDomCode(
+              LocatedToNonLocatedKeyboardCode(keyboard_code)),
+          false);
+    }
+
+    if (entry.modifiers & ui::EF_ALT_DOWN)

[xiyuan, 2017/04/12 16:15:46]

nit: document the order of the "if" should not be changed because the modifiers
are expected to be alpha sorted in the generated shortcut.

[wutao, 2017/04/12 18:06:08]

Done.

+      shortcut.append("<>ALT");
+    if (entry.modifiers & ui::EF_CONTROL_DOWN)
+      shortcut.append("<>CTRL");
+    if (entry.modifiers & ui::EF_COMMAND_DOWN)
+      shortcut.append("<>SEARCH");
+    if (entry.modifiers & ui::EF_SHIFT_DOWN)
+      shortcut.append("<>SHIFT");
+
+    bool is_display_ui_scaling_enabled;
+    ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
+        web_contents,
+        "domAutomationController.send(isDisplayUIScalingEnabled());",

[xiyuan, 2017/04/12 16:15:46]

This would not change per accelerator. Could you move it out of the loop to call
it only once?

[wutao, 2017/04/12 18:06:08]

Good catch! Done.

+        &is_display_ui_scaling_enabled));
+    if (!is_display_ui_scaling_enabled) {
+      if (shortcut == "-<>CTRL<>SHIFT" || shortcut == "+<>CTRL<>SHIFT" ||
+          shortcut == "0<>CTRL<>SHIFT")
+        continue;
+    }
+
+    bool contains;
+    ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
+        web_contents,
+        "domAutomationController.send("
+        "  (function() {"

[xiyuan, 2017/04/12 16:15:46]

nit: the "function" seems not necessary. Can we just send result directly

i.e.
  "domAutomationController.send("
  "  !!keyboardOverlayData['shortcut']['" + shortcut + "']"
  ");"

[wutao, 2017/04/12 18:06:08]

:) Try to make it formal, but yes, I can do it.

+        "       return !!keyboardOverlayData['shortcut']['" +
+            shortcut +
+            "'];"
+            "    })()"
+            "  );",

[xiyuan, 2017/04/12 16:15:47]

nit: align with "domAutomationController.send("

[wutao, 2017/04/12 18:06:08]

Done.

+        &contains));
+    ASSERT_TRUE(contains) << "Please add the new accelerators to keyboard "
+                             "overlay. Add one entry '" +
+                                 shortcut +
+                                 "' in the 'shortcut' section"
+                                 " in the file of "
+                                 "'/chrome/browser/resources/chromeos/"
+                                 "keyboard_overlay_data.js'. Please keep it in "
+                                 "alphabetical order.";
+  }
+#endif
+}