Allow keyboard shortcuts can be captured by webpage when toolbar is not visible

chrome/browser/ui/browser_command_controller_unittest.cc

Index: chrome/browser/ui/browser_command_controller_unittest.cc
diff --git a/chrome/browser/ui/browser_command_controller_unittest.cc b/chrome/browser/ui/browser_command_controller_unittest.cc
index 8fa33d207c1d87fb53c6cd94973f1dc894f0b413..5b0f13c4730fd654048506917276a9e669f92979 100644
--- a/chrome/browser/ui/browser_command_controller_unittest.cc
+++ b/chrome/browser/ui/browser_command_controller_unittest.cc
@@ -252,7 +252,9 @@ class FullscreenTestBrowserWindow : public TestBrowserWindow,
  public:
   FullscreenTestBrowserWindow(
       BrowserCommandControllerFullscreenTest* test_browser)
-      : fullscreen_(false), test_browser_(test_browser) {}
+      : fullscreen_(false),
+        toolbar_showing_(false),
+        test_browser_(test_browser) {}
 
   ~FullscreenTestBrowserWindow() override {}
 
@@ -264,6 +266,7 @@ class FullscreenTestBrowserWindow : public TestBrowserWindow,
     fullscreen_ = true;
   }
   void ExitFullscreen() override { fullscreen_ = false; }
+  bool IsToolbarShowing() const override { return toolbar_showing_; }
 
   ExclusiveAccessContext* GetExclusiveAccessContext() override { return this; }
 
@@ -277,8 +280,11 @@ class FullscreenTestBrowserWindow : public TestBrowserWindow,
       ExclusiveAccessBubbleType bubble_type) override {}
   void OnExclusiveAccessUserInput() override {}
 
+  void set_toolbar_showing(bool showing) { toolbar_showing_ = showing; }
+
  private:
   bool fullscreen_;
+  bool toolbar_showing_;
   BrowserCommandControllerFullscreenTest* test_browser_;
 
   DISALLOW_COPY_AND_ASSIGN(FullscreenTestBrowserWindow);
