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 54d468b826f90f25d1ea29eb5efde8edd6f1cce8..9e71e9acbbcb1d720d763c4344abfe135a079ced 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"
@@ -39,6 +40,7 @@
 #include "chrome/browser/extensions/launch_util.h"
 #include "chrome/browser/extensions/pack_extension_job.h"
 #include "chrome/browser/first_run/first_run.h"
+#include "chrome/browser/first_run/first_run_features.h"
 #include "chrome/browser/infobars/infobar_service.h"
 #include "chrome/browser/prefs/incognito_mode_prefs.h"
 #include "chrome/browser/prefs/session_startup_pref.h"
@@ -499,6 +501,13 @@ bool StartupBrowserCreatorImpl::OpenApplicationWindow(Profile* profile) {
 void StartupBrowserCreatorImpl::ProcessLaunchURLs(
     bool process_startup,
     const std::vector<GURL>& urls_to_open) {
+  if (base::FeatureList::IsEnabled(features::kUseConsolidatedFirstRun)) {
+    ProcessLaunchURLsUsingConsolidatedFlow(process_startup, urls_to_open);
+    return;
+  }
+  // TODO(tmartino): Remainder of this function is deprecated. Remove when
+  // kUseConsolidatedFirstRun 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;
@@ -506,15 +515,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;
@@ -566,6 +566,70 @@ void StartupBrowserCreatorImpl::ProcessLaunchURLs(
   AddInfoBarsIfNecessary(browser, is_process_startup);
 }
 
+void StartupBrowserCreatorImpl::ProcessLaunchURLsUsingConsolidatedFlow(
+    bool process_startup,
+    const std::vector<GURL>& urls_to_open) {
+  std::vector<GURL> adjusted_urls(urls_to_open);
+
+  // Don't open any browser windows if we're starting up in "background mode".

[grt (UTC plus 2), 2016/08/23 10:36:02]

nit: i'm in the "no personal pronouns" camp
(https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/BYMa_...).
if it makes sense, please see if any comments with "we" and such in them can be
reworded for clarity without the pronoun. for example, this could be:
  // Don't open any browser windows when the process is being started in
background mode.

[tmartino, 2016/09/08 21:33:17]

Done.

+  if (process_startup && command_line_.HasSwitch(switches::kNoStartupWindow))
+    return;
+
+  // If Master Prefs contains any first run tabs, use only those.

[grt (UTC plus 2), 2016/08/23 10:36:02]

in the old logic, URLs on the command-line suppress first_run_tabs.

[tmartino, 2016/09/08 21:33:16]

Good catch; done.

+  std::vector<GURL> tabs_to_insert;
+  if (browser_creator_) {

[grt (UTC plus 2), 2016/08/23 10:36:02]

 && !browser_creator_->first_run_tabs_.empty()

[tmartino, 2016/09/08 21:33:16]

Ack, see thread about ProcessMasterPrefsTabs in first_run.cc.

+    tabs_to_insert =

[grt (UTC plus 2), 2016/08/23 10:36:02]

if you're assuming that first_run_tabs_ will never be non-empty on
non-first-run, please add a DCHECK for this:
    DCHECK(first_run_);

[grt (UTC plus 2), 2016/08/23 10:36:02]

also add:
    DCHECK(process_startup);
to document/assert the expectation that first_run_tabs_ will only be non-empty
during process startup and not during rendezvous

[tmartino, 2016/09/08 21:33:16]

Note that this logic has been moved to startup_tab_provider, but I'll respond
here anyway.

On 2016/08/23 at 10:36:02, grt (UTC plus 2) wrote:
My goal here is to enforce the policy "master preferences only apply on first
run." I'm not assuming that they're empty at other times--they should be, but
I'm not relying on that assumption.

On 2016/08/23 at 10:36:02, grt (UTC plus 2) wrote:
Can you elaborate on that assertion? Is this based on the idea that we clear
their values in this function, and the first call to this function should always
occur during process startup? Or is there some other reason to believe they're
only populated at process startup that would be risky to ignore?

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

when i wrote that, i must have been thinking "how can you have first_run_tabs if
this isn't process startup since you'd have already gone through first run?" i
can imagine it's possible that the first launch of the browser does something
like start background mode or launch an app or something, and that it's only
during rendezvous that the user has what we think of as the "first run" browser
experience. it's a bit odd to think about, but as long as there are ways to
start the browser without bringing the user through the FRE then you're right,
first_run_tabs may be non-empty in the case of rendezvous. this also means we
shouldn't drop the First Run sentinel until we actually bring the user through
the experience. maybe this is already the case, i don't remember.

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

Two things of note:
1. IsChromeFirstRun uses a cached value, so even after the sentinel is created,
IsChromeFirstRun will return true until the process terminates.
2. For Onboarding Tabs, I'm increasingly convinced we don't actually want to
base our logic entirely on this sentinel. We've got a lot of edge cases, for
example: 
  * We recently decided we want to show the welcome page to all new profiles,
not just the first execution on this machine.
  * We want to defer until second run if URLs are passed in via command line.
I think the proper logic might be to add a local state value that represents
"has this profile been shown the welcome page?". The only problem with this
approach is how we avoid showing this to upgrading users. WDYT?

+        first_run::ProcessMasterPrefsTabs(browser_creator_->first_run_tabs_);
+    browser_creator_->first_run_tabs_.clear();
+  }
+  // Otherwise, use the standard Onboarding logic to determine which, if any,
+  // tabs need to be surfaced.
+  if (tabs_to_insert.empty()) {
+    first_run::FirstRunSystemStatus status =
+        first_run::GetFirstRunSystemStatus();
+    tabs_to_insert = first_run::GetOnboardingTabs(status);
+  }
+

[grt (UTC plus 2), 2016/08/23 10:36:02]

is it intentional that the NTP is no longer opened?

[tmartino, 2016/09/08 21:33:16]

No, good catch. Thanks.

+  // If this profile is marked for a reset prompt, put this before all other
+  // tabs.
+  if (ProfileHasResetTrigger()) {
+    tabs_to_insert.insert(tabs_to_insert.begin(),
+                          internals::GetTriggeredResetSettingsURL());
+  }
+
+  // TODO(tmartino): Function for processing pinned tabs.
+
+  // Prepend the resulting list of tabs to those passed in.
+  if (!tabs_to_insert.empty()) {
+    adjusted_urls.insert(adjusted_urls.begin(), tabs_to_insert.begin(),
+                         tabs_to_insert.end());
+  }
+
+  // TODO(tmartino): If this is not process startup, we need to restore
+  // asynchronously and return here.
+
+  // See if we can or should restore an old session synchronously.
+  Browser* browser = MaybeRestoreSession(process_startup, adjusted_urls);

[grt (UTC plus 2), 2016/08/23 10:36:02]

can you break this function into two: one that computes the urls and what to do,
and another that opens them in a Browser? this way, you can exhaustively
unittest the former without having to rely on browser tests. not sure how much
of a gain that would be, but it's worth considering.

+
+  // Otherwise, open.
+  if (!browser) {

[grt (UTC plus 2), 2016/08/23 10:36:02]

nit: omit braces

[tmartino, 2016/09/08 21:33:16]

N/A

+    browser = OpenURLsInBrowser(browser, process_startup, adjusted_urls);
+  }
+
+  // Finally, add info bars.
+  chrome::startup::IsProcessStartup is_process_startup =
+      process_startup ? chrome::startup::IS_PROCESS_STARTUP
+                      : chrome::startup::IS_NOT_PROCESS_STARTUP;
+  AddInfoBarsIfNecessary(browser, is_process_startup);
+}
+
+Browser* StartupBrowserCreatorImpl::MaybeRestoreSession(
+    bool process_startup,
+    const std::vector<GURL>& urls) {
+  // TODO(tmartino): Move session restore logic into this function.
+  return NULL;

[grt (UTC plus 2), 2016/08/23 10:36:02]

nit: nullptr

[tmartino, 2016/09/08 21:33:17]

Done (throughout)

+}
+
 bool StartupBrowserCreatorImpl::ProcessStartupURLs(
     const std::vector<GURL>& urls_to_open) {
   VLOG(1) << "StartupBrowserCreatorImpl::ProcessStartupURLs";
@@ -648,6 +712,9 @@ bool StartupBrowserCreatorImpl::ProcessStartupURLs(
 
 Browser* StartupBrowserCreatorImpl::ProcessSpecifiedURLs(
     const std::vector<GURL>& urls_to_open) {
+  // TODO(tmartino): Deprecated, remove this once UseConsolidatedFirstRun is
+  // enabled.
+
   SessionStartupPref pref =
       StartupBrowserCreator::GetSessionStartupPref(command_line_, profile_);
   StartupTabs tabs;
@@ -826,8 +893,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 UseConsolidatedFirstRun is
+  // enabled.
 
   // If we have urls specified by the first run master preferences use them
   // and nothing else.
@@ -893,6 +960,9 @@ void StartupBrowserCreatorImpl::AddStartupURLs(
 
 void StartupBrowserCreatorImpl::AddSpecialURLs(
     std::vector<GURL>* url_list) const {
+  // TODO(tmartino): Deprecated, remove this once UseConsolidatedFirstRun is
+  // enabled.
+
   // Optionally include the welcome page.
   if (welcome_run_type_ == WelcomeRunType::FIRST_TAB)
     url_list->insert(url_list->begin(), internals::GetWelcomePageURL());
@@ -910,6 +980,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 UseConsolidatedFirstRun is
+// enabled.
 void StartupBrowserCreatorImpl::InitializeWelcomeRunType(
     const std::vector<GURL>& urls_to_open) {
   DCHECK_EQ(static_cast<int>(WelcomeRunType::NONE),