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);

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

I should have asked earlier, but do we actually need to enable and disable these
commands when entering/exiting fullscreen? Is it possible that your earlier
changes to IsReservedCommandOrKey would have been sufficient to let those
accelerators be sent to the web content first? It might even be okay if they
were handled as browser/tab commands if they weren't handled by the web content.

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

Yes, patch set 2 of change https://codereview.chromium.org/2636013002/#ps20001
is enough for the functionality in the description. The only concern is whether
inconsistent states of CommandObserver and BrowserCommandController may
introduce unexpected issues.
If you think it should be fine. I am happy to revert the change to
https://codereview.chromium.org/2636013002/#ps20001.

[msw, 2017/02/15 22:38:18]

My naive understanding is that it would be safe to leave the commands enabled,
but return false for IsReservedCommandOrKey... it's worth looking closer, in
order to simplify the code, and to match the intended behavior change (we don't
necessarily want to disable new window creation in fullscreen, we simply want to
offer the relevant input event handling to the fullscreen web content).

   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);
     command_updater_.UpdateCommandEnabled(IDC_OPTIONS, false);
   }
 }
 
 void BrowserCommandController::UpdateCommandsForTabState() {
   WebContents* current_web_contents =
       browser_->tab_strip_model()->GetActiveWebContents();
@@ skipping 145 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);

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

Now that I'm looking closer, I'm not sure this is identical to line 883 + line
895. Line 883 enables IDC_OPTIONS in guest sessions, but this doesn't... You
should seek a reviewer (or PM/UX) that knows the intended behavior.

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

Yes, as I have replied in the previous comment, we have a very tiny difference
here. Personally I believe the one between line 880 & 890 is correct, since it
matches the test cases.
But, yes, I should find a PM/UX to make the decision. Do you happen to know who
is the PM owner of this functionality?

[msw, 2017/02/15 22:38:18]

If it matches the test expectations, then it's probably correct, but it would be
good to follow up with ewald@ (CC'ed), he should hopefully be able to point you
in the right direction.

-  command_updater_.UpdateCommandEnabled(IDC_IMPORT_SETTINGS,

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

Similarly, this disables IDC_IMPORT_SETTINGS for guest sessions, but line 882 +
line 894 would have potentially allowed this command to work for guest
sessions... Please seek a more savvy reviewer.

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

Ditto.

-                                        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