@@ -312,129 +318,191 @@ content::WebContents* FullscreenTestBrowserWindow::GetActiveWebContents() {
 
 TEST_F(BrowserCommandControllerFullscreenTest,
        UpdateCommandsForFullscreenMode) {
-  // Defaults for a tabbed browser.
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_OPEN_CURRENT_URL));
-  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_SHOW_AS_TAB));
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_TOOLBAR));
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_LOCATION));
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_SEARCH));
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_MENU_BAR));
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_NEXT_PANE));
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_PREVIOUS_PANE));
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_BOOKMARKS));
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_DEVELOPER_MENU));
+  const int commands_enabled_in_tab[] = {

[msw, 2017/04/05 23:55:11]

I have a slight preference for a struct with bool values for each of the
relevant expectations. That way we'd have a single array and all of its commands
are tested for each of the conditions. Otherwise, we're prone to some incomplete
testing (ie. we never check if IDC_OPEN_CURRENT_URL is reserved or not). Bonus
points for a nicely formatted table:

struct {
  int command_id;
  bool enabled_in_tab;
  bool reserved_in_tab;
  bool enabled_in_fullscreen;
  bool reserved_in_fullscreen;
} commands[] = {
    //       Command ID       |      in-tab       |     fullscreen    |
    //                        | enabled, reserved | enabled, reserved |
    { IDC_FAKE_COMMAND_1,       true,    false,     true,    false    },
    { IDC_FAKE_COMMAND_EXAMPLE, false,   true,      false,   true     },
};

[Hzj_jie, 2017/04/06 02:37:59]

Done.

+    IDC_OPEN_CURRENT_URL,

[msw, 2017/04/05 23:55:11]

nit: use a consistent indentation level for these arrays.

[Hzj_jie, 2017/04/06 02:37:58]

Weird, git cl format does not format all the lines.

+    IDC_FOCUS_TOOLBAR,
+    IDC_FOCUS_LOCATION,
+    IDC_FOCUS_SEARCH,
+    IDC_FOCUS_MENU_BAR,
+    IDC_FOCUS_NEXT_PANE,
+    IDC_FOCUS_PREVIOUS_PANE,
+    IDC_FOCUS_BOOKMARKS,
+    IDC_DEVELOPER_MENU,
 #if defined(GOOGLE_CHROME_BUILD)
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FEEDBACK));
+    IDC_FEEDBACK,
 #endif
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_OPTIONS));
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_IMPORT_SETTINGS));
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_EDIT_SEARCH_ENGINES));
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_VIEW_PASSWORDS));
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_ABOUT));
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_SHOW_APP_MENU));
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FULLSCREEN));
-
-  // Simulate going fullscreen.
-  chrome::ToggleFullscreenMode(browser());
-  ASSERT_TRUE(browser()->window()->IsFullscreen());
-  browser()->command_controller()->FullscreenStateChanged();
-
+    IDC_OPTIONS,
+    IDC_IMPORT_SETTINGS,
+    IDC_EDIT_SEARCH_ENGINES,
+    IDC_VIEW_PASSWORDS,
+    IDC_ABOUT,
+    IDC_SHOW_APP_MENU,
+    IDC_FULLSCREEN,
+    IDC_CLOSE_TAB,
+    IDC_CLOSE_WINDOW,
+    IDC_NEW_INCOGNITO_WINDOW,
+    IDC_NEW_TAB,
+    IDC_NEW_WINDOW,
+    IDC_SELECT_NEXT_TAB,
+    IDC_SELECT_PREVIOUS_TAB,
+    IDC_EXIT,
+  };
+  const int commands_disabled_in_tab[] = {
+      IDC_SHOW_AS_TAB,
+  };
+  const int commands_enabled_in_fullscreen[] = {
+      IDC_FULLSCREEN,
+      IDC_CLOSE_TAB,
+      IDC_CLOSE_WINDOW,
+      IDC_NEW_INCOGNITO_WINDOW,
+      IDC_NEW_TAB,
+      IDC_NEW_WINDOW,
+      IDC_SELECT_NEXT_TAB,
+      IDC_SELECT_PREVIOUS_TAB,
+      IDC_EXIT,
+  };
   // Most commands are disabled in fullscreen.
-  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_OPEN_CURRENT_URL));
-  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_SHOW_AS_TAB));
-  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_TOOLBAR));
-  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_LOCATION));
-  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_SEARCH));
-  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_MENU_BAR));
-  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_NEXT_PANE));
-  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_PREVIOUS_PANE));
-  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_BOOKMARKS));
-  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_DEVELOPER_MENU));
+  const int commands_disabled_in_fullscreen[] = {
+    IDC_OPEN_CURRENT_URL,
+    IDC_SHOW_AS_TAB,
+    IDC_FOCUS_TOOLBAR,
+    IDC_FOCUS_LOCATION,
+    IDC_FOCUS_SEARCH,
+    IDC_FOCUS_MENU_BAR,
+    IDC_FOCUS_NEXT_PANE,
+    IDC_FOCUS_PREVIOUS_PANE,
+    IDC_FOCUS_BOOKMARKS,
+    IDC_DEVELOPER_MENU,
 #if defined(GOOGLE_CHROME_BUILD)
-  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_FEEDBACK));
+    IDC_FEEDBACK,
 #endif
