Fix an inconsistent behavior in BrowserCommandController

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 be7339df18bbbe48ce7c5a76f629db6eb8d5909e..65977110be5676af21b6c3ced266e6a153c73a85 100644
--- a/chrome/browser/ui/browser_command_controller.cc
+++ b/chrome/browser/ui/browser_command_controller.cc
@@ -1046,16 +1046,6 @@ void BrowserCommandController::UpdateCommandsForFullscreenMode() {
 #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);
-  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);
@@ -1077,6 +1067,7 @@ void BrowserCommandController::UpdateCommandsForFullscreenMode() {
                                         fullscreen_enabled);
 
   UpdateCommandsForBookmarkBar();
+  UpdateCommandsForIncognitoAvailability();

[msw, 2017/02/23 19:07:28]

I was slightly worried that this updates the enabled state of additional
commands, beyond IDC_OPTIONS and IDC_IMPORT_SETTINGS (ie. IDC_NEW_WINDOW,
IDC_NEW_INCOGNITO_WINDOW, IDC_SHOW_BOOKMARK_MANAGER, IDC_MANAGE_EXTENSIONS, and
IDC_SHOW_SIGNIN), but since I don't see any other calls in this file to update
those commands with separate (and potentially conflicting) logic, it seems like
this should be okay.

[Hzj_jie, 2017/02/23 22:00:09]

I think a simple rule to avoid the conflict is to ensure every command is set in
exactly one code path. Say, IDC_OPTIONS is set only in
UpdateSharedCommandsForIncognitoAvailability() and
UpdateCommandsForIncognitoAvailability(). So no matter how many times one
command is set, for one state, we always ensure the consistency.

But before this change, the rule has been broken, IDC_OPTIONS is set in both
UpdateCommandsForIncognitoAvailability() code path and
UpdateCommandsForFullscreenMode() code path.

Should we add this into the comment of this file?

[msw, 2017/02/23 22:19:43]

I generally agree, but doubt the effectiveness of a comment. I think it's fine
to land as-is.

 }
 
 void BrowserCommandController::UpdatePrintingState() {