Disable browser key reservation in fullscreen mode

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 0d947e9a4f24cf96a056d565a89e227d60a8f26f..cefed86078e3771b464e93917dd94d020fe8d518 100644
--- a/chrome/browser/ui/browser_command_controller.cc
+++ b/chrome/browser/ui/browser_command_controller.cc
@@ -88,18 +88,6 @@ using content::NavigationEntry;
 using content::NavigationController;
 using content::WebContents;
 
-namespace {
-
-enum WindowState {
-  // Not in fullscreen mode.
-  WINDOW_STATE_NOT_FULLSCREEN,
-
-  // Fullscreen mode, occupying the whole screen.
-  WINDOW_STATE_FULLSCREEN,
-};
-
-}  // namespace
-
 namespace chrome {
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -199,8 +187,23 @@ bool BrowserCommandController::IsReservedCommandOrKey(
   }
 #endif
 
-  if (window()->IsFullscreen() && command_id == IDC_FULLSCREEN)
-    return true;
+  if (window()->IsFullscreen()) {
+    // In full-screen mode, all the keys, except for ESC and F11, should be
+    // delivered to the web page. This has been discussed in the document
+    // Allowing sites to intercept keyboard shortcuts at https://goo.gl/r6XmNt.

[dominickn, 2017/02/08 00:42:38]

Change this comment to be:

In fullscreen, all keys except Esc and F11 should be delivered to the web page.
See <link to blink-dev intent to implement and ship thread>

[Hzj_jie, 2017/02/08 18:29:37]

Done.

+    if (command_id == IDC_FULLSCREEN) {
+      return true;
+    } else if (command_id == IDC_CLOSE_TAB ||
+               command_id == IDC_CLOSE_WINDOW ||
+               command_id == IDC_NEW_INCOGNITO_WINDOW ||
+               command_id == IDC_NEW_TAB ||
+               command_id == IDC_NEW_WINDOW ||
+               command_id == IDC_RESTORE_TAB ||
+               command_id == IDC_SELECT_NEXT_TAB ||
+               command_id == IDC_SELECT_PREVIOUS_TAB) {
+      return false;
+    }
+  }
 
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS)
   // If this key was registered by the user as a content editing hotkey, then
@@ -304,20 +307,20 @@ void BrowserCommandController::ExecuteCommandWithDisposition(
       if (base::FeatureList::IsEnabled(features::kBackspaceGoesBackFeature))
         GoBack(browser_, disposition);
       else
-        browser_->window()->MaybeShowNewBackShortcutBubble(false);

[dominickn, 2017/02/08 00:42:37]

Why did you change these? It seems unrelated to me.

[Hzj_jie, 2017/02/08 18:29:38]

I have done a full document replacement to replace browser_->window() and
browser_->profile() with window() and profile(), which impacts both existing
code and my new code.
It looks like we always need to have an Intent to Implement for each change in
Chrome. I cannot quite tell how the shortcuts (window() and profile()) can be
used eventually, they have already 4.5 years old. Personally I believe these
changes should be covered by any newly added code should always use these
shortcuts. But unfortunately, they were not.

Since I am touching this file, this cleanup looks safe, and it also helps to
keep the consistency.

But anyway, if you have a strong opinion, or the policy in Chrome does not allow
this kind of cleanup to be merged with other changes. I would remove them.

[dominickn, 2017/02/08 23:40:21]

Intents to implement are only required for web-facing behaviour changes. We
needed one to change the keys available in fullscreen because that changes the
behaviour that web developers will see.

Internal Chrome cleanups don't require intents to implement (though they may
need a design document if they are complex). In this case, this isn't complex,
but it is usually a good idea to put cleanups like this in separate CLs. The
main reason is that they make the patch bigger, and can make it more complicated
if a patch has to be reverted. Plus, you don't mention this cleanup in the CL
description (making it hard to find later).

As a general rule: smaller, focused CLs are better. So my advice is to remove
these changes, and submit them separately.

[Hzj_jie, 2017/02/09 02:04:51]

Oh, that would be good. I will revert these changes.

+        window()->MaybeShowNewBackShortcutBubble(false);
       break;
     case IDC_BACK:
-      browser_->window()->HideNewBackShortcutBubble();
+      window()->HideNewBackShortcutBubble();
       GoBack(browser_, disposition);
       break;
     case IDC_BACKSPACE_FORWARD:
       if (base::FeatureList::IsEnabled(features::kBackspaceGoesBackFeature))
         GoForward(browser_, disposition);
       else
-        browser_->window()->MaybeShowNewBackShortcutBubble(true);
+        window()->MaybeShowNewBackShortcutBubble(true);
       break;
     case IDC_FORWARD:
-      browser_->window()->HideNewBackShortcutBubble();
+      window()->HideNewBackShortcutBubble();
       GoForward(browser_, disposition);
       break;
     case IDC_RELOAD:
