Add BrowserCommandController Interactive Test

chrome/browser/ui/browser_command_controller_interactive_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 <memory>
+#include <string>
+
+#include "base/memory/ptr_util.h"
+#include "base/strings/string_util.h"
+#include "build/build_config.h"
+#include "chrome/app/chrome_command_ids.h"
+#include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_commands.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/interactive_test_utils.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/test/browser_test_utils.h"
+#include "content/public/test/test_utils.h"
+#include "ui/events/keycodes/keyboard_codes.h"
+#include "url/gurl.h"
+
+namespace {
+// The html file to receive key events, prevent defaults and export all the
+// events with "report()" function. It has two magic keys: pressing "S" to enter
+// fullscreen mode; pressing "X" to indicate the end of all the keys.
+constexpr char kFullscreenKeyboardLockHTML[] =
+    "/fullscreen_keyboardlock/fullscreen_keyboardlock.html";

[msw, 2017/06/30 20:17:29]

It seems a bit silly to create a new subdirectory for this one html file, but I
guess that's already the case for fullscreen_mouselock... I'd suggest moving
both to the parent chrome/test/data/ directory, but that's not critical.

[Hzj_jie, 2017/07/01 01:56:47]

Yes, I followed the fullscreen_mouselock. Done.

+
+class TabCountObserver : public TabStripModelObserver {

[msw, 2017/06/30 20:17:28]

Could you replace this class with a simpler helper function, like:
// Wait for the browser to have the expected tab count, or timeout.
bool WaitForTabCount(Browser* browser, int tab_count) {
  const int kMaxTries = 10;
  for (int i = 0; i < kMaxTries && browser->tab_strip_model()->count() !=
tab_count; ++i)
    content::RunAllPendingInMessageLoop();
  return browser->tab_strip_model()->count() == tab_count;
}

[Hzj_jie, 2017/07/01 01:56:47]

Done.

+ public:
+  TabCountObserver(Browser* browser, int expected);
+  ~TabCountObserver() override;
+
+ private:
+  // TabStripModelObserver implementations.
+  void TabInsertedAt(TabStripModel* tab_strip_model,

[msw, 2017/06/30 20:17:29]

Since you use this class to also track the closing of tabs, is it necessary to
watch for TabClosingAt, in case the tab isn't closed synchronously with the
accelerator (and the initial tab count in TabCountObserver's ctor is greater
than the expected number of tabs)? Presumably it's safe to ignore TabDetachedAt
(etc.), since nothing should be detaching tabs in this test (etc.).

[Hzj_jie, 2017/07/01 01:56:47]

Since a function is used to replace the observer, following several comments are
out-of-date.

+                     content::WebContents* contents,
+                     int index,
+                     bool foreground) override;
+
+  const int expected_;
+  bool seen_ = false;

[msw, 2017/06/30 20:17:29]

nit: rename |seen_| to something like |has_expected_tab_count_|

[Hzj_jie, 2017/07/01 01:56:48]

Acknowledged.

+};
+
+TabCountObserver::TabCountObserver(Browser* browser, int expected)
+    : expected_(expected) {
+  DCHECK(browser);
+  seen_ = (browser->tab_strip_model()->count() == expected_);

[msw, 2017/06/30 20:17:28]

nit: parens not needed

[Hzj_jie, 2017/07/01 01:56:46]

Acknowledged.

+  if (!seen_) {

[msw, 2017/06/30 20:17:28]

nit: curlies not needed for one line conditional and body

[Hzj_jie, 2017/07/01 01:56:47]

Acknowledged.

+    browser->tab_strip_model()->AddObserver(this);
+  }
+}
+TabCountObserver::~TabCountObserver() {

[msw, 2017/06/30 20:17:28]

nit: blank line above (or inline the definitions in the declarations above)

[Hzj_jie, 2017/07/01 01:56:46]

Acknowledged.

+  while (!seen_) {

[msw, 2017/06/30 20:17:29]

ditto nit: curlies not needed for one line conditional and body

[Hzj_jie, 2017/07/01 01:56:47]

Acknowledged.

+    content::RunAllPendingInMessageLoop();
+  }
+}
+
+void TabCountObserver::TabInsertedAt(TabStripModel* tab_strip_model,
+                                     content::WebContents* contents,
+                                     int index,
+                                     bool foreground) {
+  if (tab_strip_model->count() == expected_) {

[msw, 2017/06/30 20:17:28]

ditto nit: curlies not needed for one line conditional and body, or do:
  seen_ |= tab_strip_model->count() == expected_;

[Hzj_jie, 2017/07/01 01:56:47]

Acknowledged.

+    seen_ = true;
+  }
+}
+
+}  // namespace
+
+class BrowserCommandControllerInteractiveTest : public InProcessBrowserTest {
+ public:
+  BrowserCommandControllerInteractiveTest() = default;
+  ~BrowserCommandControllerInteractiveTest() override = default;
+
+ protected:
+  // Starts the test page and waits for it to be loaded.
+  void StartTestPage();
+
+  // Sends a control or command + |key| shortcut to the focused window. Shift
+  // modifier will be added once |shift| is true.

[msw, 2017/06/30 20:17:28]

nit: s/once/if/

[Hzj_jie, 2017/07/01 01:56:46]

Done.

+  void SendShortcut(ui::KeyboardCode key, bool shift = false);
+
+  // Sends a control or command + shift + |key| shortcut to the focused window.
+  void SendShiftShortcut(ui::KeyboardCode key);

[msw, 2017/06/30 20:17:28]

nit: I'd opt to just use "SendShortcut(foo, true);" and remove this helper.

[Hzj_jie, 2017/07/01 01:56:47]

My concern is
SendShortcut(ui::VKEY_A, true);
is not as clear as
SendShirtShortcut(ui::VKEY_A);

If you do not have a very strong opinion, I would like to keep it.

+
+  // Sends a fullscreen shortcut to the focused window and wait for the
+  // operation to take effect.
+  void SendFullscreenShortcutAndWait();
+
+  // Sends a KeyS to the focused window to trigger JavaScript fullscreen and
+  // wait for the operation to take effect.
+  void SendJsFullscreenShortcutAndWait();
+
+  // Sends an ESC to the focused window.
+  void SendEscape();
+
+  // Sends an ESC to the focused window to exit JavaScript fullscreen and wait
+  // for the operation to take effect.
+  void SendEscapeAndWaitForExitingFullscreen();
+
+  // Sends a magic KeyX to the focused window to stop the test case and receives
+  // the result.
+  std::string FinishTestAndGetResult();
+};

