Completing refactor of startup_browser_creator_impl

chrome/browser/ui/startup/startup_browser_creator_impl.h

Index: chrome/browser/ui/startup/startup_browser_creator_impl.h
diff --git a/chrome/browser/ui/startup/startup_browser_creator_impl.h b/chrome/browser/ui/startup/startup_browser_creator_impl.h
index 7cc98d360d1c914372b1b10d7b955511c1bb9ae2..978ed4eda0716b0317e7ce05283e774b5a3c034e 100644
--- a/chrome/browser/ui/startup/startup_browser_creator_impl.h
+++ b/chrome/browser/ui/startup/startup_browser_creator_impl.h
@@ -84,6 +84,12 @@ class StartupBrowserCreatorImpl {
     FIRST_RUN_LAST_TAB,  // Inject the welcome page as the last first-run tab.
   };
 
+  enum class BrowserOpenBehavior {
+    NEW,                  // Open in a new browser.
+    SYNCHRONOUS_RESTORE,  // Attempt a synchronous session restore.
+    USE_EXISTING,         // Attempt to add to an existing tabbed browser.
+  };
+
   // Creates a tab for each of the Tabs in |tabs|. If browser is non-null
   // and a tabbed browser, the tabs are added to it. Otherwise a new tabbed
   // browser is created and the tabs are added to it. The browser the tabs
@@ -122,6 +128,40 @@ class StartupBrowserCreatorImpl {
                                    bool is_incognito,
                                    bool is_post_crash_launch);
 
+  // Begins an asynchronous session restore if current state allows it (e.g.,
+  // this is not process startup) and SessionService indicates that one is
+  // necessary. Returns true if restore was initiated, or false if launch
+  // should continue (either synchronously, or asynchronously without
+  // restoring).
+  bool MaybeAsyncRestore(const StartupTabs& tabs,
+                         bool process_startup,
+                         bool is_post_crash_launch);
+
+  // Returns a browser displaying the contents of |tabs|. Based on |behavior|,
+  // this may attempt a session restore or create a new browser. May also allow
+  // DOM Storage to begin cleanup once it's clear it is not needed anymore.
+  Browser* RestoreOrCreateBrowser(const StartupTabs& tabs,
+                                  BrowserOpenBehavior behavior,
+                                  uint32_t restore_options,

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

Nit: Here and in DetermineSynchronousRestoreOptions() below we use a uint32_t to
refer to some nebulous "restore options".  It took me a while to figure out this
is just SessionRestore::Behavior (but expressed as a uint32_t, I guess to
emphasize it could be a bitmask of any of those values.

It would be nice if there was a type alias for this type, defined in
session_restore.h, so that functions could pass that around instead of uint32_t,
and its meaning would be clear.

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

Ack. Made a note to self to look at this when I make a cleanup pass post-launch,
but I'd rather not let /browser/sessions creep into scope right now.

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

SGTM

+                                  bool process_startup,
+                                  bool is_post_crash_launch);
+
+  // Determines how the launch flow should obtain a Browser.
+  static BrowserOpenBehavior DetermineBrowserOpenBehavior(

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

Nit: I suggest placing static methods either above or below non-static methods,
since they're really more like namespaced globals. (2 places)

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

Agreed. Moved them to be the last non-deprecated functions.

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

Minor note: the style guide doesn't dictate where static methods should go in
relation to non-static methods
(https://google.github.io/styleguide/cppguide.html#Declaration_Order). I
interpret this to mean "put them where it makes the most sense given the class
as a whole."

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

Yes; there is no style rule on this.  The majority of Chrome code I've seen
seems to keep them together above or below non-statics, but it's not required.

+      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);
+
+  // Populates and returns the relevant bitmask options which must be passed

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

Nit: No need for "Populates and"

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

Done.

+  // when restoring a session.
+  static uint32_t DetermineSynchronousRestoreOptions(
+      bool has_create_browser_default,
+      bool has_create_browser_switch,
+      bool was_mac_login_or_resume);
+
   // Adds a Tab to |tabs| for each url in |urls| that doesn't already exist
   // in |tabs|.
   void AddUniqueURLs(const std::vector<GURL>& urls, StartupTabs* tabs);