@@ -401,13 +404,13 @@ void BrowserCommandController::ExecuteCommandWithDisposition(
 #if defined(OS_CHROMEOS)
     case IDC_VISIT_DESKTOP_OF_LRU_USER_2:
     case IDC_VISIT_DESKTOP_OF_LRU_USER_3:
-      ExecuteVisitDesktopCommand(id, browser_->window()->GetNativeWindow());

[dominickn, 2017/02/08 00:42:38]

This seems unrelated

[Hzj_jie, 2017/02/08 18:29:38]

Explained above.

+      ExecuteVisitDesktopCommand(id, window()->GetNativeWindow());
       break;
 #endif
 
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS)
     case IDC_USE_SYSTEM_TITLE_BAR: {
-      PrefService* prefs = browser_->profile()->GetPrefs();

[dominickn, 2017/02/08 00:42:38]

Unrelated?

[Hzj_jie, 2017/02/08 18:29:38]

Ditto.

+      PrefService* prefs = profile()->GetPrefs();
       prefs->SetBoolean(prefs::kUseCustomChromeFrame,
                         !prefs->GetBoolean(prefs::kUseCustomChromeFrame));
       break;
@@ -1020,13 +1023,10 @@ void BrowserCommandController::UpdateCommandsForFileSelectionDialogs() {
 }
 
 void BrowserCommandController::UpdateCommandsForFullscreenMode() {
-  WindowState window_state = WINDOW_STATE_NOT_FULLSCREEN;
-  if (window() && window()->IsFullscreen()) {
-    window_state = WINDOW_STATE_FULLSCREEN;
-  }
-  bool show_main_ui = IsShowingMainUI();
-  bool main_not_fullscreen =
-      show_main_ui && window_state == WINDOW_STATE_NOT_FULLSCREEN;
+  const bool is_fullscreen = (window() && window()->IsFullscreen());
+  const bool is_not_fullscreen = !is_fullscreen;

[dominickn, 2017/02/08 00:42:38]

I don't think you need bools for is_fullscreen and is_not_fullscreen. The only
thing you use is_fullscreen for is computing is_not_fullscreen. Just use:

bool is_not_fullscreen = (window() && !window()->IsFullscreen());

And remove is_fullscreen.

[Hzj_jie, 2017/02/08 18:29:37]

is_fullscreen is also used when calculating fullscreen_enabled. See line 1081.

[dominickn, 2017/02/08 23:40:21]

It just feels a little wasteful to have two boolean variables that are opposites
of one another (one boolean should suffice). Is there something wrong with using
!is_not_fullscreen at line 1081? There's a bunch of code already that  looks
like (!not_something).

[Hzj_jie, 2017/02/09 02:04:51]

No, nothing could be wrong. I would prefer to remove is_not_fullscreen, and use
is_fullscreen & !is_fullscreen everywhere. Double negative is bad.

+  const bool show_main_ui = IsShowingMainUI();
+  const bool main_not_fullscreen = show_main_ui && is_not_fullscreen;
 
   // Navigation commands
   command_updater_.UpdateCommandEnabled(IDC_OPEN_CURRENT_URL, show_main_ui);
@@ -1034,8 +1034,7 @@ void BrowserCommandController::UpdateCommandsForFullscreenMode() {
   // Window management commands
   command_updater_.UpdateCommandEnabled(
       IDC_SHOW_AS_TAB,
-      !browser_->is_type_tabbed() &&
-          window_state == WINDOW_STATE_NOT_FULLSCREEN);
+      !browser_->is_type_tabbed() && is_not_fullscreen);
 
   // Focus various bits of UI
   command_updater_.UpdateCommandEnabled(IDC_FOCUS_TOOLBAR, show_main_ui);
@@ -1077,19 +1076,29 @@ void BrowserCommandController::UpdateCommandsForFullscreenMode() {
   if (base::debug::IsProfilingSupported())
     command_updater_.UpdateCommandEnabled(IDC_PROFILING_ENABLED, show_main_ui);
 
-  bool fullscreen_enabled = true;
 #if !defined(OS_MACOSX)
-  if (window_state == WINDOW_STATE_NOT_FULLSCREEN &&
-      !profile()->GetPrefs()->GetBoolean(prefs::kFullscreenAllowed)) {
-    // Disable toggling into fullscreen mode if disallowed by pref.
-    fullscreen_enabled = false;
-  }
+  // Disable toggling into fullscreen mode if disallowed by pref.
+  const bool fullscreen_enabled = is_fullscreen ||
+      profile()->GetPrefs()->GetBoolean(prefs::kFullscreenAllowed);
+#else
+  const bool fullscreen_enabled = true;
 #endif
 
   command_updater_.UpdateCommandEnabled(IDC_FULLSCREEN, fullscreen_enabled);
   command_updater_.UpdateCommandEnabled(IDC_TOGGLE_FULLSCREEN_TOOLBAR,
                                         fullscreen_enabled);
 
+  command_updater_.UpdateCommandEnabled(IDC_CLOSE_TAB, is_not_fullscreen);
+  command_updater_.UpdateCommandEnabled(IDC_CLOSE_WINDOW, is_not_fullscreen);
+  command_updater_.UpdateCommandEnabled(IDC_NEW_INCOGNITO_WINDOW,
+                                        is_not_fullscreen);
+  command_updater_.UpdateCommandEnabled(IDC_NEW_TAB, is_not_fullscreen);
+  command_updater_.UpdateCommandEnabled(IDC_NEW_WINDOW, is_not_fullscreen);
+  command_updater_.UpdateCommandEnabled(IDC_RESTORE_TAB, is_not_fullscreen);
+  command_updater_.UpdateCommandEnabled(IDC_SELECT_NEXT_TAB, is_not_fullscreen);
+  command_updater_.UpdateCommandEnabled(IDC_SELECT_PREVIOUS_TAB,
+                                        is_not_fullscreen);
+
   UpdateCommandsForBookmarkBar();
 }