Disable --load-and-launch-app, --load-apps and --cloud-print-file if last_used_profile is not avail…

chrome/browser/ui/startup/startup_browser_creator.cc

Index: chrome/browser/ui/startup/startup_browser_creator.cc
diff --git a/chrome/browser/ui/startup/startup_browser_creator.cc b/chrome/browser/ui/startup/startup_browser_creator.cc
index 9caec3b3b51986a3d74f665f9aa47bbd0971989f..bed9aa0829dc8e15f516a66d8ff0e29ff7617e2d 100644
--- a/chrome/browser/ui/startup/startup_browser_creator.cc
+++ b/chrome/browser/ui/startup/startup_browser_creator.cc
@@ -262,34 +262,34 @@ void DumpBrowserHistograms(const base::FilePath& output_file) {
                   static_cast<int>(output_string.size()));
 }
 
-// Shows the User Manager on startup if the last used profile must sign in or
-// if the last used profile was the guest or system profile.
-// Returns true if the User Manager was shown, false otherwise.
-bool ShowUserManagerOnStartupIfNeeded(
-    Profile* last_used_profile, const base::CommandLine& command_line) {
+// Returns true if we should show the user manager on startup.
+bool ShouldShowUserManagerOnStartup(Profile* last_profile) {
 #if defined(OS_CHROMEOS)
   // ChromeOS never shows the User Manager on startup.
   return false;
 #else
+  // Avoid reopen profile that requires sign in.

[Peter Kasting, 2016/12/20 23:21:09]

Nit: You changed my suggested comments, which is fine, but your changes
introduced grammar/mechanics/spelling errors.  Here, for example, you'd want to
say "...reopening a profile...".  The comment below has at least three different
issues.  Please fix these carefully, or else use the comment suggestions I
provided.

[zmin, 2016/12/21 00:09:50]

Done.

   ProfileAttributesEntry* entry = nullptr;
-  bool has_entry =
-      g_browser_process->profile_manager()
+  if (g_browser_process->profile_manager()
           ->GetProfileAttributesStorage()
-          .GetProfileAttributesWithPath(last_used_profile->GetPath(), &entry);
-
-  if (!has_entry || !entry->IsSigninRequired()) {
-    // Signin is not required. However, guest, system or locked profiles cannot
-    // be re-opened on startup. The only exception is if there's already a Guest
-    // window open in a separate process (for example, launching a new browser
-    // after clicking on a downloaded file in Guest mode).
-    if ((!last_used_profile->IsGuestSession() &&
-         !last_used_profile->IsSystemProfile()) ||
-        (chrome::GetBrowserCount(last_used_profile->GetOffTheRecordProfile()) >
-         0)) {
-      return false;
-    }
+          .GetProfileAttributesWithPath(last_profile->GetPath(), &entry) &&
+      entry->IsSigninRequired()) {
+    return true;
   }
 
+  // Avoid reopen guest or sysmte profile, unless a separate process already has
+  // a window open for the profile,
+  return (last_profile->IsGuestSession() || last_profile->IsSystemProfile()) &&
+         (chrome::GetBrowserCount(last_profile->GetOffTheRecordProfile()) == 0);
+#endif
+}
+
+// Returns whether the User Manager was shown.
+bool ShowUserManagerOnStartupIfNeeded(Profile* last_used_profile,
+                                      const base::CommandLine& command_line) {
+  if (!ShouldShowUserManagerOnStartup(last_used_profile))
+    return false;
+
   // Show the User Manager.
   profiles::UserManagerAction action =
       command_line.HasSwitch(switches::kShowAppList) ?
@@ -298,7 +298,6 @@ bool ShowUserManagerOnStartupIfNeeded(
   UserManager::Show(
       base::FilePath(), profiles::USER_MANAGER_NO_TUTORIAL, action);
   return true;
-#endif
 }
 
 }  // namespace
@@ -587,11 +586,14 @@ bool StartupBrowserCreator::ProcessCmdLineImpl(
   }
 
   bool silent_launch = false;
+  bool should_show_user_manager =
+      ShouldShowUserManagerOnStartup(last_used_profile);
 
 #if BUILDFLAG(ENABLE_PRINT_PREVIEW)
   // If we are just displaying a print dialog we shouldn't open browser
   // windows.
   if (command_line.HasSwitch(switches::kCloudPrintFile) &&
+      !should_show_user_manager &&

[Peter Kasting, 2016/12/20 23:21:09]

Should this code have previously been checking !ShouldLaunchIncognito(...) like
the other two spots you've modified do?

If so, then we could factor the combined check out into some single temp which
would express "safe to use a previous profile", which would in turn make it
easier to be clear about why these options need this.

My concern with the existing patch is that it's still not immediately clear as a
reader why these lines actually want to check this variable -- that a particular
feature needs to use some profile data, so it needs to be able to get at the
profile data, so we need to check this flag to make sure that data can be
obtained.  We could add comments at each site noting some of this, but this
would be easier to do if each spot did the exact same checks, as then all the
constraints could be expressed in one place on the construction of that temp.

[zmin, 2016/12/21 00:09:50]

I just did a quick test,--cloud-print-file will ignore --incognito option and
launch chrome in the normal mode.
However, if incognito mode is enabled by IncognitoModeAvailability policy,
Chrome will just do silent launch.
In the other word, incognito mode doesn't compatible with cloud print. However,
the priority is
incognito mode policy > cloud-print cmd option > incognito cmd option.

Checking !ShouldLaunchIncognito(...) here means change the priority to
incognito mode policy > incognito cmd option > cloud-print cmd option
So I'm not sure if you're ok with that.
 
Another issue is UserManager will be displayed if the last_used_profile is
locked or guest session. In the mean time, I don't think UserManager cares about
the incognito mode.

However, just as you said below, I think it's still a good idea to put them into
one function like:
IsLastUsedProfileAvailable(Profile* last_used_profile, bool allow_incognito);
With that, the caller can choose to care about incognito mode or not.

[Peter Kasting, 2016/12/21 00:21:25]

At first glance, that sounds more consistent with how the other options work,
and thus preferable.

       print_dialog_cloud::CreatePrintDialogFromCommandLine(last_used_profile,
                                                            command_line)) {
     silent_launch = true;
@@ -683,6 +685,7 @@ bool StartupBrowserCreator::ProcessCmdLineImpl(
     return true;
 
   if (command_line.HasSwitch(extensions::switches::kLoadApps) &&
+      !should_show_user_manager &&
       !IncognitoModePrefs::ShouldLaunchIncognito(
           command_line, last_used_profile->GetPrefs())) {
     if (!ProcessLoadApps(command_line, cur_dir, last_used_profile))
@@ -699,6 +702,7 @@ bool StartupBrowserCreator::ProcessCmdLineImpl(
 
   // Check for --load-and-launch-app.
   if (command_line.HasSwitch(apps::kLoadAndLaunchApp) &&
+      !should_show_user_manager &&
       !IncognitoModePrefs::ShouldLaunchIncognito(
           command_line, last_used_profile->GetPrefs())) {
     base::CommandLine::StringType path =