Commands should have consistent behaviors across different modes

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 b59372d2c58d305beef3ecd49f680e4c1cefacf8..3032b16ad5ad9d41801f7c600f3a2a629fe3edf1 100644
--- a/chrome/browser/ui/browser_command_controller.cc
+++ b/chrome/browser/ui/browser_command_controller.cc
@@ -848,17 +848,20 @@ void BrowserCommandController::InitCommandState() {
 // 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 ||
@@ -883,7 +886,9 @@ void BrowserCommandController::UpdateSharedCommandsForIncognitoAvailability(
 }
 
 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.

@@ -1081,16 +1086,14 @@ void BrowserCommandController::UpdateCommandsForFullscreenMode() {
 
   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() {
@@ -1129,12 +1132,14 @@ void BrowserCommandController::UpdateReloadStopState(bool 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));