Commands should have consistent behaviors across different modes

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 830 matching lines @@
   // Initialize other commands whose state changes based on various conditions.
   UpdateCommandsForFullscreenMode();
   UpdateCommandsForContentRestrictionState();
   UpdateCommandsForBookmarkEditing();
   UpdateCommandsForIncognitoAvailability();
 }
 
 // static
 void BrowserCommandController::UpdateSharedCommandsForIncognitoAvailability(
     CommandUpdater* command_updater,
-    Profile* profile) {
+    Profile* profile,
+    bool is_fullscreen) {
   const bool guest_session = profile->IsGuestSession();
   // TODO(mlerman): Make GetAvailability account for profile->IsGuestSession().
   IncognitoModePrefs::Availability incognito_availability =
       IncognitoModePrefs::GetAvailability(profile->GetPrefs());
   command_updater->UpdateCommandEnabled(
       IDC_NEW_WINDOW,
-      incognito_availability != IncognitoModePrefs::FORCED);
+      !is_fullscreen && incognito_availability != IncognitoModePrefs::FORCED);
   command_updater->UpdateCommandEnabled(
       IDC_NEW_INCOGNITO_WINDOW,
-      incognito_availability != IncognitoModePrefs::DISABLED && !guest_session);
+      !is_fullscreen &&
+      incognito_availability != IncognitoModePrefs::DISABLED &&
+      !guest_session);
 
   const bool forced_incognito =
       incognito_availability == IncognitoModePrefs::FORCED ||
       guest_session;  // Guest always runs in Incognito mode.
   command_updater->UpdateCommandEnabled(
       IDC_SHOW_BOOKMARK_MANAGER,
       browser_defaults::bookmarks_enabled && !forced_incognito);
   ExtensionService* extension_service =
       extensions::ExtensionSystem::Get(profile)->extension_service();
   const bool enable_extensions =
       extension_service && extension_service->extensions_enabled();
 
   // Bookmark manager and settings page/subpages are forced to open in normal
   // mode. For this reason we disable these commands when incognito is forced.
   command_updater->UpdateCommandEnabled(IDC_MANAGE_EXTENSIONS,
                                         enable_extensions && !forced_incognito);
 
   command_updater->UpdateCommandEnabled(IDC_IMPORT_SETTINGS, !forced_incognito);
   command_updater->UpdateCommandEnabled(IDC_OPTIONS,
                                         !forced_incognito || guest_session);
   command_updater->UpdateCommandEnabled(IDC_SHOW_SIGNIN, !forced_incognito);
 }
 
 void BrowserCommandController::UpdateCommandsForIncognitoAvailability() {
-  UpdateSharedCommandsForIncognitoAvailability(&command_updater_, profile());
+  const bool is_fullscreen = window() && window()->IsFullscreen();
+  UpdateSharedCommandsForIncognitoAvailability(
+      &command_updater_, profile(), is_fullscreen);
 
   if (!IsShowingMainUI()) {
     command_updater_.UpdateCommandEnabled(IDC_IMPORT_SETTINGS, false);

[msw, 2017/02/14 22:34:50]

aside: these two commands (IDC_IMPORT_SETTINGS and IDC_OPTIONS) are already
redundantly/conflictingly set in UpdateSharedCommandsForIncognitoAvailability

[Hzj_jie, 2017/02/15 02:00:49]

I think this is by design, since UpdateSharedCommandsForIncognitoAvailability()
is also used in app_controller_mac.mm, it won't cover the IsShowingMainUI()
scenario.

[msw, 2017/02/15 18:40:51]

It seems like the pattern here is to call
UpdateSharedCommandsForIncognitoAvailability without state information, and then
use state information to disable some commands that would otherwise be enabled.
For example, UpdateSharedCommandsForIncognitoAvailability might enable
IDC_IMPORT_SETTINGS, and then this function would disable it if
!IsShowingMainUI(). Perhaps we should do the same for the commands that need to
be disabled during fullscreen. So, you should probably remove the
|is_fullscreen| argument to UpdateSharedCommandsForIncognitoAvailability and at
the end of this function do:

if (window() && window()->IsFullscreen()) {
  command_updater->UpdateCommandEnabled(IDC_NEW_WINDOW, false);
  command_updater->UpdateCommandEnabled(IDC_NEW_INCOGNITO_WINDOW, false);
}

[Hzj_jie, 2017/02/15 21:20:16]

Good suggestion. Thank you.

     command_updater_.UpdateCommandEnabled(IDC_OPTIONS, false);
   }
 }
 
 void BrowserCommandController::UpdateCommandsForTabState() {
   WebContents* current_web_contents =
       browser_->tab_strip_model()->GetActiveWebContents();
   if (!current_web_contents)  // May be NULL during tab restore.
     return;
 
@@ skipping 148 matching lines @@
   command_updater_.UpdateCommandEnabled(IDC_FEEDBACK, show_main_ui);
 #endif
   UpdateShowSyncState(show_main_ui);
 
   // Settings page/subpages are forced to open in normal mode. We disable these
   // commands for guest sessions and when incognito is forced.
   const bool options_enabled = show_main_ui &&
       IncognitoModePrefs::GetAvailability(
           profile()->GetPrefs()) != IncognitoModePrefs::FORCED;
   const bool guest_session = profile()->IsGuestSession();
   command_updater_.UpdateCommandEnabled(IDC_OPTIONS, options_enabled);

[msw, 2017/02/14 22:34:50]

These two commands (IDC_OPTIONS and IDC_IMPORT_SETTINGS) are already
redundantly/conflictingly set in UpdateCommandsForIncognitoAvailability and
UpdateSharedCommandsForIncognitoAvailability

[Hzj_jie, 2017/02/15 02:00:49]

These several lines are duplicated with
UpdateCommandsForIncognitoAvailability(), but have slightly difference: Whether
IDC_OPTIONS should be enabled in guest_session. I tend to believe logic in
UpdateSharedCommandsForIncognitoAvailability() is correct, since it matches test
cases. I have removed these lines.
Thank you for the reminding.

   command_updater_.UpdateCommandEnabled(IDC_IMPORT_SETTINGS,
                                         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);
 
 #if !defined(OS_MACOSX)
   // 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);
   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();
+  UpdateCommandsForIncognitoAvailability();
+  UpdateTabRestoreCommandState();
 }
 
 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 18 matching lines @@
   command_updater->UpdateCommandEnabled(IDC_OPEN_FILE, enabled);
 }
 
 void BrowserCommandController::UpdateReloadStopState(bool is_loading,
                                                      bool force) {
   window()->UpdateReloadStopState(is_loading, force);
   command_updater_.UpdateCommandEnabled(IDC_STOP, is_loading);
 }
 
 void BrowserCommandController::UpdateTabRestoreCommandState() {
+  const bool is_fullscreen = window() && window()->IsFullscreen();
   sessions::TabRestoreService* tab_restore_service =
       TabRestoreServiceFactory::GetForProfile(profile());
   // The command is enabled if the service hasn't loaded yet to trigger loading.
   // The command is updated once the load completes.
   command_updater_.UpdateCommandEnabled(
       IDC_RESTORE_TAB,
+      !is_fullscreen &&
       tab_restore_service &&
       (!tab_restore_service->IsLoaded() ||
        GetRestoreTabType(browser_) != TabStripModelDelegate::RESTORE_NONE));
 }
 
 void BrowserCommandController::UpdateCommandsForFind() {
   TabStripModel* model = browser_->tab_strip_model();
   bool enabled = !model->IsTabBlocked(model->active_index()) &&
                  !browser_->is_devtools();
 
@@ skipping 25 matching lines @@
 
 BrowserWindow* BrowserCommandController::window() {
   return browser_->window();
 }
 
 Profile* BrowserCommandController::profile() {
   return browser_->profile();
 }
 
 }  // namespace chrome