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

chrome/browser/ui/browser_command_controller.cc

 // Copyright (c) 2012 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 "chrome/browser/ui/browser_command_controller.h"
 
 #include <stddef.h>
 
 #include <string>
 
@@ skipping 169 matching lines @@
   // reserve browser back/forward and refresh here.
   ui::KeyboardCode key_code =
     static_cast<ui::KeyboardCode>(event.windowsKeyCode);
   if ((key_code == ui::VKEY_BROWSER_BACK && command_id == IDC_BACK) ||
       (key_code == ui::VKEY_BROWSER_FORWARD && command_id == IDC_FORWARD) ||
       (key_code == ui::VKEY_BROWSER_REFRESH && command_id == IDC_RELOAD)) {
     return true;
   }
 #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
   // it is not reserved.
   ui::TextEditKeyBindingsDelegateAuraLinux* delegate =
       ui::GetTextEditKeyBindingsDelegate();
   if (delegate && event.os_event && delegate->MatchEvent(*event.os_event, NULL))
     return false;
 #endif
 
@@ skipping 942 matching lines @@
 
 BrowserWindow* BrowserCommandController::window() {
   return browser_->window();
 }
 
 Profile* BrowserCommandController::profile() {
   return browser_->profile();
 }
 
 }  // namespace chrome