Completing refactor of startup_browser_creator_impl

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 3a3aea13a33b8073d22daff879e45a471dee66d8..7249b055d5c615ce75567f346e459cc4df2c3e94 100644
--- a/chrome/browser/ui/startup/startup_browser_creator_impl.cc
+++ b/chrome/browser/ui/startup/startup_browser_creator_impl.cc
@@ -190,6 +190,12 @@ void UrlsToTabs(const std::vector<GURL>& urls, StartupTabs* tabs) {
   }
 }
 
+void TabsToUrls(const StartupTabs& tabs, std::vector<GURL>* urls) {
+  urls->resize(tabs.size());
+  std::transform(tabs.begin(), tabs.end(), urls->begin(),
+                 [](StartupTab tab) { return tab.url; });
+}
+
 // Return true if the command line option --app-id is used.  Set
 // |out_extension| to the app to open, and |out_launch_container|
 // to the type of window into which the app should be open.
@@ -268,6 +274,16 @@ void AppendTabs(const StartupTabs& from, StartupTabs* to) {
     to->insert(to->end(), from.begin(), from.end());
 }
 
+// Determines whether the Consolidated startup flow should be used, based on
+// OS, OS version, and the kUseConsolidatedStartupFlow Feature.
+bool UseConsolidatedFlow() {
+#if defined(OS_WIN)
+  if (base::win::GetVersion() >= base::win::VERSION_WIN10)
+    return false;
+#endif  // defined(OS_WIN)
+  return base::FeatureList::IsEnabled(features::kUseConsolidatedStartupFlow);
+}
+
 }  // namespace
 
 namespace internals {
@@ -354,7 +370,7 @@ bool StartupBrowserCreatorImpl::Launch(Profile* profile,
     RecordLaunchModeHistogram(urls_to_open.empty() ?
                               LM_TO_BE_DECIDED : LM_WITH_URLS);
 
-    if (base::FeatureList::IsEnabled(features::kUseConsolidatedStartupFlow))
+    if (UseConsolidatedFlow())
       ProcessLaunchUrlsUsingConsolidatedFlow(process_startup, urls_to_open);
     else
       ProcessLaunchURLs(process_startup, urls_to_open);
@@ -596,24 +612,41 @@ void StartupBrowserCreatorImpl::ProcessLaunchUrlsUsingConsolidatedFlow(
   StartupTabs cmd_line_tabs;
   UrlsToTabs(cmd_line_urls, &cmd_line_tabs);
 
-  bool is_incognito = IncognitoModePrefs::ShouldLaunchIncognito(
-      command_line_, profile_->GetPrefs());
+  bool is_incognito_or_guest =
+      profile_->GetProfileType() != Profile::ProfileType::REGULAR_PROFILE;
   bool is_post_crash_launch = HasPendingUncleanExit(profile_);
   StartupTabs tabs =
       DetermineStartupTabs(StartupTabProviderImpl(), cmd_line_tabs,
-                           is_incognito, is_post_crash_launch);
+                           is_incognito_or_guest, is_post_crash_launch);
+
+  // If an async restore is necessary and is successfully kicked off, the
+  // remainder of the process is self-contained, so return.
+  if (MaybeAsyncRestore(tabs, process_startup, is_post_crash_launch))

[grt (UTC plus 2), 2016/10/28 09:22:12]

nit: i've had other reviewers ask me not to use "Maybe" since it's a bit
wishy-washy. wdyt of "BeginAsyncRestore" and document it as returning false if
nothing was initiated? something like:

// Begins an asynchronous session restore if blahdeblah. Returns true if the
// operation was initiated, or false if a synchronous launch should continue.

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

Hmm, I've tried three different names (Maybe..., Begin..., and Attempt...) and
read through the code again. The problem is that IMO the latter two imply that
Async restore is the "normal" case and that it *should* usually happen. When I
read if(Begin...) or if(Attempt...) it comes across as a success/failure check,
and this is more of a "does this apply?" check.

I did expand the comments per your suggestions, but feel free to push back on
the name if you feel strongly.

+    return;
 
-  // 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.
+  BrowserOpenBehavior behavior = DetermineBrowserOpenBehavior(

[tmartino, 2016/10/27 23:00:43]

One example of weird logic structure questions: do you think this would be
cleaner with an extra layer of lasagna code that grabs the bits of system state
and passes them on to the static function? Same goes for the
DetermineSynchronousRestoreOptions call below.

+      StartupBrowserCreator::GetSessionStartupPref(command_line_, profile_),
+      process_startup, is_post_crash_launch,
+      command_line_.HasSwitch(switches::kRestoreLastSession),
+      command_line_.HasSwitch(switches::kOpenInNewWindow),
+      !cmd_line_tabs.empty());
 
-  // 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.
+  uint32_t restore_options = 0;
+  if (behavior == BrowserOpenBehavior::SYNCHRONOUS_RESTORE) {
+#if defined(OS_MACOSX)
+    bool was_mac_login_or_resume = base::mac::WasLaunchedAsLoginOrResumeItem();
+#else
+    bool was_mac_login_or_resume = false;
+#endif
+    restore_options = DetermineSynchronousRestoreOptions(
+        browser_defaults::kAlwaysCreateTabbedBrowserOnSessionRestore,
+        base::CommandLine::ForCurrentProcess()->HasSwitch(
+            switches::kCreateBrowserOnStartupForTests),
+        was_mac_login_or_resume);
+  }
 
-  Browser* browser = OpenTabsInBrowser(nullptr, process_startup, tabs);
+  Browser* browser = RestoreOrCreateBrowser(
+      tabs, behavior, restore_options, process_startup, is_post_crash_launch);
 
   // Finally, add info bars.
   AddInfoBarsIfNecessary(
@@ -624,12 +657,12 @@ void StartupBrowserCreatorImpl::ProcessLaunchUrlsUsingConsolidatedFlow(
 StartupTabs StartupBrowserCreatorImpl::DetermineStartupTabs(
     const StartupTabProvider& provider,
     const StartupTabs& cmd_line_tabs,
-    bool is_incognito,
+    bool is_incognito_or_guest,
     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 (is_incognito_or_guest || is_post_crash_launch) {
     if (cmd_line_tabs.empty())
       return StartupTabs({StartupTab(GURL(chrome::kChromeUINewTabURL), false)});
     return cmd_line_tabs;
@@ -665,17 +698,125 @@ StartupTabs StartupBrowserCreatorImpl::DetermineStartupTabs(
   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
+  // replace (and eventually funnel the user to) the NTP. Likewise, URLs
   // from preferences are explicitly meant to override showing the NTP.
   if (onboarding_tabs.empty() && prefs_tabs.empty())
-    tabs.emplace_back(GURL(chrome::kChromeUINewTabURL), false);
+    AppendTabs(provider.GetNewTabPageTabs(command_line_, profile_), &tabs);
 
-  // Add any tabs which the user has previously pinned.
-  AppendTabs(provider.GetPinnedTabs(profile_), &tabs);
+  // Maybe add any tabs which the user has previously pinned.
+  AppendTabs(provider.GetPinnedTabs(command_line_, profile_), &tabs);
 
   return tabs;
 }
 
+bool StartupBrowserCreatorImpl::MaybeAsyncRestore(const StartupTabs& tabs,
+                                                  bool process_startup,
+                                                  bool is_post_crash_launch) {
+  // Restore is performed synchronously on startup, and is never performed when
+  // launching after crashing.
+  if (process_startup || is_post_crash_launch)
+    return false;
+
+  SessionService* service =
+      SessionServiceFactory::GetForProfileForSessionRestore(profile_);
+  std::vector<GURL> urls;

[grt (UTC plus 2), 2016/10/28 09:22:12]

i think this looks nicer as:
  return (service && service->RestoreIfNecessary(TabsToUrls(tabs)));
below, too

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

Agreed, done.

+  TabsToUrls(tabs, &urls);
+  // Service will be null in incognito and guest mode.
+  return (service && service->RestoreIfNecessary(urls));
+}
+
+Browser* StartupBrowserCreatorImpl::RestoreOrCreateBrowser(
+    const StartupTabs& tabs,
+    BrowserOpenBehavior behavior,
+    uint32_t restore_options,
+    bool process_startup,
+    bool is_post_crash_launch) {
+  Browser* browser = nullptr;
+
+  if (behavior == BrowserOpenBehavior::SYNCHRONOUS_RESTORE) {
+    std::vector<GURL> urls;
+    TabsToUrls(tabs, &urls);
+    browser = SessionRestore::RestoreSession(profile_, nullptr, restore_options,
+                                             urls);
+    if (browser)
+      return browser;
+  } else if (behavior == BrowserOpenBehavior::USE_EXISTING) {
+    browser = chrome::FindTabbedBrowser(profile_, process_startup);
+  }
+
+  StartupBrowserCreator::in_synchronous_profile_launch_ = true;
+
+  // OpenTabsInBrowser requires at least one tab be passed. As a fallback to
+  // prevent a crash, use the NTP if |tabs| is empty.
+  browser = OpenTabsInBrowser(

[tmartino, 2016/10/27 23:00:43]

This seems gross and unnecessary; however, I was able to trigger this crash at
least once, so I think it's a worthwhile fallback.

The scenario where it crashed was obscure: using the --restore-last-session flag
on a profile without the restore pref set, while Chrome process was already
running. We view this as a restore desired (the pref comes back as Type::LAST)
but for some reason downstream, the restore doesn't happen.

Since that flag is only meant to work at startup per its documentation, I'm not
too worried that there's a substantial flaw in our logic here. But I do think
it's better to launch just the NTP than to crash in this situation (and any
other session restore mismatches that might pop up). That also preserves
existing behavior--I'm not sure what sequence of steps is invoked in the
existing logic, but the end result is that the NTP is displayed instead of a
session restore.

[grt (UTC plus 2), 2016/10/28 09:22:12]

The doc comment on kRestoreLastSession says it's "primarily used for testing",
which doesn't seem to be the case. Since it's a fundamental part of the product
(it's used for restarts driven by the OS on Windows and macOS), would you remove
that part of the comment? Also, the part of the comment about the switch's value
seems out of date. A simple codesearch doesn't reveal any code that sets or gets
a value from it.

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

Done, and thank you for helping to keep docs up-to-date, even in places not
directly touched by the current CL. This is not a common enough habit.

+      browser, process_startup,
+      (tabs.empty()
+           ? StartupTabs({StartupTab(GURL(chrome::kChromeUINewTabURL), false)})
+           : tabs));
+
+  StartupBrowserCreator::in_synchronous_profile_launch_ = false;
+
+  // Now that a restore is no longer possible, it is safe to clear DOM storage,
+  // unless this is a crash recovery.
+  if (!is_post_crash_launch) {
+    content::BrowserContext::GetDefaultStoragePartition(profile_)
+        ->GetDOMStorageContext()
+        ->StartScavengingUnusedSessionStorage();
+  }
+
+  return browser;
+}
+
+// static

[tmartino, 2016/10/27 23:00:43]

Will add unit tests for static code to this CL later in review process. Wanted
you to get a look at the structure before writing tests.

+StartupBrowserCreatorImpl::BrowserOpenBehavior
+StartupBrowserCreatorImpl::DetermineBrowserOpenBehavior(
+    const SessionStartupPref& pref,
+    bool process_startup,
+    bool is_post_crash_launch,
+    bool has_restore_switch,
+    bool has_new_window_switch,
+    bool has_cmd_line_tabs) {
+  if (!process_startup) {

[grt (UTC plus 2), 2016/10/28 09:22:12]

i forget: is |process_startup| true or false for the case where the browser
process is already running in background mode? i think that case should be
handled as if the process were being started for the first time.

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

I haven't been able to completely trace that particular use case. This is passed
all the way down from StartupBrowserCreator::LaunchBrowser, specifically the
value of in_synchronous_profile_launch_, which in turn is determined by what
value is passed as |process_startup| to ProcessCmdLineImpl; it diverges from
there.

The existing code doesn't do anything special to check for and handle the
background case--I suspect the idea was to trust the caller to figure out
whether this should "look like" startup or not.

+    // For existing processes, restore would have happened before invoking this
+    // function. If Chrome was launched with passed URLs, assume these should
+    // be appended to an existing window if possible, unless overridden by
+    // switch.
+    return (has_cmd_line_tabs && !has_new_window_switch)
+               ? BrowserOpenBehavior::USE_EXISTING
+               : BrowserOpenBehavior::NEW;
+  }
+
+  if (pref.type == SessionStartupPref::Type::LAST) {

[grt (UTC plus 2), 2016/10/28 09:22:12]

nit: omit "::Type" since it's a regular enum rather than an enum class.

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

Done

+    // Don't perform a session restore on a post-crash launch, as this could
+    // cause a crash loop. These checks can be overridden by a switch.
+    // TODO(crbug.com/647851): Group this check with the logic in
+    // GetSessionStartupPref.
+    if (!is_post_crash_launch || has_restore_switch)
+      return BrowserOpenBehavior::SYNCHRONOUS_RESTORE;
+  }
+
+  return BrowserOpenBehavior::NEW;
+}
+
+// static
+uint32_t StartupBrowserCreatorImpl::DetermineSynchronousRestoreOptions(
+    bool has_create_browser_default,
+    bool has_create_browser_switch,
+    bool was_mac_login_or_resume) {
+  uint32_t options = SessionRestore::SYNCHRONOUS;
+
+  if (has_create_browser_default || has_create_browser_switch)

[grt (UTC plus 2), 2016/10/28 09:22:12]

nit: rather than setting this bit and then clearing it below for the mac case,
how about "if (!was_mac_login_or_resume && (foo || bar))" here (with the
mac-specific comment just above)?

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

Done

+    options |= SessionRestore::ALWAYS_CREATE_TABBED_BROWSER;
+
+  // On Mac, when restoring a session with no windows, suppress the creation
+  // of a new window in the case where the system is launching Chrome via a
+  // login item or Lion's resume feature.

[grt (UTC plus 2), 2016/10/28 09:22:12]

nit: while everyone new what "Lion" meant when it was the new hot and shiny, i
think it would be better for this to say "...or the OS's resume feature
introduced in OS X 10.7."

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

Done

+  if (was_mac_login_or_resume)
+    options = options & ~SessionRestore::ALWAYS_CREATE_TABBED_BROWSER;
+
+  return options;
+}
+
 void StartupBrowserCreatorImpl::AddUniqueURLs(const std::vector<GURL>& urls,
                                               StartupTabs* tabs) {
   size_t num_existing_tabs = tabs->size();