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

 // 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/startup/startup_browser_creator.h"
 
 #include <stddef.h>
 
 #include <algorithm>  // For max().
 #include <limits>
@@ skipping 244 matching lines @@
 // The file is overwritten if it exists. This function should only be called in
 // the blocking pool.
 void DumpBrowserHistograms(const base::FilePath& output_file) {
   base::ThreadRestrictions::AssertIOAllowed();
 
   std::string output_string(base::StatisticsRecorder::ToJSON(std::string()));
   base::WriteFile(output_file, output_string.data(),
                   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);
+          .GetProfileAttributesWithPath(last_profile->GetPath(), &entry) &&
+      entry->IsSigninRequired()) {
+    return true;
+  }
 
-  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;
-    }
-  }
+  // 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) ?
           profiles::USER_MANAGER_SELECT_PROFILE_APP_LAUNCHER :
           profiles::USER_MANAGER_SELECT_PROFILE_NO_ACTION;
   UserManager::Show(
       base::FilePath(), profiles::USER_MANAGER_NO_TUTORIAL, action);
   return true;
-#endif
 }
 
 }  // namespace
 
 StartupBrowserCreator::StartupBrowserCreator()
     : is_default_browser_dialog_suppressed_(false),
       show_main_browser_window_(true) {}
 
 StartupBrowserCreator::~StartupBrowserCreator() {}
 
@@ skipping 268 matching lines @@
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   TRACE_EVENT0("startup", "StartupBrowserCreator::ProcessCmdLineImpl");
 
   DCHECK(last_used_profile);
   if (process_startup) {
     if (command_line.HasSwitch(switches::kDisablePromptOnRepost))
       content::NavigationController::DisablePromptOnRepost();
   }
 
   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;
   }
 #endif  // BUILDFLAG(ENABLE_PRINT_PREVIEW)
 
   if (command_line.HasSwitch(switches::kExplicitlyAllowedPorts)) {
     std::string allowed_ports =
         command_line.GetSwitchValueASCII(switches::kExplicitlyAllowedPorts);
     net::SetExplicitlyAllowedPorts(allowed_ports);
@@ skipping 71 matching lines @@
   // If --no-startup-window is specified and Chrome is already running then do
   // not open a new window.
   if (!process_startup && command_line.HasSwitch(switches::kNoStartupWindow))
     silent_launch = true;
 
   // If we don't want to launch a new browser window or tab we are done here.
   if (silent_launch)
     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))
       return false;
 
     // Return early here to avoid opening a browser window.
     // The exception is when there are no browser windows, since we don't want
     // chrome to shut down.
     // TODO(jackhou): Do this properly once keep-alive is handled by the
     // background page of apps. Tracked at http://crbug.com/175381
     if (chrome::GetBrowserCount(last_used_profile) != 0)
       return true;
   }
 
   // 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 =
         command_line.GetSwitchValueNative(apps::kLoadAndLaunchApp);
 
     if (!apps::AppLoadService::Get(last_used_profile)->LoadAndLaunch(
             base::FilePath(path), command_line, cur_dir)) {
       return false;
     }
 
@@ skipping 276 matching lines @@
     if (!entry->IsSigninRequired()) {
       Profile* profile = profile_manager->GetProfile(entry->GetPath());
       if (profile)
         return profile;
     }
   }
 
   return nullptr;
 }
 #endif  // !defined(OS_CHROMEOS) && !defined(OS_ANDROID)