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..2a9f0f0e4a9339764dc170faf3414429fb29ac04 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)) {
+    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,98 @@ 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;
+
+  std::unique_ptr<StartupTabProvider> provider(new StartupTabProviderImpl());

[grt (UTC plus 2), 2016/09/09 11:40:30]

can you put this on the stack here:
  StartupTabProviderImpl provider;
or just put the object in the call below:
    tabs = DetermineStartupTabs(StartupTabProviderImpl(), ...);
the latter is my preference since it keeps the object in the narrowest scope
possible

or will this get more complicated with other provider impls?

[tmartino, 2016/09/10 00:19:41]

My understanding is that this needs to be a pointer if we are to access the
virtual methods in StartupTabProvider. (This is supported by the boatload of
linker errors I get when I try changing it to pass an object.)

[grt (UTC plus 2), 2016/09/12 07:28:46]

Were you passing by value or by constref? The latter is what I meant, and should
work. If not, what were the errors?

+  // If command-line URLs were passed, they supercede all other policies and
+  // logic.
+  StartupTabs tabs;
+  if (cmd_line_urls.empty()) {

[grt (UTC plus 2), 2016/09/09 11:40:29]

nit: i think the code will follow the comment above more naturally if you invert
the logic here: if (!empty) {...} else {...}

+    bool is_incognito = IncognitoModePrefs::ShouldLaunchIncognito(
+        command_line_, profile_->GetPrefs());
+    bool is_crash = HasPendingUncleanExit(profile_);
+    tabs = DetermineStartupTabs(provider.get(), is_incognito, is_crash);
+  } else {
+    UrlsToTabs(cmd_line_urls, &tabs);
+  }
+
+  // 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. Incorporates code from ProcessStartupUrls.
+
+  Browser* browser =
+      RestoreOrCreateBrowser(tabs
+                             /* TODO(tmartino): Also pass behavior here */);
+
+  // Finally, add info bars.
+  chrome::startup::IsProcessStartup is_process_startup =
+      process_startup ? chrome::startup::IS_PROCESS_STARTUP

[grt (UTC plus 2), 2016/09/09 11:40:29]

nit: inline this below?

+                      : chrome::startup::IS_NOT_PROCESS_STARTUP;
+  AddInfoBarsIfNecessary(browser, is_process_startup);
+}
+
+StartupTabs StartupBrowserCreatorImpl::DetermineStartupTabs(
+    StartupTabProvider* provider,

[grt (UTC plus 2), 2016/09/09 11:40:30]

if this doesn't mutate the provider, pass it by constref

[tmartino, 2016/09/10 00:19:41]

See discussion above re: pointers. However, I did change this to const (along
with all the methods in the Provider).

+    bool is_incognito,
+    bool is_crash) {

[grt (UTC plus 2), 2016/09/09 11:40:29]

is_crash -> is_post_crash_launch?

+  StartupTabs tabs;
+
+  // No policy- or profile-related content is shown in incognito mode.
+  // Similarly, when recovering from a crash, nothing should be opened
+  // automatically to prevent getting the user stuck in a crash loop.
+  if (!is_incognito && !is_crash) {
+    StartupTabs master_prefs_tabs =
+        provider->GetMasterPreferencesTabs(browser_creator_);

[grt (UTC plus 2), 2016/09/09 11:40:29]

do you need a nullptr check on browser_creator_ either here or within
GetMasterPreferencesTabs?

[grt (UTC plus 2), 2016/09/09 11:40:29]

i think it should be more clear here that this is the first_run_tabs and that
they're only processed for first_run. how about changing "MasterPreferencesTabs"
with "DistributionFirstRunTabs" since they are the first-run-tabs provided by a
distribution? also, does it make sense to have the if (first run) check up here
rather than hidden in the impl?

[tmartino, 2016/09/10 00:19:41]

I think it does. In my mind, the fact that this only applies on first run is a
policy detail specific to DistributionFirstRun, because the is (first run) check
applies to some policy categories (DistributionFirstRun, Onboarding) but not
others (ResetTrigger, Pinned, Preferences). My hope is to reserve this function
only for:
  * Cases where one set of policies affects another (e.g., DistributionFirstRun
blocks all others)
  * Logic that applies to ALL policies (i.e., incognito/crash blocks everything)

It also moves it down to a static function where it's more easily tested.

+    if (!master_prefs_tabs.empty()) {
+      // Master Preferences content overrides any other policy.
+      tabs.insert(tabs.end(), master_prefs_tabs.begin(),

[grt (UTC plus 2), 2016/09/09 11:40:29]

to avoid the extra churn here, what do you think of changing the method above so
that it takes the container as an in/out param and returns a size_t of the # of
items added (or just a bool if anything was added). then lines 620-627 would
become:
    if (provider->AddDistributionFirstRunTabs(browser_creator_, &tabs)) {
      return tabs;
    }

+                  master_prefs_tabs.end());
+      return tabs;
+    }
+
+    StartupTabs onboarding_tabs = provider->GetOnboardingTabs();

[grt (UTC plus 2), 2016/09/09 11:40:29]

how about all of these being Add...(&tabs);? lots of unneeded heap churn here.

+    if (!onboarding_tabs.empty())
+      tabs.insert(tabs.end(), onboarding_tabs.begin(), onboarding_tabs.end());
+
+    StartupTabs reset_trigger_tabs = provider->GetResetTriggerTabs(profile_);
+    if (!reset_trigger_tabs.empty())
+      tabs.insert(tabs.end(), reset_trigger_tabs.begin(),
+                  reset_trigger_tabs.end());
+
+    StartupTabs prefs_tabs = provider->GetPreferencesTabs();
+    if (!prefs_tabs.empty())
+      tabs.insert(tabs.end(), prefs_tabs.begin(), prefs_tabs.end());
+
+    StartupTabs pinned_tabs = provider->GetPinnedTabs();

[grt (UTC plus 2), 2016/09/09 11:40:29]

can this be Add, too, with it happening before Onboarding tabs so that it
doesn't need to insert at the beginning of the array? otherwise, if it really
does need to happen the end of processing, how about calling it
"PrependPinnedTabs". since inserting into the beginning of an array is so
horrendous, the impl could do something like:
void PrependPinnedTabs(StartupTabs* tabs) {
  StartupTabs pinned_tabs = ...whatever;
  pinned_tabs.insert(pinned_tabs.end(), tabs->begin(), tabs->end());
  tabs->swap(pinned_tabs);
}

+    if (!pinned_tabs.empty())
+      // Pinned tabs always appear as the leftmost tabs; insert at the beginning
+      // for consistency's sake.
+      tabs.insert(tabs.begin(), pinned_tabs.begin(), pinned_tabs.end());
+  }
+
+  // Default to showing New Tab Page when nothing else is cued up.
+  if (tabs.empty())
+    tabs.push_back(StartupTab(GURL(chrome::kChromeUINewTabURL), 0));
+
+  return tabs;
+}
+
+Browser* StartupBrowserCreatorImpl::RestoreOrCreateBrowser(StartupTabs tabs) {
+  // TODO(tmartino): Based on passed behavior flag, possibly restore session
+  // instead of creating a new Browser.
+
+  return OpenTabsInBrowser(nullptr, true, tabs);
+}
+
 bool StartupBrowserCreatorImpl::ProcessStartupURLs(
     const std::vector<GURL>& urls_to_open) {
   VLOG(1) << "StartupBrowserCreatorImpl::ProcessStartupURLs";
@@ -650,6 +742,9 @@ bool StartupBrowserCreatorImpl::ProcessStartupURLs(
 
 Browser* StartupBrowserCreatorImpl::ProcessSpecifiedURLs(
     const std::vector<GURL>& urls_to_open) {
+  // TODO(tmartino): Deprecated, remove this once UseConsolidatedStartupFlow is
+  // enabled.
+
   SessionStartupPref pref =
       StartupBrowserCreator::GetSessionStartupPref(command_line_, profile_);
   StartupTabs tabs;
@@ -829,8 +924,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 +991,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 +1011,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 +1088,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)