[msw, 2017/06/30 20:17:28]

nit: add the private:
DISSALLOW_COPY_AND_ASSIGN(BrowserCommandControllerInteractiveTest);

[Hzj_jie, 2017/07/01 01:56:47]

Done.

+
+void BrowserCommandControllerInteractiveTest::StartTestPage() {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(),
+      embedded_test_server()->GetURL(kFullscreenKeyboardLockHTML),
+      WindowOpenDisposition::CURRENT_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+  ASSERT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser()));
+}
+
+void BrowserCommandControllerInteractiveTest::SendShortcut(
+    ui::KeyboardCode key,
+    bool shift /* = false */) {
+#if defined(OS_MACOSX)
+  const bool control_modifier = false;
+  const bool command_modifier = true;
+#else
+  const bool control_modifier = true;
+  const bool command_modifier = false;
+#endif
+  ASSERT_TRUE(ui_test_utils::SendKeyPressSync(browser(),
+      key, control_modifier, shift, false, command_modifier));
+}
+
+void BrowserCommandControllerInteractiveTest::SendShiftShortcut(
+    ui::KeyboardCode key) {
+  SendShortcut(key, true);
+}
+
+void BrowserCommandControllerInteractiveTest::SendFullscreenShortcutAndWait() {
+  content::WindowedNotificationObserver observer(
+      chrome::NOTIFICATION_FULLSCREEN_CHANGED,
+      content::NotificationService::AllSources());
+  // Enter fullscreen.
+#if defined(OS_MACOSX)
+// Command + Shift + F shortcut is not registered to interactive_ui_tests, so

[msw, 2017/06/30 20:17:29]

nit: indent these two comment lines

[Hzj_jie, 2017/07/01 01:56:48]

Done.

+// sending directly a fullscreen command instead.

[msw, 2017/06/30 20:17:28]

nit: "send a fullscreen command directly instead."

[Hzj_jie, 2017/07/01 01:56:47]

Done.

+  ASSERT_TRUE(chrome::ExecuteCommand(browser(), IDC_FULLSCREEN));

[msw, 2017/06/30 20:17:28]

Strange, can you point to a bug or more info about why accelerators (eg. this
and Command+W) are not registered on Mac interactive test environments? +tapted
for advice

[Hzj_jie, 2017/07/01 01:56:47]

I have very limited understanding to MacOSX. But it looks like all the shortcuts
are defined in src/chrome/app/nibs/MainMenu.xib, which is included in to
"chrome_xibs" target, and referred by "chrome_framework" target only.

The conclusion is based on my test: run the test, and types shortcuts when the
window of interactive_ui_tests is showing. Some takes effect, such as ctrl + t,
some not, such as ctrl + w and ctrl + shift + f.

[tapted, 2017/07/03 00:57:47]

This doesn't match my experience.

There is an interactive UI test running on Mac that successfully sends Cmd+w and
other shortcuts. E.g. PermissionBubbleInteractiveUITest.CmdWClosesWindow  --
https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/permission_bubbl...

[Hzj_jie, 2017/07/07 23:18:10]

Found the root cause: interactive_ui_tests window won't be able to receive most
of the shortcuts without executing BringBrowserWindowToFront().

+#elif defined(OS_CHROMEOS)
+  // A dedicated fullscreen key is used on Chrome OS, so sending directly a

[msw, 2017/06/30 20:17:28]

nit: "send a fullscreen command directly instead, to avoid constructing the key
press."

[Hzj_jie, 2017/07/01 01:56:47]

Done.

+  // fullscreen command to avoid hacking the key press.
+  ASSERT_TRUE(chrome::ExecuteCommand(browser(), IDC_FULLSCREEN));
+#else
+  ASSERT_TRUE(ui_test_utils::SendKeyPressSync(
+        browser(), ui::VKEY_F11, false, false, false, false));
+#endif
+  observer.Wait();
+}
+
+void
+BrowserCommandControllerInteractiveTest::SendJsFullscreenShortcutAndWait() {
+  content::WindowedNotificationObserver observer(
+      chrome::NOTIFICATION_FULLSCREEN_CHANGED,

[tapted, 2017/07/03 00:57:47]

This might not be enough for Mac.  We have a
NSWindowFullscreenNotificationWaiter, but it only works in .mm files.  A good
first step would be to get the test working using
ui::test::ScopedFakeNSWindowFullscreen -- this _can_ be used in .cc files (still
guard with #if defined(OS_MACOSX))

Tests that don't use ScopedFakeNSWindowFullscreen tend to be flaky on Mac. There
are only a few tests, specifically for testing fullscreen, which don't use it
(and are not disabled).

We probably do want a test that _doesn't_ use ScopedFakeNSWindowFullscreen, but
it might need to be run manually rather than on the bots.

There's a MANUAL_ prefix that may help --
https://cs.chromium.org/chromium/src/content/public/test/test_launcher.cc?typ...
-- but usually there's an associated .py script to run the test with an
appropriate --gtest_filter


But if you can get these test passing on Mac using ScopedFakeNSWindowFullscreen
then that is preferable to having the tests disabled or MANUAL_ - prefixed.

[Hzj_jie, 2017/07/03 05:08:15]

Emmm, indeed this test passes on Mac OSX, though I believe
ui::test::ScopedFakeNSWindowFullscreen has a totally different implementation.

+      content::NotificationService::AllSources());
+  ASSERT_TRUE(ui_test_utils::SendKeyPressSync(
+        browser(), ui::VKEY_S, false, false, false, false));
+  observer.Wait();
+}
+
+void BrowserCommandControllerInteractiveTest::SendEscape() {
+  ASSERT_TRUE(ui_test_utils::SendKeyPressSync(
+        browser(), ui::VKEY_ESCAPE, false, false, false, false));
+}
+
+void BrowserCommandControllerInteractiveTest
+    ::SendEscapeAndWaitForExitingFullscreen() {
+  content::WindowedNotificationObserver observer(
+      chrome::NOTIFICATION_FULLSCREEN_CHANGED,
+      content::NotificationService::AllSources());
+  SendEscape();
+  observer.Wait();
+}
+
+std::string BrowserCommandControllerInteractiveTest::FinishTestAndGetResult() {
+  // Magic KeyX to stop the test.
+  EXPECT_TRUE(ui_test_utils::SendKeyPressSync(browser(),
+      ui::VKEY_X, false, false, false, false));
+  std::string result;
+  EXPECT_TRUE(content::ExecuteScriptAndExtractString(
+      browser()->tab_strip_model()->GetActiveWebContents()->GetRenderViewHost(),
+      "report();",
+      &result));
+#if defined(OS_MACOSX)
+  // On MacOSX command key is used for most of the shortcuts, so replace it with
+  // control to reduce the complexity of comparison of the results.
+  base::ReplaceSubstringsAfterOffset(&result, 0, "MetaLeft", "ControlLeft");
+#endif
+  base::TrimWhitespaceASCII(result, base::TRIM_ALL, &result);
+  return result;
+}
+
+IN_PROC_BROWSER_TEST_F(BrowserCommandControllerInteractiveTest,
+                       ShortcutsShouldTakeEffectInWindowMode) {
+  ASSERT_EQ(browser()->tab_strip_model()->count(), 1);
+  SendShortcut(ui::VKEY_T);
+  { TabCountObserver observer(browser(), 2); }
+  ASSERT_EQ(browser()->tab_strip_model()->count(), 2);
+  SendShortcut(ui::VKEY_T);
+  { TabCountObserver observer(browser(), 3); }
+  ASSERT_EQ(browser()->tab_strip_model()->count(), 3);
+#if !defined(OS_MACOSX)
+  // Command + W is not registered to interactive_ui_tests. So the following
+  // test cases won't work on Mac OSX.
+  SendShortcut(ui::VKEY_W);
+  { TabCountObserver observer(browser(), 2); }
+  ASSERT_EQ(browser()->tab_strip_model()->count(), 2);
+  SendShortcut(ui::VKEY_W);
+  { TabCountObserver observer(browser(), 1); }
+  ASSERT_EQ(browser()->tab_strip_model()->count(), 1);
+#endif
+  SendFullscreenShortcutAndWait();
+  ASSERT_TRUE(browser()->
+      exclusive_access_manager()->
+      fullscreen_controller()->
+      IsFullscreenForBrowser());
+}
+
+IN_PROC_BROWSER_TEST_F(BrowserCommandControllerInteractiveTest,
+                       UnpreservedShortcutsShouldBePreventable) {
+  ASSERT_EQ(browser()->tab_strip_model()->count(), 1);
+  StartTestPage();
+
+  // Print.
+  SendShortcut(ui::VKEY_P);
+  // Print by using system function.
+  SendShiftShortcut(ui::VKEY_P);
+  std::string result = FinishTestAndGetResult();
+  // TODO(zijiehe): Investigate why inconsistent key event sequence has been

[msw, 2017/06/30 20:17:28]

Can you clarify what's unexpected about the reported key event sequences here
and elsewhere? Is it the lack of the "keyup KeyP" events (and keyup for other
non-modifier accelerator keys)? +tapted for thoughts.

[Hzj_jie, 2017/07/01 01:56:47]

IMO, this is an issue in test utilities: the test web page works well on my
machine. Though it does not look like a trivial fix.

[tapted, 2017/07/03 00:57:47]

This probably has something to do with how the test events are generated under
SendKeyPressSync - going into this much detail is probably testing the test
event generator rather than actual production code. Can you perhaps filter out
modifier keys and just check the modifier flags on the events for the
non-control keys? I think this much detail make the test fragile.

[Hzj_jie, 2017/07/03 05:08:14]

Besides the order of modifier keys, the keyup event cannot be caught by
interactive_ui_tests on Mac OSX.
The order of modifier keys can be easily fixed, since it's generated at
https://cs.chromium.org/chromium/src/ui/base/test/ui_controls_mac.mm?type=cs&....
Though I think it may not safe to do so as I cannot guarantee whether there are
other tests which depend on the predefined order. I will try it tomorrow. But I
do not know the reason of lacking keyup event.

[tapted, 2017/07/03 05:52:41]

The point is that if a result in a test is not relevant for what you're trying
to test, you shouldn't check for it. It makes the test fragile without adding
extra code coverage or any benefit for catching real regressions.

The print preview dialog triggers on key *down* on all platforms. The key up is
not relevant: Chrome ignores it. Likewise, Chrome does not react to changes in
Command/Control/Shift modifier keys.

Here, it looks like you can test for two specific events to occur:
 1) A KeyDown for "p" with modifier flags indicating that Control is down at the
same time
 2) A KeyDown for "p" with modifier flags indicating that both Control and Shift
