Fix an inconsistent behavior in BrowserCommandController

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 1028 matching lines @@
   command_updater_.UpdateCommandEnabled(
       IDC_FOCUS_INFOBARS, main_not_fullscreen);
 
   // Show various bits of UI
   command_updater_.UpdateCommandEnabled(IDC_DEVELOPER_MENU, show_main_ui);
 #if defined(GOOGLE_CHROME_BUILD)
   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);
-  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);
 
   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() {
   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 69 matching lines @@
 
 BrowserWindow* BrowserCommandController::window() {
   return browser_->window();
 }
 
 Profile* BrowserCommandController::profile() {
   return browser_->profile();
 }
 
 }  // namespace chrome