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 fullscreen, all keys except Esc and F11 should be delivered to the web

[msw, 2017/02/09 18:01:13]

nit: maybe describe this in terms of commands, not keys, as you do in the unit
test? Also, this comment should mention that IDC_EXIT (exiting the app) is still
reserved in fullscreen.

[Hzj_jie, 2017/02/10 01:14:23]

Done.

+    // page. See https://goo.gl/4tJ32G.
+    if (command_id == IDC_FULLSCREEN) {

[msw, 2017/02/09 18:01:13]

If the comment above is correct; why can't we just do:
  return command_id == IDC_FULLSCREEN;
Do we want to still respect 'content editing hotkey' below in fullscreen?

[Hzj_jie, 2017/02/10 01:14:23]

I have updated the comment. IDC_EXIT should also be excluded.

+      return true;
+    } else if (command_id == IDC_CLOSE_TAB ||

[msw, 2017/02/09 18:01:13]

nit: no else after return

[Hzj_jie, 2017/02/10 01:14:23]

Done.

+               command_id == IDC_CLOSE_WINDOW ||

[msw, 2017/02/09 18:01:13]

nit: since most of this block is duplicated below, maybe cache the check as:
bool is_browser_command = command_id == IDC_CLOSE_TAB || ...;
(or maybe you'd prefer the name |is_tab_or_window_command|)
for use here and in the last return statement.

[Hzj_jie, 2017/02/10 01:14:23]

Done.

+               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 799 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());

[msw, 2017/02/09 18:01:13]

nit: parens not needed

[Hzj_jie, 2017/02/10 01:14:23]

Done.

+  const bool show_main_ui = IsShowingMainUI();
+  const bool main_not_fullscreen = show_main_ui && !is_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_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_fullscreen);

[msw, 2017/02/09 18:01:13]

Does this CL need security/UX review beyond the 'intent to implement' thread?
(it seems high-impact and worth checking if security/UX has any concerns)

[Hzj_jie, 2017/02/10 01:14:23]

Dominick has reviewed this already, though I cannot tell whether there are extra
steps we need to take care.

+  command_updater_.UpdateCommandEnabled(IDC_CLOSE_WINDOW, !is_fullscreen);
+  command_updater_.UpdateCommandEnabled(IDC_NEW_INCOGNITO_WINDOW,
+                                        !is_fullscreen);
+  command_updater_.UpdateCommandEnabled(IDC_NEW_TAB, !is_fullscreen);
+  command_updater_.UpdateCommandEnabled(IDC_NEW_WINDOW, !is_fullscreen);
+  command_updater_.UpdateCommandEnabled(IDC_RESTORE_TAB, !is_fullscreen);
+  command_updater_.UpdateCommandEnabled(IDC_SELECT_NEXT_TAB, !is_fullscreen);
+  command_updater_.UpdateCommandEnabled(IDC_SELECT_PREVIOUS_TAB,
+                                        !is_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