were down at the same time


As well as adding fragility, testing for other things makes it harder to tell
why the test exists - i.e. why it's important that it shouldn't fail.

I this case, the behaviour for keup on Mac seems to be intentional -
http://crbug.com/28089  mentions it. Basically Safari doesn't send keyup when a
command modifier is present, so Chrome doesn't either. See also
http://crbug.com/126282 and a few others if you search for OS=Mac + "keyup"

[Hzj_jie, 2017/07/03 20:08:15]

Thank you for the information. Since the keyup is expected to be ignored and the
order of control / shift is also controlled by test utils, the TODO can be
replaced by bug # and link to ui_controls_mac.mm.

This "#if defined(OS_MACOSX)" does not add any fragility: only an inconsistent
behavior on Mac OSX and other platforms.

[tapted, 2017/07/03 23:22:17]

The kind of fragility I'm talking about here comes from over-specifying the
expectation. Ideally, a test only fails if there is a regression in the thing it
is testing, or someone modifies the behavior of the specific thing being tested.
In this case, someone may fix http://crbug.com/28089 so that Chrome's behaviour
is a closer match to Safari (e.g. compare http://unixpapa.com/js/testkey.html
with Chrome and Safari). Fixing http://crbug.com/28089 would involve additional
events appearing on Mac and these expectations to fail. The author then needs to
reason about whether the change represents a regression to your feature or not.

