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 0e66bf556c084f51720c1550641addcb1cd8e5de..bd6c77b64e5dcee969bd1ca1f08a3c3043543cf8 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"
@@ -501,6 +503,13 @@ bool StartupBrowserCreatorImpl::OpenApplicationWindow(Profile* profile) {
 void StartupBrowserCreatorImpl::ProcessLaunchURLs(
     bool process_startup,
     const std::vector<GURL>& urls_to_open) {
+  if (base::FeatureList::IsEnabled(features::kUseConsolidatedStartupFlow)) {

[Peter Kasting, 2016/09/29 07:23:34]

Nit: I think it would be slightly clearer to move this condition to the caller
of this method instead of implementing one function in terms of the other.

This way you can mark this method as fully deprecated like the others below (see
comments there).

+    ProcessLaunchUrlsUsingConsolidatedFlow(process_startup, urls_to_open);
+    return;
+  }
+  // TODO(tmartino): Remainder of this function is deprecated. Remove when
+  // kUseConsolidatedStartupFlow is on by default.
+
   // Don't open any browser windows if we're starting up in "background mode".
   if (process_startup && command_line_.HasSwitch(switches::kNoStartupWindow))
     return;
@@ -508,15 +517,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 +568,106 @@ 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_crash = HasPendingUncleanExit(profile_);

[Peter Kasting, 2016/09/29 07:23:34]

Nit: |is_post_crash_launch| would be more descriptive and match the naming you
use below.

+  StartupTabs tabs = DetermineStartupTabs(
+      StartupTabProviderImpl(), cmd_line_tabs, is_incognito, is_crash);
+
+  // 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

[Peter Kasting, 2016/09/29 07:23:34]

Nit: Function which determines -> Determine? (Not really sure if there was some
reason you were calling out the "function" bit explicitly.)

[tmartino, 2016/09/30 20:55:01]

The reason was simply to give a better idea of the intended future structure:
the plan is to separate the "determine behavior" logic into its own function
that shouldn't interact directly with any state (and should therefore be
unit-testable). It will mirror what's been done with the tab logic.

+  // restore, if any, is necessary. Incorporates code from ProcessStartupUrls.
+
+  Browser* browser =
+      RestoreOrCreateBrowser(process_startup, tabs
+                             /* TODO(tmartino): Also pass behavior here */);
+
+  // 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) {
+  // 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())
+      return StartupTabs({StartupTab(GURL(chrome::kChromeUINewTabURL), false)});

[Peter Kasting, 2016/09/29 07:23:33]

Nit: Is there any way to avoid having both ( and { here?  I'm not opposed to
constructing things this way, I just don't usually use this syntax, so I don't
know if either one of them could be omitted.

Another way involves moving the |startup_tabs| declaration up:

  StartupTabs startup_tabs;
  if (is_incognito || is_post_crash_launch) {
    if (cmd_line_tabs.empty())
      startup_tabs.emplace_back(GURL(chrome::kChromeUINewTabURL), false);
    return cmd_line_tabs;
  }

I kinda prefer this honestly but up to you.

[tmartino, 2016/09/30 20:55:01]

I've tried this a few ways (this way was actually Greg's suggestion) and at the
end of the day I found this to most clearly/directly express what's going on
here. Every other method (e.g., vector<Foo> foolist = {foo1};) involves doing
some local storage and manipulation, when what we're really trying to do is say
"just bomb out and return this thing that's basically a constant". It's a little
unaesthetic, but I think it makes the most sense to do it inline.

[Peter Kasting, 2016/09/30 21:50:03]

Right, I'm mostly looking for different syntax for this.  The reason I liked the
"call emplace_back and then return" route, in addition to paralleling things
below, was simply that this line is very noisy to read; it has the phrase
StartupTab twice, there are tons of ({(())}).  I figured breaking into two
lines, especially when you had to return cmd_line_tabs anyway and thus you
wouldn't cost any net LOC, was overall more readable.  Semantically, I'm not
sure "push this constant and return" is fundamentally different than "return
this constant"; they read the same to me.

You can override me on this.

[tmartino, 2016/10/03 23:26:06]

OK, I went with moving the |tabs| declaration up.

+    else

[Peter Kasting, 2016/09/29 07:23:34]

Nit: No else after return

+      return cmd_line_tabs;
+  }
+
+  StartupTabs tabs;
+
+  // 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() &&
+      provider.AddDistributionFirstRunTabs(browser_creator_, &tabs))
+    return 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.
+  provider.AddResetTriggerTabs(profile_, &tabs);
+
+  // URLs passed at the command line supersede onboarding content as well as
+  // user-specified defaults.
+  if (!cmd_line_tabs.empty()) {
+    tabs.insert(tabs.end(), cmd_line_tabs.begin(), cmd_line_tabs.end());
+    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.
+  bool onboarding_added = provider.AddOnboardingTabs(&tabs);
+
+  // If the user has set the preference indicating URLs to show on opening,
+  // read and add those.
+  bool prefs_added = provider.AddPreferencesTabs(&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_added && !prefs_added)
+    tabs.push_back(StartupTab(GURL(chrome::kChromeUINewTabURL), false));

[Peter Kasting, 2016/09/29 07:23:34]

Nit: Up to you, but I tend to prefer emplace_back() here just to omit the type
name that is not really important.

It would be nice to figure out a way to rearrange the code so we weren't doing
this both here and at the beginning of this function, but I don't know whether
doing so is very feasible.  Possibly if we broke this function into more
helpers?  Don't worry too hard about this.

[tmartino, 2016/09/30 20:55:01]

Changed to emplace_back here and throughout startup_tab_provider.cc as well.

On the latter point, grt and I have put a lot of thought and iterations into
this and this is really the best we've come up with--I discussed this
extensively in message #34, if you're interested in the rationale. My goal with
this method was to express, as directly as possible, the relationships between
various policies, and in this case there are really two different policies at
play:

#1. In incognito/crash, exit early and only show the "constant" list of just the
NTP.
#2. The NTP should be added to the list of tabs depending on the results of
certain other policies.

I found it best to express those two separately, rather than trying to somehow
cram them together in a more general way when they aren't really saying the same
thing despite sharing a line of code.

+
+  // Reads and adds any tabs which the user has previously pinned.

[Peter Kasting, 2016/09/29 07:23:34]

Nit: Reads and adds -> Add? (For parallel structure)

+  provider.AddPinnedTabs(&tabs);
+
+  return tabs;
+}
+
+Browser* StartupBrowserCreatorImpl::RestoreOrCreateBrowser(
+    bool process_startup,
+    const StartupTabs& tabs) {
+  // TODO(tmartino): Based on passed behavior flag, possibly restore session
+  // instead of creating a new Browser.
+
+  return OpenTabsInBrowser(nullptr, process_startup, tabs);
+}
+
 bool StartupBrowserCreatorImpl::ProcessStartupURLs(
     const std::vector<GURL>& urls_to_open) {
   VLOG(1) << "StartupBrowserCreatorImpl::ProcessStartupURLs";
@@ -650,6 +750,9 @@ bool StartupBrowserCreatorImpl::ProcessStartupURLs(
 
 Browser* StartupBrowserCreatorImpl::ProcessSpecifiedURLs(
     const std::vector<GURL>& urls_to_open) {
+  // TODO(tmartino): Deprecated, remove this once UseConsolidatedStartupFlow is
+  // enabled.

[Peter Kasting, 2016/09/29 07:23:33]

Nit: Some of these comments you put inside the function, some above.  Be
consistent.  But perhaps even better, don't comment here, comment in the header
instead, placing all the deprecated functions together in one big block with one
comment that says they're all deprecated.

+
   SessionStartupPref pref =
       StartupBrowserCreator::GetSessionStartupPref(command_line_, profile_);
   StartupTabs tabs;
@@ -829,8 +932,8 @@ 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
+  // TODO(tmartino): Deprecated, remove this once UseConsolidatedStartupFlow is
+  // enabled.
 
   // If we have urls specified by the first run master preferences use them
   // and nothing else.
@@ -896,6 +999,9 @@ void StartupBrowserCreatorImpl::AddStartupURLs(
 
 void StartupBrowserCreatorImpl::AddSpecialURLs(
     std::vector<GURL>* url_list) const {
+  // TODO(tmartino): Deprecated, remove this once UseConsolidatedStartupFlow is
+  // enabled.
+
   // Optionally include the welcome page.
   if (welcome_run_type_ == WelcomeRunType::FIRST_TAB)
     url_list->insert(url_list->begin(), internals::GetWelcomePageURL());
@@ -913,6 +1019,9 @@ void StartupBrowserCreatorImpl::AddSpecialURLs(
 // will be NONE for all systems except for Windows 10+, where it will be
 // ANY_RUN_FIRST if this is the first somewhat normal launch since an OS
 // upgrade.
+
+// TODO(tmartino): Deprecated, remove this once UseConsolidatedStartupFlow is
+// enabled.
 void StartupBrowserCreatorImpl::InitializeWelcomeRunType(
     const std::vector<GURL>& urls_to_open) {
   DCHECK_EQ(static_cast<int>(WelcomeRunType::NONE),
@@ -987,6 +1096,8 @@ void StartupBrowserCreatorImpl::RecordRapporOnStartupURLs(
   }
 }
 
+// TODO(tmartino): Deprecated, remove this once UseConsolidatedStartupFlow is
+// enabled.
 bool StartupBrowserCreatorImpl::ProfileHasResetTrigger() const {
   bool has_reset_trigger = false;
 #if defined(OS_WIN)