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

chrome/browser/ui/browser_command_controller.cc

Index: chrome/browser/ui/browser_command_controller.cc
diff --git a/chrome/browser/ui/browser_command_controller.cc b/chrome/browser/ui/browser_command_controller.cc
index 125126de6c63eee0ace3dd5f09879de389270d92..a5893c70addb77a940da703148fded6fe7043787 100644
--- a/chrome/browser/ui/browser_command_controller.cc
+++ b/chrome/browser/ui/browser_command_controller.cc
@@ -187,8 +187,27 @@ bool BrowserCommandController::IsReservedCommandOrKey(
   }
 #endif
 
-  if (window()->IsFullscreen() && command_id == IDC_FULLSCREEN)
-    return true;
+  if (window()->IsFullscreen()) {
+    // In fullscreen, all commands except for IDC_FULLSCREEN and IDC_EXIT should
+    // be delivered to the web page. See, intent to implement,
+    // https://goo.gl/4tJ32G.
+    // This behavior is different on Mac OS, which has a unique user-initiated

[msw, 2017/04/04 19:32:48]

nit: move this comment into the OS_MACOSX block, consider revising it to
something like:
"On Mac, allow the browser to handle commands when the toolbar is visible. See
crbug.com/702251"

[msw, 2017/04/04 19:33:27]

Ignore this, sorry.

[Hzj_jie, 2017/04/04 20:54:43]

I think this comment is still relevant. I moved the comments regarding to Mac OS
into the #if defined(OS_MACOSX).

+    // full-screen mode. According to the discussion in http://crbug.com/702251,
+    // the shortcuts will be reserved once the tab strip / Chrome toolbar is
+    // visible.
+    const bool is_exit_fullscreen =
+        (command_id == IDC_EXIT || command_id == IDC_FULLSCREEN);
+#if defined(OS_MACOSX)
+    if (window()->IsToolbarVisible()) {

[dominickn, 2017/03/28 02:01:38]

I'm not sure this works correctly on on Mac, where the toolbar slides in and out
without actually changing the value of IsToolbarVisible().

 * BrowserWindowCocoa::IsToolbarVisible() calls
Browser::SupportsWindowFeature(Browser::FEATURE_TOOLBAR)

 * Browser::SupportsWindowFeature() calls
Browser::SupportsWindowFeatureImpl(TOOLBAR, true);

 * Browser::SupportsWindowFeatureImpl() looks like it will always return
FEATURE_TOOLBAR, since BrowserWindowCocoa::ShouldHideUIForFullscreen() always
returns false (and you're almost always in a tabbed browser since that's
constant).

Have you tested this on Mac? If it works, then great (and my understanding of
the call sequence must be wrong). Otherwise, I think you need to add a new
method on BrowserWindowCocoa that the FullscreenToolbarController (possibly
calling mustShowFullscreenToolbar).

We should also add explicit tests on Mac, possibly based on existing fullscreen
toolbar tests there.

[Hzj_jie, 2017/03/29 00:47:54]

Thank you Dominick. Unfortunately, I do not even have a Mac. I am blindly
changing the Mac specific logic.
But yes, you are correct. The name of the function is a little bit confusing. It
really means CanToolbarBeVisible(). Even in BrowserView implementation, it does
not indicate the visibility of the toolbar.

I will need more time to work on this change.

+      if (command_id == IDC_FULLSCREEN)

[msw, 2017/04/04 19:32:48]

Why not handle IDC_EXIT here?

[msw, 2017/04/04 19:33:27]

Ignore this, sorry.

[Hzj_jie, 2017/04/04 20:54:43]

I think this is by-design. The IDC_EXIT is covered by is_exit_fullscreen boolean
value.

+        return true;
+    } else {
+      return is_exit_fullscreen;
+    }
+#else
+    return is_exit_fullscreen;
+#endif
+  }
 
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS)
   // If this key was registered by the user as a content editing hotkey, then