Testing the full key sequence is OK, but it should be in a test called
"BrowserCommandControllerInteractiveTest.VerifyKeyEventSequence". For the other
tests, the full sequence is incidental to whether a regression has occurred: a
change in the sequence is not necessary to detect a regression.

+  // generated on Mac OSX.
+#if defined(OS_MACOSX)
+  ASSERT_EQ(result,
+            "keydown ControlLeft\n"
+            "keydown KeyP\n"
+            "keyup ControlLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown ControlLeft\n"
+            "keydown KeyP\n"
+            "keyup ControlLeft\n"
+            "keyup ShiftLeft\n"
+            "keydown KeyX\n"
+            "keyup KeyX");
+#else
+  ASSERT_EQ(result,
+            "keydown ControlLeft\n"
+            "keydown KeyP\n"
+            "keyup KeyP\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown KeyP\n"
+            "keyup KeyP\n"
+            "keyup ShiftLeft\n"
+            "keyup ControlLeft\n"
+            "keydown KeyX\n"
+            "keyup KeyX");
+#endif
+}
+
+#if defined(OS_MACOSX)
+// TODO(zijiehe): Figure out why this test crashes on Mac OSX. The suspecious

[msw, 2017/06/30 20:17:28]

nit: suspicious

[Hzj_jie, 2017/07/01 01:56:47]