-  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_OPTIONS));
-  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_IMPORT_SETTINGS));
-  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_EDIT_SEARCH_ENGINES));
-  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_VIEW_PASSWORDS));
-  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_ABOUT));
-  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_SHOW_APP_MENU));
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FULLSCREEN));
-
-  EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
+    IDC_OPTIONS,
+    IDC_IMPORT_SETTINGS,
+    IDC_EDIT_SEARCH_ENGINES,
+    IDC_VIEW_PASSWORDS,
+    IDC_ABOUT,
+    IDC_SHOW_APP_MENU,
+  };
+
+  // The command shortcuts which are reserved by browser.
+  const int commands_reserved_in_tab[] = {
       IDC_CLOSE_TAB,
-      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
-  EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
       IDC_CLOSE_WINDOW,
-      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
-  EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
       IDC_NEW_INCOGNITO_WINDOW,
-      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
-  EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
       IDC_NEW_TAB,
-      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
-  EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
       IDC_NEW_WINDOW,
-      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
-  EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
       IDC_SELECT_NEXT_TAB,
-      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
-  EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
       IDC_SELECT_PREVIOUS_TAB,
-      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
-  EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
       IDC_EXIT,
-      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
-
-  // Exit fullscreen.
-  chrome::ToggleFullscreenMode(browser());
-  ASSERT_FALSE(browser()->window()->IsFullscreen());
-  browser()->command_controller()->FullscreenStateChanged();
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_OPEN_CURRENT_URL));
-  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_SHOW_AS_TAB));
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_TOOLBAR));
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_LOCATION));
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_SEARCH));
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_MENU_BAR));
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_NEXT_PANE));
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_PREVIOUS_PANE));
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FOCUS_BOOKMARKS));
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_DEVELOPER_MENU));
-#if defined(GOOGLE_CHROME_BUILD)
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FEEDBACK));
-#endif
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_OPTIONS));
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_IMPORT_SETTINGS));
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_EDIT_SEARCH_ENGINES));
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_VIEW_PASSWORDS));
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_ABOUT));
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_SHOW_APP_MENU));
-  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FULLSCREEN));
-
-  EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
+  };
+  const int commands_unreserved_in_tab[] = {
+      IDC_FULLSCREEN,
+  };
+  // In fullscreen, only the exit fullscreen commands are reserved. All other
+  // shortcuts should be delivered to the web page. See http://crbug.com/680809.
+  const int commands_reserved_in_fullscreen[] = {
+      IDC_EXIT, IDC_FULLSCREEN,
+  };
+  const int commands_unreserved_in_fullscreen[] = {
       IDC_CLOSE_TAB,
-      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
-  EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
       IDC_CLOSE_WINDOW,
-      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
-  EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
       IDC_NEW_INCOGNITO_WINDOW,
-      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
-  EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
       IDC_NEW_TAB,
-      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
-  EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
       IDC_NEW_WINDOW,
-      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
-  EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
       IDC_SELECT_NEXT_TAB,
-      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
-  EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
       IDC_SELECT_PREVIOUS_TAB,
-      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
+  };
+  const content::NativeWebKeyboardEvent key_event(
+      blink::WebInputEvent::TypeFirst, 0, 0);
+  // Defaults for a tabbed browser.
+  for (size_t i = 0; i < arraysize(commands_enabled_in_tab); i++) {
+    EXPECT_TRUE(

[msw, 2017/04/05 23:55:11]

Have you checked the output for a failure in this test? My guess is that it's
not informative (ie. doesn't state the command id) It'd be nice to have some
SCOPED_TRACE statements to give the command id value (and maybe the array
index?).

[Hzj_jie, 2017/04/06 02:37:59]

Done.

+        chrome::IsCommandEnabled(browser(), commands_enabled_in_tab[i]));
+  }
+  for (size_t i = 0; i < arraysize(commands_disabled_in_tab); i++) {
+    EXPECT_FALSE(
+        chrome::IsCommandEnabled(browser(), commands_disabled_in_tab[i]));
+  }
+  for (size_t i = 0; i < arraysize(commands_reserved_in_tab); i++) {
+    EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
+        commands_reserved_in_tab[i], key_event));
+  }
+  for (size_t i = 0; i < arraysize(commands_unreserved_in_tab); i++) {
+    EXPECT_FALSE(browser()->command_controller()->IsReservedCommandOrKey(
+        commands_unreserved_in_tab[i], key_event));
+  }
+
+  // Simulate going fullscreen.
+  chrome::ToggleFullscreenMode(browser());
+  ASSERT_TRUE(browser()->window()->IsFullscreen());
+  browser()->command_controller()->FullscreenStateChanged();
+
+#if defined(OS_MACOSX)
+  // Once the toolbar is not showing, the behavior on Mac OS should be

[msw, 2017/04/05 23:55:10]

nit: 'When the'

[Hzj_jie, 2017/04/06 02:37:58]

Done.

