Refactoring startup logic for upcoming FRE changes (non-Win 10).

chrome/browser/ui/startup/startup_browser_creator_impl.cc

Index: chrome/browser/ui/startup/startup_browser_creator_impl.cc
diff --git a/chrome/browser/ui/startup/startup_browser_creator_impl.cc b/chrome/browser/ui/startup/startup_browser_creator_impl.cc
index 272d6786e7ee274b7e52a5da8436f06aa65271a6..ba04dc8a04f303328f00ddc44affec01e5f1b895 100644
--- a/chrome/browser/ui/startup/startup_browser_creator_impl.cc
+++ b/chrome/browser/ui/startup/startup_browser_creator_impl.cc
@@ -18,6 +18,7 @@
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/environment.h"
+#include "base/feature_list.h"
 #include "base/lazy_instance.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/statistics_recorder.h"
@@ -70,6 +71,7 @@
 #include "chrome/browser/ui/startup/obsolete_system_infobar_delegate.h"
 #include "chrome/browser/ui/startup/session_crashed_infobar_delegate.h"
 #include "chrome/browser/ui/startup/startup_browser_creator.h"
+#include "chrome/browser/ui/startup/startup_features.h"
 #include "chrome/browser/ui/tabs/pinned_tab_codec.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/chrome_constants.h"
@@ -260,6 +262,12 @@ const Extension* GetPlatformApp(Profile* profile,
   return extension && extension->is_platform_app() ? extension : NULL;
 }
 