Done.

+// command is "SendFullscreenShortcutAndWait()".
+#define MAYBE(X) DISABLED_##X

[msw, 2017/06/30 20:17:29]

Hmm, bummer this entire test is broken on Mac... tapted?

[tapted, 2017/07/03 00:57:47]

urp - yeah what's the crash stacktrace? There are a bunch fullscreen tests that
are disabled, but they still run fine on Mac when using
--gtest_also_run_disabled_tests 

E.g.

$ ./out/gn_Debug/interactive_ui_tests
--gtest_filter=FullscreenControllerInteractiveTest.*
--gtest_also_run_disabled_tests
/ *snip */
[19/19] FullscreenControllerInteractiveTest.DISABLED_ToggleFullscreenModeForTab
(2373 ms)
SUCCESS: all tests passed.

[Hzj_jie, 2017/07/03 05:08:15]

I tend to believe combining
content::WindowedNotificationObserver observer(
    chrome::NOTIFICATION_FULLSCREEN_CHANGED,
    content::NotificationService::AllSources());
with
SendKeyPressSync()
will trigger the crash.
The crash randomly happens after several SendKeyPressSync() function call.
I will provide a callstack tomorrow, and try if ScopedFakeNSWindowFullscreen
resolves the issue.

[Hzj_jie, 2017/07/07 23:18:10]

Crash stack traces are included in bug http://crbug.com/738949.

+#else
+#define MAYBE(X) X

[msw, 2017/06/30 20:17:28]

Please follow the more common pattern of:
#if defined(OS_MACOSX)
#define MAYBE_KeyEventsShouldBeConsumedByWebPageInBrowserFullscreen
DISABLED_KeyEventsShouldBeConsumedByWebPageInBrowserFullscreen
#else
#define MAYBE_KeyEventsShouldBeConsumedByWebPageInBrowserFullscreen
KeyEventsShouldBeConsumedByWebPageInBrowserFullscreen
#endif
to avoid defining and undefining a more generic MAYBE() macro.

[Hzj_jie, 2017/07/01 01:56:46]

Done.

