Disable browser key reservation in fullscreen mode

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 70 matching lines @@
 #endif
 
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS)
 #include "ui/base/ime/linux/text_edit_key_bindings_delegate_auralinux.h"
 #endif
 
 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 {
 
 ///////////////////////////////////////////////////////////////////////////////
 // BrowserCommandController, public:
 
 BrowserCommandController::BrowserCommandController(Browser* browser)
     : browser_(browser),
       command_updater_(this),
       block_command_execution_(false),
       last_blocked_command_id_(-1),
@@ skipping 79 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 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
   // it is not reserved.
   ui::TextEditKeyBindingsDelegateAuraLinux* delegate =
       ui::GetTextEditKeyBindingsDelegate();
   if (delegate && event.os_event && delegate->MatchEvent(*event.os_event, NULL))
     return false;
 #endif
 
@@ skipping 83 matching lines @@
   }
 
   // The order of commands in this switch statement must match the function
   // declaration order in browser.h!
   switch (id) {
     // Navigation commands
     case IDC_BACKSPACE_BACK:
       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:
       Reload(browser_, disposition);
       break;
     case IDC_RELOAD_CLEARING_CACHE:
       ClearCache(browser_);
       // FALL THROUGH
     case IDC_RELOAD_BYPASSING_CACHE:
       ReloadBypassingCache(browser_, disposition);
@@ skipping 63 matching lines @@
     case IDC_SHOW_AS_TAB:
       ConvertPopupToTabbedBrowser(browser_);
       break;
     case IDC_FULLSCREEN:
       chrome::ToggleFullscreenMode(browser_);
       break;
 
 #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;
     }
 #endif
 
 #if defined(OS_MACOSX)
     case IDC_TOGGLE_FULLSCREEN_TOOLBAR:
       chrome::ToggleFullscreenToolbar(browser_);
       break;
@@ skipping 592 matching lines @@
               bookmarks::prefs::kShowBookmarkBar) &&
           IsShowingMainUI());
 }
 
 void BrowserCommandController::UpdateCommandsForFileSelectionDialogs() {
   UpdateSaveAsState();
   UpdateOpenFileState(&command_updater_);
 }
 
 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);
 
   // 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);
   command_updater_.UpdateCommandEnabled(IDC_FOCUS_LOCATION, show_main_ui);
   command_updater_.UpdateCommandEnabled(IDC_FOCUS_SEARCH, show_main_ui);
   command_updater_.UpdateCommandEnabled(
       IDC_FOCUS_MENU_BAR, main_not_fullscreen);
   command_updater_.UpdateCommandEnabled(
       IDC_FOCUS_NEXT_PANE, main_not_fullscreen);
   command_updater_.UpdateCommandEnabled(
@@ skipping 21 matching lines @@
                                         options_enabled && !guest_session);
 
   command_updater_.UpdateCommandEnabled(IDC_EDIT_SEARCH_ENGINES, show_main_ui);
   command_updater_.UpdateCommandEnabled(IDC_VIEW_PASSWORDS, show_main_ui);
   command_updater_.UpdateCommandEnabled(IDC_ABOUT, show_main_ui);
   command_updater_.UpdateCommandEnabled(IDC_SHOW_APP_MENU, show_main_ui);
 
   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();
 }
 
 void BrowserCommandController::UpdatePrintingState() {
   bool print_enabled = CanPrint(browser_);
   command_updater_.UpdateCommandEnabled(IDC_PRINT, print_enabled);
 #if BUILDFLAG(ENABLE_BASIC_PRINTING)
   command_updater_.UpdateCommandEnabled(IDC_BASIC_PRINT,
                                         CanBasicPrint(browser_));
 #endif  // ENABLE_BASIC_PRINTING
@@ skipping 70 matching lines @@
 
 BrowserWindow* BrowserCommandController::window() {
   return browser_->window();
 }
 
 Profile* BrowserCommandController::profile() {
   return browser_->profile();
 }
 
 }  // namespace chrome