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

chrome/browser/ui/startup/startup_browser_creator_impl.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_impl.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
 #include <memory>
 #include <vector>
 
 #include "apps/app_restore_service.h"
 #include "apps/app_restore_service_factory.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #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"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_restrictions.h"
 #include "build/build_config.h"
@@ skipping 32 matching lines @@
 #include "chrome/browser/ui/chrome_pages.h"
 #include "chrome/browser/ui/extensions/app_launch_params.h"
 #include "chrome/browser/ui/extensions/application_launch.h"
 #include "chrome/browser/ui/session_crashed_bubble.h"
 #include "chrome/browser/ui/startup/bad_flags_prompt.h"
 #include "chrome/browser/ui/startup/default_browser_prompt.h"
 #include "chrome/browser/ui/startup/google_api_keys_infobar_delegate.h"
 #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"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_result_codes.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "chrome/common/extensions/extension_metrics.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
@@ skipping 411 matching lines @@
       WebContents* app_tab = ::OpenAppShortcutWindow(profile, url);
       return (app_tab != NULL);
     }
   }
   return false;
 }
 
 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;
 
   // 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;
   }
 
   chrome::startup::IsProcessStartup is_process_startup = process_startup ?
       chrome::startup::IS_PROCESS_STARTUP :
       chrome::startup::IS_NOT_PROCESS_STARTUP;
   if (!process_startup) {
     // Even if we're not starting a new process, this may conceptually be
@@ skipping 31 matching lines @@
     // Always open a list of urls in a window on the native desktop.
     browser = chrome::FindTabbedBrowser(profile_, false);
   }
   // This will launch a browser; prevent session restore.
   StartupBrowserCreator::in_synchronous_profile_launch_ = true;
   browser = OpenURLsInBrowser(browser, process_startup, adjusted_urls);
   StartupBrowserCreator::in_synchronous_profile_launch_ = false;
   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";
   SessionStartupPref pref =
       StartupBrowserCreator::GetSessionStartupPref(command_line_, profile_);
   if (pref.type == SessionStartupPref::LAST)
     VLOG(1) << "Pref: last";
   else if (pref.type == SessionStartupPref::URLS)
     VLOG(1) << "Pref: urls";
   else if (pref.type == SessionStartupPref::DEFAULT)
@@ skipping 62 matching lines @@
       !HasPendingUncleanExit(profile_)) {
     content::BrowserContext::GetDefaultStoragePartition(profile_)->
         GetDOMStorageContext()->StartScavengingUnusedSessionStorage();
   }
 
   return true;
 }
 
 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;
   // Pinned tabs should not be displayed when chrome is launched in incognito
   // mode. Also, no pages should be opened automatically if the session
   // crashed. Otherwise it might trigger another crash, locking the user out of
   // chrome. The crash infobar is shown in this case.
   if (!IncognitoModePrefs::ShouldLaunchIncognito(command_line_,
                                                  profile_->GetPrefs()) &&
       !HasPendingUncleanExit(profile_)) {
@@ skipping 159 matching lines @@
            browser_creator_->is_default_browser_dialog_suppressed())) {
         chrome::ShowDefaultBrowserPrompt(profile_);
       }
     }
 #endif
   }
 }
 
 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.
   if (browser_creator_ && startup_urls->empty()) {
     if (!browser_creator_->first_run_tabs_.empty()) {
       std::vector<GURL>::iterator it =
           browser_creator_->first_run_tabs_.begin();
       while (it != browser_creator_->first_run_tabs_.end()) {
         // Replace magic names for the actual urls.
         if (it->host() == "new_tab_page") {
@@ skipping 45 matching lines @@
           startup_urls->at(0) == GURL(chrome::kChromeUINewTabURL))
         startup_urls->at(0) = sync_promo_url;
       else
         startup_urls->insert(startup_urls->begin(), sync_promo_url);
     }
   }
 }
 
 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());
 
   // If this Profile is marked for a reset prompt, ensure the reset
   // settings dialog appears.
   if (ProfileHasResetTrigger()) {
     url_list->insert(url_list->begin(),
                      internals::GetTriggeredResetSettingsURL());
   }
 }
 
 // For first-run, the type will be FIRST_RUN_LAST for all systems except for
 // Windows 10+, where it will be FIRST_RUN_FIRST. For non-first run, the type
 // 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),
             static_cast<int>(welcome_run_type_));
 #if defined(OS_WIN)
   // Do not welcome if there are any URLs to open.
   if (!urls_to_open.empty())
     return;
 
   base::win::OSInfo* const os_info = base::win::OSInfo::GetInstance();
@@ skipping 54 matching lines @@
 }
 
 void StartupBrowserCreatorImpl::RecordRapporOnStartupURLs(
     const std::vector<GURL>& urls_to_open) {
   for (const GURL& url : urls_to_open) {
     rappor::SampleDomainAndRegistryFromGURL(g_browser_process->rappor_service(),
                                             "Startup.BrowserLaunchURL", url);
   }
 }
 
+// TODO(tmartino): Deprecated, remove this once UseConsolidatedStartupFlow is
+// enabled.
 bool StartupBrowserCreatorImpl::ProfileHasResetTrigger() const {
   bool has_reset_trigger = false;
 #if defined(OS_WIN)
   TriggeredProfileResetter* triggered_profile_resetter =
       TriggeredProfileResetterFactory::GetForBrowserContext(profile_);
   // TriggeredProfileResetter instance will be nullptr for incognito profiles.
   if (triggered_profile_resetter) {
     has_reset_trigger = triggered_profile_resetter->HasResetTrigger();
   }
 #endif  // defined(OS_WIN)
   return has_reset_trigger;
 }