+#endif
+IN_PROC_BROWSER_TEST_F(
+    BrowserCommandControllerInteractiveTest,
+    MAYBE(KeyEventsShouldBeConsumedByWebPageInBrowserFullscreen)) {
+  ASSERT_EQ(browser()->tab_strip_model()->count(), 1);
+  StartTestPage();
+
+  SendFullscreenShortcutAndWait();
+  // Close tab.

[msw, 2017/06/30 20:17:28]

Rather than just commenting on what the shortcut typically does, can you explain
and/or test the expected outcomes after pressing these accelerators? ie. instead
of say "Close tab." say something like "The tab should not close.", and then
assert that the tab count hasn't changed. Ditto for all the other similar
comments, like "New window." -> "A new window should not be opened.", or put
something above these comments like "None of the accelerators should have an
effect on the browser or tab strip when the page is in fullscreen mode." ditto
in the other test fixtures.

[Hzj_jie, 2017/07/01 01:56:46]

Done.

+  SendShortcut(ui::VKEY_W);
+  // Close window.
+  SendShiftShortcut(ui::VKEY_W);
+  // TODO(zijiehe): ChromeOS has special shortcuts for the following four

[msw, 2017/06/30 20:17:29]

nit: "// TODO(zijiehe): ChromeOS incorrectly handles these; see
crbug.com/737307." ditto elsewhere below.

[Hzj_jie, 2017/07/01 01:56:47]

Done.

+  // commands, which should respect
+  // BrowserCommandController::IsReservedCommandOrKey().
+  // see http://crbug.com/737307.
+#if !defined(OS_CHROMEOS)
+  // New tab.
+  SendShortcut(ui::VKEY_T);
+  // New window.
+  SendShortcut(ui::VKEY_N);
+  // New incognito window.
+  SendShiftShortcut(ui::VKEY_N);
+  // Restore tab.
+  SendShiftShortcut(ui::VKEY_T);
+#endif
+  // Next tab.
+  SendShortcut(ui::VKEY_TAB);
+  // Previous tab.
+  SendShiftShortcut(ui::VKEY_TAB);
+  // Esc.
+  SendEscape();
+
+  std::string result = FinishTestAndGetResult();
+  // TODO(zijiehe): Investigate why inconsistent key event sequence has been
+  // generated on Mac OSX.
+#if defined(OS_MACOSX)
+  ASSERT_EQ(result,
+            "keydown ControlLeft\n"
+            "keydown KeyW\n"
+            "keyup ControlLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown ControlLeft\n"
+            "keydown KeyW\n"
+            "keyup ControlLeft\n"
+            "keyup ShiftLeft\n"
+            "keydown ControlLeft\n"
+            "keydown KeyT\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown KeyN\n"
+            "keyup ControlLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown ControlLeft\n"
+            "keydown KeyN\n"
+            "keyup ControlLeft\n"
+            "keyup ShiftLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown ControlLeft\n"
+            "keydown KeyT\n"
+            "keyup ControlLeft\n"
+            "keyup ShiftLeft\n"
+            "keydown ControlLeft\n"
+            "keydown Tab\n"
+            "keyup ControlLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown ControlLeft\n"
+            "keydown Tab\n"
+            "keyup Tab\n"
+            "keyup ControlLeft\n"
+            "keyup ShiftLeft\n"
+            "keydown Escape\n"
+            "keydown KeyX\n"
+            "keyup KeyX");
+#elif defined(OS_CHROMEOS)
+  ASSERT_EQ(result,
+            "keydown ControlLeft\n"
+            "keydown KeyW\n"
+            "keyup KeyW\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown KeyW\n"
+            "keyup KeyW\n"
+            "keyup ShiftLeft\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown Tab\n"
+            "keyup Tab\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown Tab\n"
+            "keyup Tab\n"
+            "keyup ShiftLeft\n"
+            "keyup ControlLeft\n"
+            "keydown Escape\n"
+            "keyup Escape\n"
+            "keydown KeyX\n"
+            "keyup KeyX");
+#else
+  ASSERT_EQ(result,
+            "keydown ControlLeft\n"
+            "keydown KeyW\n"
+            "keyup KeyW\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown KeyW\n"
+            "keyup KeyW\n"
+            "keyup ShiftLeft\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown KeyT\n"
+            "keyup KeyT\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown KeyN\n"
+            "keyup KeyN\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown KeyN\n"
+            "keyup KeyN\n"
+            "keyup ShiftLeft\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown KeyT\n"
+            "keyup KeyT\n"
+            "keyup ShiftLeft\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown Tab\n"
+            "keyup Tab\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown Tab\n"
+            "keyup Tab\n"
+            "keyup ShiftLeft\n"
+            "keyup ControlLeft\n"
+            "keydown Escape\n"
+            "keyup Escape\n"
+            "keydown KeyX\n"
+            "keyup KeyX");
+#endif
+}
+#undef MAYBE
+
+IN_PROC_BROWSER_TEST_F(
+    BrowserCommandControllerInteractiveTest,
+    KeyEventsShouldBeConsumedByWebPageInJsFullscreenExceptForEsc) {

[msw, 2017/06/30 20:17:28]

Can you split these tests up a bit to avoid redundancy:
1) KeyEventsShouldBeConsumedByWebPageInJsFullscreen (tests all the shortcuts in
JS)
2) EscShouldNotBeConsumedByWebPageInJsFullscreen (just tests Esc)
3) F11ShouldNotBeConsumedByWebPageInJsFullscreen (just tests F11)
Bonus points if you manage to factor out a single helper that tests all the
shortcuts, used by the browser and js fullscreen test fixtures (so we don't have
two big blocks of "keydown...").

