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..7e17c3d06e59b9c27dc1c988cbefddef948b0f7f 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,12 @@ class FullscreenTestBrowserWindow : public TestBrowserWindow,
       ExclusiveAccessBubbleType bubble_type) override {}
   void OnExclusiveAccessUserInput() override {}
 
+  // Setters

[msw, 2017/04/04 22:46:39]

nit: remove this comment, it doesn't help.

[Hzj_jie, 2017/04/05 18:09:16]

Done.

+  void ShowToolbar() { toolbar_showing_ = true; }

[msw, 2017/04/04 22:46:39]

nit: make this set_toolbar_showing(bool showing) { toolbar_showing_ = showing; }

[Hzj_jie, 2017/04/05 18:09:17]

Done.

+
  private:
   bool fullscreen_;
+  bool toolbar_showing_;
   BrowserCommandControllerFullscreenTest* test_browser_;
 
   DISALLOW_COPY_AND_ASSIGN(FullscreenTestBrowserWindow);
@@ -361,6 +368,149 @@ TEST_F(BrowserCommandControllerFullscreenTest,
   EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_SHOW_APP_MENU));
   EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_FULLSCREEN));

[msw, 2017/04/04 22:46:39]

Test IDC_EXIT (and others) here?

[Hzj_jie, 2017/04/05 18:09:17]

Done.

 
+  // In fullscreen, only the exit fullscreen commands are reserved. All other
+  // shortcuts should be delivered to the web page. See https://goo.gl/4tJ32G.

[msw, 2017/04/04 22:46:39]

Ditto, use the full url, or a crbug.com/# addy.

[Hzj_jie, 2017/04/05 18:09:17]

Done.

+  EXPECT_FALSE(browser()->command_controller()->IsReservedCommandOrKey(

[msw, 2017/04/04 22:46:39]

nit: it would be nice to reduce the boilerplate for all these checks, to make
the behavior of the tests/expectations more readily apparent. Consider one or
both of these suggestions:
(1) creating command id array(s) with corresponding expectations (ie. const
int[] commands_reserved_in_fullscreen = { IDC..., IDC... }; ), or an array of
structs with bool values for the expected (per-fullscreen?) reserved/enabled
state, which you can then loop over like RenderTextTest:
  struct CommandExpectations { int command_id; bool reserved; bool enabled; };
  CommandExpectations fullscreen_expectations[] = { {IDC..., true, true}, ... };
  for (size_t i = 0; i < arraysize(fullscreen_expectations); ++i) {
    SCOPED_TRACE(base::StringPrintf("Testing i=[%" PRIuS "] '%d'", i,
fullscreen_expectations[i].command_id));
    EXPECT_EQ(fullscreen_expectations[i].enabled,
chrome::IsCommandEnabled(browser(), fullscreen_expectations[i].command_id));
If the enabled state always matches the reserved state, combine those bools
(2) Caching the BrowserCommandController and NativeWebKeyboardEvent like:
  BrowserCommandController* controller = browser()->command_controller();
  content::NativeWebKeyboardEvent event(blink::WebInputEvent::TypeFirst, 0, 0);
  EXPECT_FALSE(controller->IsReservedCommandOrKey(IDC_CLOSE_TAB, event);

[Hzj_jie, 2017/04/05 18:09:16]

Done.

+      IDC_CLOSE_TAB,
+      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
+  EXPECT_FALSE(browser()->command_controller()->IsReservedCommandOrKey(
+      IDC_CLOSE_WINDOW,
+      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
+  EXPECT_FALSE(browser()->command_controller()->IsReservedCommandOrKey(
+      IDC_NEW_INCOGNITO_WINDOW,
+      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
+  EXPECT_FALSE(browser()->command_controller()->IsReservedCommandOrKey(
+      IDC_NEW_TAB,
+      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
+  EXPECT_FALSE(browser()->command_controller()->IsReservedCommandOrKey(
+      IDC_NEW_WINDOW,
+      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
+  EXPECT_FALSE(browser()->command_controller()->IsReservedCommandOrKey(
+      IDC_SELECT_NEXT_TAB,
+      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
+  EXPECT_FALSE(browser()->command_controller()->IsReservedCommandOrKey(
+      IDC_SELECT_PREVIOUS_TAB,
+      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
+  EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
+      IDC_EXIT,

[msw, 2017/04/04 22:46:39]

Test IDC_FULLSCREEN here?

[Hzj_jie, 2017/04/05 18:09:16]

Done.

+      content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
+
+  // Exit fullscreen.

[msw, 2017/04/04 22:46:39]

If this block is just a duplicate of the one at the top, it'd be extra nice to
use command arrays and loops, instead of copy-pasting this whole thing.

[Hzj_jie, 2017/04/05 18:09:17]

Yes, updated.

+  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(
+      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)));
+
+  // Guest Profiles disallow some options.
+  TestingProfile* testprofile = browser()->profile()->AsTestingProfile();
+  EXPECT_TRUE(testprofile);
+  testprofile->SetGuestSession(true);
+
+  browser()->command_controller()->FullscreenStateChanged();
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_OPTIONS));
+  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_IMPORT_SETTINGS));
+}
+
+#if defined(OS_MACOSX)
+TEST_F(BrowserCommandControllerFullscreenTest,
+       TabShortcutsShouldBePreservedOnMacOSX) {
+  static_cast<FullscreenTestBrowserWindow*>(window())->ShowToolbar();
+
+  // Defaults for a tabbed browser.

[msw, 2017/04/04 22:46:39]

If this is a duplicate of the top of UpdateCommandsForFullscreenMode, it'd be
nice to use the array approach. It'd be even better to merge this test with that
above, with either some extra loops for Mac, or as a parameterized fixture
TEST_P() with a bool value for set_toolbar_showing or similar.

[Hzj_jie, 2017/04/05 18:09:17]

Merged into the previous test.

+  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));
+
+  // Simulate going fullscreen.
+  chrome::ToggleFullscreenMode(browser());
+  ASSERT_TRUE(browser()->window()->IsFullscreen());
+  browser()->command_controller()->FullscreenStateChanged();
+
+  // 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));
+#if defined(GOOGLE_CHROME_BUILD)
+  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), 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));
+
+  // In fullscreen, once the toolbar is visible, browser shortcuts should still
+  // be reserved. See http://crbug.com/702251.
   EXPECT_TRUE(browser()->command_controller()->IsReservedCommandOrKey(
       IDC_CLOSE_TAB,
       content::NativeWebKeyboardEvent(blink::WebInputEvent::TypeFirst, 0, 0)));
@@ -445,6 +595,7 @@ TEST_F(BrowserCommandControllerFullscreenTest,
   EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_OPTIONS));
   EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_IMPORT_SETTINGS));
 }
+#endif
 
 // Ensure that the logic for enabling IDC_OPTIONS is consistent, regardless of
 // the order of entering fullscreen and forced incognito modes. See