+  // consistent as other platforms.

[msw, 2017/04/05 23:55:10]

nit: 'consistent with'

[Hzj_jie, 2017/04/06 02:37:58]

Done.

+  static_cast<FullscreenTestBrowserWindow*>(window())->set_toolbar_showing(
+      false);
+#endif
+  for (size_t i = 0; i < arraysize(commands_enabled_in_fullscreen); i++) {
+    EXPECT_TRUE(
+        chrome::IsCommandEnabled(browser(), commands_enabled_in_fullscreen[i]));
+  }
+  for (size_t i = 0; i < arraysize(commands_disabled_in_fullscreen); i++) {
+    EXPECT_FALSE(chrome::IsCommandEnabled(browser(),
+                                          commands_disabled_in_fullscreen[i]));
+  }
+  for (size_t i = 0; i < arraysize(commands_reserved_in_fullscreen); i++) {
+    EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
+        commands_reserved_in_fullscreen[i], key_event));
+  }
+  for (size_t i = 0; i < arraysize(commands_unreserved_in_fullscreen); i++) {
+    EXPECT_FALSE(browser()->command_controller()->IsReservedCommandOrKey(
+        commands_unreserved_in_fullscreen[i], key_event));
+  }
+
+#if defined(OS_MACOSX)
+  // If the toolbar is visible, all the keyboard shortcuts should be preserved

[msw, 2017/04/05 23:55:11]

nit: "When the toolbar is showing, commands should be reserved as if the content
were in a tab; IDC_FULLSCREEN should also be reserved."

[Hzj_jie, 2017/04/06 02:37:59]

Done.

+  // by browser, include IDC_FULLSCREEN.
+  static_cast<FullscreenTestBrowserWindow*>(window())->set_toolbar_showing(
+      true);
+  for (size_t i = 0; i < arraysize(commands_reserved_in_tab); i++) {
+    EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
+        commands_reserved_in_tab[i], key_event));
+  }
   EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
-      IDC_EXIT,
-      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
+      IDC_FULLSCREEN, key_event));
+  for (size_t i = 0; i < arraysize(commands_unreserved_in_tab); i++) {
+    if (commands_unreserved_in_tab[i] != IDC_FULLSCREEN) {

[msw, 2017/04/05 23:55:11]

aside: This is a bit odd, since the only value in |commands_unreserved_in_tab|
is IDC_FULLSCREEN... But I guess that's future-proofing for other items to be
added to that array?

[Hzj_jie, 2017/04/06 02:37:59]

Now all commands are in one structure, we do not have this issue anymore.

+      EXPECT_FALSE(browser()->command_controller()->IsReservedCommandOrKey(
+          commands_unreserved_in_tab[i], key_event));
+    }
+  }
+  // Return to default state.
+  static_cast<FullscreenTestBrowserWindow*>(window())->set_toolbar_showing(
+      false);
+#endif
+
+  // Exit fullscreen.
+  chrome::ToggleFullscreenMode(browser());
+  ASSERT_FALSE(browser()->window()->IsFullscreen());
+  browser()->command_controller()->FullscreenStateChanged();
+
+  for (size_t i = 0; i < arraysize(commands_enabled_in_tab); i++) {
+    EXPECT_TRUE(
+        chrome::IsCommandEnabled(browser(), commands_enabled_in_tab[i]));
+  }
+  for (size_t i = 0; i < arraysize(commands_disabled_in_tab); i++) {
+    EXPECT_FALSE(
+        chrome::IsCommandEnabled(browser(), commands_disabled_in_tab[i]));
+  }
+  for (size_t i = 0; i < arraysize(commands_reserved_in_tab); i++) {
+    EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
+        commands_reserved_in_tab[i], key_event));
+  }
+  for (size_t i = 0; i < arraysize(commands_unreserved_in_tab); i++) {
+    EXPECT_FALSE(browser()->command_controller()->IsReservedCommandOrKey(
+        commands_unreserved_in_tab[i], key_event));
+  }
 
   // Guest Profiles disallow some options.
   TestingProfile* testprofile = browser()->profile()->AsTestingProfile();