[Hzj_jie, 2017/07/01 01:56:46]

Yes, maybe I can use Send* functions to generate the result instead of write
them manually.

+  ASSERT_EQ(browser()->tab_strip_model()->count(), 1);
+  StartTestPage();
+
+  SendJsFullscreenShortcutAndWait();
+  // Close tab.
+  SendShortcut(ui::VKEY_W);
+  // Close window.
+  SendShiftShortcut(ui::VKEY_W);
+  // TODO(zijiehe): ChromeOS has special shortcuts for the following four
+  // commands, which should respect
+  // BrowserCommandController::IsReservedCommandOrKey().
+  // see http://crbug.com/737307.
+#if !defined(OS_CHROMEOS)
+  // New tab.
+  SendShortcut(ui::VKEY_T);
+  // New window.
+  SendShortcut(ui::VKEY_N);
+  // New incognito window.
+  SendShiftShortcut(ui::VKEY_N);
+  // Restore tab.
+  SendShiftShortcut(ui::VKEY_T);
+#endif
+  // Next tab.
+  SendShortcut(ui::VKEY_TAB);
+  // Previous tab.
+  SendShiftShortcut(ui::VKEY_TAB);
+  SendEscapeAndWaitForExitingFullscreen();
+
+  std::string result = FinishTestAndGetResult();
+  // TODO(zijiehe): Investigate why inconsistent key event sequence has been
+  // generated on Mac OSX.
+#if defined(OS_MACOSX)
+  ASSERT_EQ(result,
+            "keydown KeyS\n"
+            "keyup KeyS\n"
+            "keydown ControlLeft\n"
+            "keydown KeyW\n"
+            "keyup ControlLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown ControlLeft\n"
+            "keydown KeyW\n"
+            "keyup ControlLeft\n"
+            "keyup ShiftLeft\n"
+            "keydown ControlLeft\n"
+            "keydown KeyT\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown KeyN\n"
+            "keyup ControlLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown ControlLeft\n"
+            "keydown KeyN\n"
+            "keyup ControlLeft\n"
+            "keyup ShiftLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown ControlLeft\n"
+            "keydown KeyT\n"
+            "keyup ControlLeft\n"
+            "keyup ShiftLeft\n"
+            "keydown ControlLeft\n"
+            "keydown Tab\n"
+            "keyup ControlLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown ControlLeft\n"
+            "keydown Tab\n"
+            "keyup ControlLeft\n"
+            "keyup ShiftLeft\n"
+            "keydown KeyX\n"
+            "keyup KeyX");
+#elif defined(OS_CHROMEOS)
+  ASSERT_EQ(result,
+            "keydown KeyS\n"
+            "keyup KeyS\n"
+            "keydown ControlLeft\n"
+            "keydown KeyW\n"
+            "keyup KeyW\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown KeyW\n"
+            "keyup KeyW\n"
+            "keyup ShiftLeft\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown Tab\n"
+            "keyup Tab\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown Tab\n"
+            "keyup Tab\n"
+            "keyup ShiftLeft\n"
+            "keyup ControlLeft\n"
+            "keydown KeyX\n"
+            "keyup KeyX");
+#else
+  ASSERT_EQ(result,
+            "keydown KeyS\n"
+            "keyup KeyS\n"
+            "keydown ControlLeft\n"
+            "keydown KeyW\n"
+            "keyup KeyW\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown KeyW\n"
+            "keyup KeyW\n"
+            "keyup ShiftLeft\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown KeyT\n"
+            "keyup KeyT\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown KeyN\n"
+            "keyup KeyN\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown KeyN\n"
+            "keyup KeyN\n"
+            "keyup ShiftLeft\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown KeyT\n"
+            "keyup KeyT\n"
+            "keyup ShiftLeft\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown Tab\n"
+            "keyup Tab\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown Tab\n"
+            "keyup Tab\n"
+            "keyup ShiftLeft\n"
+            "keyup ControlLeft\n"
+            "keydown KeyX\n"
+            "keyup KeyX");
+#endif
+}
+
+IN_PROC_BROWSER_TEST_F(
+    BrowserCommandControllerInteractiveTest,
+    KeyEventsShouldBeConsumedByWebPageInJsFullscreenExceptForF11) {
+  ASSERT_EQ(browser()->tab_strip_model()->count(), 1);
+  StartTestPage();
+
+  SendJsFullscreenShortcutAndWait();
+  // Close tab.
+  SendShortcut(ui::VKEY_W);
+  // Close window.
+  SendShiftShortcut(ui::VKEY_W);
+  // TODO(zijiehe): ChromeOS has special shortcuts for the following four
+  // commands, which should respect
+  // BrowserCommandController::IsReservedCommandOrKey().
+  // see http://crbug.com/737307.
+#if !defined(OS_CHROMEOS)
+  // New tab.
+  SendShortcut(ui::VKEY_T);
+  // New window.
+  SendShortcut(ui::VKEY_N);
+  // New incognito window.
+  SendShiftShortcut(ui::VKEY_N);
+  // Restore tab.
+  SendShiftShortcut(ui::VKEY_T);
+#endif
+  // Next tab.
+  SendShortcut(ui::VKEY_TAB);
+  // Previous tab.
+  SendShiftShortcut(ui::VKEY_TAB);
+  SendFullscreenShortcutAndWait();
+
+  std::string result = FinishTestAndGetResult();
+  // TODO(zijiehe): Investigate why inconsistent key event sequence has been
+  // generated on Mac OSX.
+#if defined(OS_MACOSX)
+  ASSERT_EQ(result,
+            "keydown KeyS\n"
+            "keyup KeyS\n"
+            "keydown ControlLeft\n"
+            "keydown KeyW\n"
+            "keyup ControlLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown ControlLeft\n"
+            "keydown KeyW\n"
+            "keyup ControlLeft\n"
+            "keyup ShiftLeft\n"
+            "keydown ControlLeft\n"
+            "keydown KeyT\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown KeyN\n"
+            "keyup ControlLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown ControlLeft\n"
+            "keydown KeyN\n"
+            "keyup ControlLeft\n"
+            "keyup ShiftLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown ControlLeft\n"
+            "keydown KeyT\n"
+            "keyup ControlLeft\n"
+            "keyup ShiftLeft\n"
+            "keydown ControlLeft\n"
+            "keydown Tab\n"
+            "keyup ControlLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown ControlLeft\n"
+            "keydown Tab\n"
+            "keyup ControlLeft\n"
+            "keyup ShiftLeft\n"
+            "keydown KeyX\n"
+            "keyup KeyX");
+#elif defined(OS_CHROMEOS)
+  ASSERT_EQ(result,
+            "keydown KeyS\n"
+            "keyup KeyS\n"
+            "keydown ControlLeft\n"
+            "keydown KeyW\n"
+            "keyup KeyW\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown KeyW\n"
+            "keyup KeyW\n"
+            "keyup ShiftLeft\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown Tab\n"
+            "keyup Tab\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown Tab\n"
+            "keyup Tab\n"
+            "keyup ShiftLeft\n"
+            "keyup ControlLeft\n"
+            "keydown KeyX\n"
+            "keyup KeyX");
+#else
+  ASSERT_EQ(result,
+            "keydown KeyS\n"
+            "keyup KeyS\n"
+            "keydown ControlLeft\n"
+            "keydown KeyW\n"
+            "keyup KeyW\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown KeyW\n"
+            "keyup KeyW\n"
+            "keyup ShiftLeft\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown KeyT\n"
+            "keyup KeyT\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown KeyN\n"
+            "keyup KeyN\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown KeyN\n"
+            "keyup KeyN\n"
+            "keyup ShiftLeft\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown KeyT\n"
+            "keyup KeyT\n"
+            "keyup ShiftLeft\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown Tab\n"
+            "keyup Tab\n"
+            "keyup ControlLeft\n"
+            "keydown ControlLeft\n"
+            "keydown ShiftLeft\n"
+            "keydown Tab\n"
+            "keyup Tab\n"
+            "keyup ShiftLeft\n"
+            "keyup ControlLeft\n"
+            "keydown KeyX\n"
+            "keyup KeyX");
+#endif
+}