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..676819826599a32da5be6285b05adf2dc50cacc4 100644
--- a/chrome/browser/ui/startup/startup_browser_creator_impl.cc
+++ b/chrome/browser/ui/startup/startup_browser_creator_impl.cc
@@ -8,6 +8,7 @@
 #include <stdint.h>
 
 #include <algorithm>
+#include <iterator>
 #include <memory>
 #include <vector>
 
@@ -190,6 +191,14 @@ void UrlsToTabs(const std::vector<GURL>& urls, StartupTabs* tabs) {
   }
 }
 
+std::vector<GURL> TabsToUrls(const StartupTabs& tabs) {
+  std::vector<GURL> urls;
+  urls.reserve(tabs.size());
+  std::transform(tabs.begin(), tabs.end(), std::back_inserter(urls),
+                 [](const StartupTab& tab) { return tab.url; });
+  return urls;
+}
+
 // 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 +277,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)

[Peter Kasting, 2016/10/31 19:24:06]

Nit: Should there be a comment here about why Win 10 does not use the
consolidated flow?

[tmartino, 2016/11/02 16:33:59]

Added TODO.

+    return false;
+#endif  // defined(OS_WIN)
+  return base::FeatureList::IsEnabled(features::kUseConsolidatedStartupFlow);
+}
+
 }  // namespace
 
 namespace internals {
@@ -354,7 +373,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 +615,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 =

[Peter Kasting, 2016/10/31 19:24:06]

Nit: Do you think it would be any clearer to reverse the sense of this bool
(|is_regular_profile|) and pass that around to various places instead?  On one
hand I think "is A or B" is inherently an awkward concept to reason about, on
the other hand "regular profile" is kind of nebulous.

Maybe there's a phrase which describes the common nature of incognito/guest
profiles, e.g. "is_tempory_profile" or "is_ephemeral_profile" or something?

[tmartino, 2016/11/02 16:33:59]

Definitely don't like "regular", but I'm OK with "ephemeral profile".

[grt (UTC plus 2), 2016/11/03 09:43:33]

Note that an "ephemeral profile" is a thing in and of itself
(https://cs.chromium.org/chromium/src/chrome/browser/chromeos/profiles/profile...).
Please only use that term if it truly applies here.

[tmartino, 2016/11/03 18:39:24]

Changing in forthcoming CL.

I see the term "off the record" being thrown around in the code; would that be
an appropriate term for "incognito/guest" or does that, too, mean something
specific?

[Peter Kasting, 2016/11/03 21:46:42]

"off the record" is an archaic term for incognito.  It is confusingly unclear
whether it means guest or not; I would avoid it.

(Is an incognito profile not actually an ephemeral profile, then?  I had a hard
time telling whether it would actually count as one in the linked function,
since it wasn't checked directly.)

[grt (UTC plus 2), 2016/11/04 08:41:31]

I'm not sure. I had a memory of hearing about "ephemeral profiles" a year or
more ago and did a little code search.

+      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.

[Peter Kasting, 2016/10/31 19:24:06]

Nit: Passive voice; maybe "Return immediately if we start an async restore,
since the remainder of that process is self-contained."

[tmartino, 2016/11/02 16:33:59]

Done

+  if (MaybeAsyncRestore(tabs, process_startup, is_post_crash_launch))
+    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(
+      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 +660,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 +701,121 @@ 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_);
+
+  // Service will be null in incognito and guest mode.

[Peter Kasting, 2016/10/31 19:24:05]

Nit: Service -> |service|

Or:

  // Note: there's no session service in incognito or guest mode.
  auto* service =
      SessionServiceFactory::GetForProfileForSessionRestore(profile_);
  return ...;

Or (more verbose):

  // There's no session service in incognito or guest mode.
  auto* service =
      SessionServiceFactory::GetForProfileForSessionRestore(profile_);
  if (!service)
    return false;

  return service->RestoreIfNecessary(TabsToUrls(tabs));

[tmartino, 2016/11/02 16:33:59]

Done (went for the middle option)

+  return (service && service->RestoreIfNecessary(TabsToUrls(tabs)));

[Peter Kasting, 2016/10/31 19:24:05]

Nit: No need for outer ()

[tmartino, 2016/11/02 16:33:59]

Done

+}
+
+Browser* StartupBrowserCreatorImpl::RestoreOrCreateBrowser(
+    const StartupTabs& tabs,
+    BrowserOpenBehavior behavior,
+    uint32_t restore_options,
+    bool process_startup,
+    bool is_post_crash_launch) {
+  Browser* browser = nullptr;
+

[Peter Kasting, 2016/10/31 19:24:06]

Nit: I'd remove this blank, since the subsequent block is still about setting
|browser| correctly.

[tmartino, 2016/11/02 16:34:00]

Done

+  if (behavior == BrowserOpenBehavior::SYNCHRONOUS_RESTORE) {
+    browser = SessionRestore::RestoreSession(profile_, nullptr, restore_options,
+                                             TabsToUrls(tabs));
+    if (browser)
+      return browser;
+  } else if (behavior == BrowserOpenBehavior::USE_EXISTING) {
+    browser = chrome::FindTabbedBrowser(profile_, process_startup);
+  }
+
+  StartupBrowserCreator::in_synchronous_profile_launch_ = true;

[Peter Kasting, 2016/10/31 19:24:05]

Nit: I suggest using base::AutoReset to make clear this is a "set for limited
scope".  This also makes it harder to add an early-return later that jumps out
before we reset this variable.

  {
    base::AutoReset<bool> synchronous_launch_resetter(
        &StartupBrowserCreator::in_synchronous_profile_launch_, true);

    browser = OpenTabsInBrowser(...);
  }

(Also, is it just me, or is it really weird that a class called
"StartupBrowserCreatorImpl" does not inherit from "StartupBrowserCreator", so we
have to qualify the name of this member?)

[tmartino, 2016/11/02 16:33:59]

Done.

And, yeah, I don't care for the structure of these two classes. It feels like
the Impl class isn't so much an implementation as "more functions we moved out
of the class because it was getting unwieldy."

[grt (UTC plus 2), 2016/11/03 09:43:33]

My favorite part of this class's "impl"-ness is the fact that it's used by code
outside of ui/startup:
https://cs.chromium.org/chromium/src/chrome/browser/app_controller_mac.mm?typ....
Barf.

+
+  // OpenTabsInBrowser requires at least one tab be passed. As a fallback to
+  // prevent a crash, use the NTP if |tabs| is empty.

[Peter Kasting, 2016/10/31 19:24:05]

Nit: "As a fallback to prevent a crash" makes it sound like this is some sort of
error case.  Maybe instead of this, say when we'd expect this to be empty?

[tmartino, 2016/11/02 16:33:59]

It really *is* an error case. I had a comment earlier in the thread when grt was
reviewing that explained this further:

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.

[Peter Kasting, 2016/11/02 17:00:46]

Can we add this case to the comment here?  For weird obscure code it's always
good to have a description of the actual triggering flow, so ten years later
someone can retest to see whether this is still needed.

By "error case" I meant "it sounds like you're working around someone else's
bug", but the description you give just sounds like this is an
obscure/unexpected use.  (But maybe you still are saying this is someone else's
bug?  Should we be detecting and handling this differently somewhere else?)

[tmartino, 2016/11/02 17:36:52]

Sorry for CQing before responding--I thought I had sent these responses last
night.

https://codereview.chromium.org/2469363002

+  browser = OpenTabsInBrowser(
+      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
+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) {
+    // 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

[Peter Kasting, 2016/10/31 19:24:06]

Nit: by -> by a?

[tmartino, 2016/11/02 16:33:59]

Done

+    // switch.
+    return (has_cmd_line_tabs && !has_new_window_switch)
+               ? BrowserOpenBehavior::USE_EXISTING
+               : BrowserOpenBehavior::NEW;
+  }
+
+  if (pref.type == SessionStartupPref::LAST) {
+    // 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;
+
+  // 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 the resume feature in OS 10.7+.

[Peter Kasting, 2016/10/31 19:24:06]

Nit: This sentence reads confusingly because the part about what to do comes in
the middle of two different groups of qualifiers about when we do it.

[tmartino, 2016/11/02 16:33:59]

There are a lot of conditions to fit in, but I moved the action ("suppress the
creation...") to the beginning.

+  if (!was_mac_login_or_resume &&
+      (has_create_browser_default || has_create_browser_switch)) {

[Peter Kasting, 2016/10/31 19:24:05]

Nit: {} optional

[tmartino, 2016/11/02 16:33:59]

Done

+    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();