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

[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 ||
@@ -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);
@@ -1049,16 +1054,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);

[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);
@@ -1081,16 +1076,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 +1122,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));