+// Appends the contents of |from| to the end of |to|.
+void AppendTabs(const StartupTabs& from, StartupTabs* to) {
+  if (!from.empty())
+    to->insert(to->end(), from.begin(), from.end());
+}
+
 }  // namespace
 
 namespace internals {
@@ -346,7 +354,11 @@ bool StartupBrowserCreatorImpl::Launch(Profile* profile,
     RecordLaunchModeHistogram(urls_to_open.empty() ?
                               LM_TO_BE_DECIDED : LM_WITH_URLS);
 
-    ProcessLaunchURLs(process_startup, urls_to_open);
+    if (base::FeatureList::IsEnabled(features::kUseConsolidatedStartupFlow)) {

[Peter Kasting, 2016/10/06 05:49:08]

Nit: No {}

+      ProcessLaunchUrlsUsingConsolidatedFlow(process_startup, urls_to_open);
+    } else {
+      ProcessLaunchURLs(process_startup, urls_to_open);
+    }
 
     if (command_line_.HasSwitch(switches::kInstallChromeApp)) {
       install_chrome_app::InstallChromeApp(
@@ -508,15 +520,6 @@ void StartupBrowserCreatorImpl::ProcessLaunchURLs(
   // Determine whether or not this launch must include the welcome page.
   InitializeWelcomeRunType(urls_to_open);
 
-// TODO(tapted): Move this to startup_browser_creator_win.cc after refactor.
-#if defined(OS_WIN)
-  if (base::win::GetVersion() >= base::win::VERSION_WIN8) {
-    // See if there are apps for this profile that should be launched on startup
-    // due to a switch from Metro mode.
-    app_metro_launch::HandleAppLaunchForMetroRestart(profile_);
-  }
-#endif
-
   if (process_startup && ProcessStartupURLs(urls_to_open)) {
     // ProcessStartupURLs processed the urls, nothing else to do.
     return;
@@ -568,6 +571,105 @@ void StartupBrowserCreatorImpl::ProcessLaunchURLs(
   AddInfoBarsIfNecessary(browser, is_process_startup);
 }
 
+void StartupBrowserCreatorImpl::ProcessLaunchUrlsUsingConsolidatedFlow(
+    bool process_startup,
+    const std::vector<GURL>& cmd_line_urls) {
+  // Don't open any browser windows if starting up in "background mode".
+  if (process_startup && command_line_.HasSwitch(switches::kNoStartupWindow))
+    return;
+
+  StartupTabs cmd_line_tabs;
+  UrlsToTabs(cmd_line_urls, &cmd_line_tabs);
+
+  bool is_incognito = IncognitoModePrefs::ShouldLaunchIncognito(
+      command_line_, profile_->GetPrefs());
+  bool is_post_crash_launch = HasPendingUncleanExit(profile_);
+  StartupTabs tabs =
+      DetermineStartupTabs(StartupTabProviderImpl(), cmd_line_tabs,
+                           is_incognito, is_post_crash_launch);
+
+  // TODO(tmartino): If this is not process startup, attempt to restore
+  // asynchronously and return here. This logic is self-contained in
+  // SessionService and therefore can't be combined with the other Browser
+  // creation logic.
+
+  // TODO(tmartino): Function which determines what behavior of session
+  // restore, if any, is necessary, and passes the result to a new function
+  // which opens tabs in a restored or newly-created Browser accordingly.
+  // Incorporates code from ProcessStartupUrls.
+
+  Browser* browser = OpenTabsInBrowser(nullptr, process_startup, tabs);
+
+  // Finally, add info bars.
+  AddInfoBarsIfNecessary(
+      browser, process_startup ? chrome::startup::IS_PROCESS_STARTUP
+                               : chrome::startup::IS_NOT_PROCESS_STARTUP);
+}
+
+StartupTabs StartupBrowserCreatorImpl::DetermineStartupTabs(
+    const StartupTabProvider& provider,
+    const StartupTabs& cmd_line_tabs,
+    bool is_incognito,
+    bool is_post_crash_launch) {
+  StartupTabs tabs;
+
+  // Only the New Tab Page or command line URLs may be shown in incognito mode.
+  // A similar policy exists for crash recovery launches, to prevent getting the
+  // user stuck in a crash loop.
+  if (is_incognito || is_post_crash_launch) {
+    if (cmd_line_tabs.empty()) {
+      tabs.emplace_back(GURL(chrome::kChromeUINewTabURL), false);
+      return tabs;
+    }
+    return cmd_line_tabs;

[Peter Kasting, 2016/10/06 05:49:08]

Nit: Hmm, I made an error in the previous suggestion I made.  The code doesn't
actually work.  Your version does, but at the cost of two return statements and
some extra {} that end up losing the readability win over the old thing.

One way to get back to very short code here would be to have
DetermineStartupTabs() take |cmd_line_tabs| by value instead of const ref, then
change this to:

    if (cmd_line_tabs.empty())
      cmd_line_tabs.emplace_back(GURL(chrome::kChromeUINewTabURL), false);
    return cmd_line_tabs;

This allows moving the |tabs| declaration back down.

Otherwise, I would probably go back to the old route (except for eliminate the
"else" after the first "return" as I previously suggested).  This also allows
moving |tabs| back down, so I guess I would move it back down regardless.  :P 
See next comment also.

Sorry about this.

[tmartino, 2016/10/06 18:27:28]

Went back to the old way, mostly out of preference for const ref.

+  }
+

[Peter Kasting, 2016/10/06 05:49:08]

Nit: Some reordering of these next few blocks produces equivalent behavior but
avoids testing for cmd_line_tabs.empty() repeatedly, which feels a bit
confusing.  Overall, I find this logic clearer.  (It is also slightly shorter.) 
I rewrote the comments a little to better clarify the new order.

  // A trigger on a profile may indicate that we should show a tab which
  // offers to reset the user's settings.  When this appears, it is first, and
  // may be shown alongside command-line tabs.
  StartupTabs tabs = provider.GetResetTriggerTabs(profile_);

  // URLs passed on the command line supersede all others.
  AppendTabs(cmd_line_tabs, &tabs);
  if (!cmd_line_tabs.empty())
    return tabs;

  // A Master Preferences file provided with this distribution may specify
  // tabs to be displayed on first run, overriding all non-command-line tabs,
  // including the profile reset tab.
  StartupTabs distribution_tabs =
      provider.GetDistributionFirstRunTabs(browser_creator_);
  if (!distribution_tabs.empty())
    return distribution_tabs;

  ... continues with "Policies for onboarding"

[tmartino, 2016/10/06 18:27:28]

This is great! Thanks.

+  // A Master Preferences file provided with this distribution may specify
+  // tabs to be displayed on first run, overriding any other tabs which would
+  // normally be shown. Only command line tabs take priority.
+  if (cmd_line_tabs.empty()) {
+    StartupTabs distribution_tabs =

[Peter Kasting, 2016/10/06 05:49:08]

Nit: Could just use |tabs| here (but not if you take my suggestion above)

+        provider.GetDistributionFirstRunTabs(browser_creator_);
+    if (!distribution_tabs.empty()) {

[Peter Kasting, 2016/10/06 05:49:08]

Nit: No {}

+      return distribution_tabs;
+    }
+  }
+
+  // A trigger on a profile may indicate that we should show a tab which
+  // offers to reset the user's settings. Unlike other policy-based tabs,
+  // this may be shown alongside command line tabs, and always appears first.
+  AppendTabs(provider.GetResetTriggerTabs(profile_), &tabs);
+
+  // URLs passed at the command line supersede onboarding content as well as
+  // user-specified defaults.
+  if (!cmd_line_tabs.empty()) {
+    AppendTabs(cmd_line_tabs, &tabs);
+    return tabs;
+  }
+
+  // Policies for onboarding (e.g., first run) may show promotional and
+  // introductory content depending on a number of system status factors,
+  // including OS and whether or not this is First Run.
+  StartupTabs onboarding_tabs = provider.GetOnboardingTabs();
+  AppendTabs(onboarding_tabs, &tabs);
+
+  // If the user has set the preference indicating URLs to show on opening,
+  // read and add those.
+  StartupTabs prefs_tabs = provider.GetPreferencesTabs();
+  AppendTabs(prefs_tabs, &tabs);
+
+  // Potentially add the New Tab Page. Onboarding content is designed to
+  // replace (and eventually funnel the user to) the NTP. Likewise, URLs read
+  // from preferences are explicitly meant to override showing the NTP.
+  if (onboarding_tabs.empty() && prefs_tabs.empty())
+    tabs.emplace_back(GURL(chrome::kChromeUINewTabURL), false);
+
+  // Add any tabs which the user has previously pinned.
+  AppendTabs(provider.GetPinnedTabs(), &tabs);
+
+  return tabs;
+}
+
 bool StartupBrowserCreatorImpl::ProcessStartupURLs(
     const std::vector<GURL>& urls_to_open) {
   VLOG(1) << "StartupBrowserCreatorImpl::ProcessStartupURLs";
@@ -829,9 +931,6 @@ void StartupBrowserCreatorImpl::AddInfoBarsIfNecessary(
 
 void StartupBrowserCreatorImpl::AddStartupURLs(
     std::vector<GURL>* startup_urls) const {
-  // TODO(atwilson): Simplify the logic that decides which tabs to open on
-  // start-up and make it more consistent. http://crbug.com/248883
-
   // If we have urls specified by the first run master preferences use them
   // and nothing else.
   if (browser_creator_ && startup_urls->empty()) {