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) {
-#if defined(OS_CHROMEOS)
-  // ChromeOS never shows the User Manager on startup.
-  return false;
-#else

[Peter Kasting, 2016/12/20 01:43:49]

It's not obvious to me from the new callers whether it was correct to move this
#ifdef out of this function.  Maybe this is really a question for each of those
new callsites: why is it important to depend on this function in each of those
spots?

[zmin, 2016/12/20 20:32:14]

I have moved the #ifdef to ShouldShowUserManagerOnStartup() from
ShowUserManagerOnStartupIfNeeded().

ShouldShowUserManagerOnStartup() will check whether the last_used_profile is
available. If not we will show UserManager(what we had already) and disable
--load-and-launch-app, --load-apps and --cloud-print-file(in this CL). However,
based on my understanding, user have to choose and login a profile before using
Chrome on ChromeOS. So there is no last_used_profile unavailable issue on
ChromeOS. So I think it makes sense to let ShouldShowUserManagerOnStartup()
always returns false on ChromeOS.

+// Return true if the last used profile must sign in or the last used profile

[Peter Kasting, 2016/12/20 01:43:49]

Nit: Return -> returns

[zmin, 2016/12/20 20:32:13]

Done.

+// was the guest or system profile. Otherwise, return false.

[Peter Kasting, 2016/12/20 01:43:49]

Nit: Remove last sentence, it's implied.

This comment is a bit more "what" than "why".  Maybe state the "why" a little
more instead, e.g.

Returns true if we should show the user manager on startup instead of the last
used profile.

(The "what" can be done as comments within the function.)

[zmin, 2016/12/20 20:32:13]

Done.

+bool ShouldShowUserManagerOnStartup(Profile* last_used_profile) {
   ProfileAttributesEntry* entry = nullptr;
   bool has_entry =
       g_browser_process->profile_manager()
           ->GetProfileAttributesStorage()
           .GetProfileAttributesWithPath(last_used_profile->GetPath(), &entry);
 
   if (!has_entry || !entry->IsSigninRequired()) {

[Peter Kasting, 2016/12/20 01:43:49]

Nit: This equivalent implementation is shorter and, for me, was easier to
understand.

Note the rename of the function parameter, which avoids as much line-wrapping.

bool ShouldShowUserManagerOnStartup(Profile* last_profile) {
  // Avoid reopening profiles that require signin.
  ProfileAttributesEntry* entry = nullptr;
  auto& storage =
      g_browser_process->profile_manager()->GetProfileAttributesStorage();
  if (storage.GetProfileAttributesWithPath(last_profile->GetPath(), &entry) &&
      entry->IsSigninRequired())
    return true;

  // Avoid reopening guest or system profiles, unless a separate process already
  // has a window open for the profile.
  return ((last_profile->IsGuestSession() || last_profile->IsSystemProfile()) &&
          !chrome::GetBrowserCount(last_profile->GetOffTheRecordProfile()));
}

[zmin, 2016/12/20 20:32:14]

Done. But I have changed it little bit:
* Put "g_browser_process->profile_manager()->GetProfileAttributesStorage()" into
the if condition. Because storage is only used in one place and I prefer this
way.
* Use == 0 instead of ! in the return value.  Because the function name is
GetBrowserCount.
* Remove () pair for the return value to keep it in two lines.

     // 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;
     }
   }
+  return true;
+}
+
+// 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.

[Peter Kasting, 2016/12/20 01:43:49]

Nit: Or just "Returns whether the User Manager was shown."

[zmin, 2016/12/20 20:32:14]

Done.

+bool ShowUserManagerOnStartupIfNeeded(Profile* last_used_profile,
+                                      const base::CommandLine& command_line) {
+#if defined(OS_CHROMEOS)
+  // ChromeOS never shows the User Manager on startup.
+  return false;
+#else
+
+  if (!ShouldShowUserManagerOnStartup(last_used_profile)) {

[Peter Kasting, 2016/12/20 01:43:49]

Nit: {} unnecessary (and inconsistent with other places in this file, e.g. line
596)

[zmin, 2016/12/20 20:32:13]

Done.

+    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
@@ skipping 278 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 &&
       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)