Tech Debt Repayment for StartupBrowserCreatorImpl Refactor

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 0ce1f3b775307784c001b237b83e5787ffc812f1..4c15f8d69a1a520566ea740545e6aafac2eda29b 100644
--- a/chrome/browser/ui/startup/startup_browser_creator_impl.h
+++ b/chrome/browser/ui/startup/startup_browser_creator_impl.h
@@ -11,6 +11,7 @@
 #include "base/files/file_path.h"
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
+#include "chrome/browser/sessions/session_restore.h"
 #include "chrome/browser/ui/startup/startup_tab.h"
 #include "chrome/browser/ui/startup/startup_tab_provider.h"
 #include "chrome/browser/ui/startup/startup_types.h"
@@ -73,6 +74,14 @@ class StartupBrowserCreatorImpl {
                            DetermineStartupTabs_CommandLine);
   FRIEND_TEST_ALL_PREFIXES(StartupBrowserCreatorImplTest,
                            DetermineStartupTabs_NewTabPage);
+  FRIEND_TEST_ALL_PREFIXES(StartupBrowserCreatorImplTest,
+                           DetermineBrowserOpenBehavior_Startup);
+  FRIEND_TEST_ALL_PREFIXES(StartupBrowserCreatorImplTest,
+                           DetermineBrowserOpenBehavior_CmdLineTabs);
+  FRIEND_TEST_ALL_PREFIXES(StartupBrowserCreatorImplTest,
+                           DetermineBrowserOpenBehavior_PostCrash);
+  FRIEND_TEST_ALL_PREFIXES(StartupBrowserCreatorImplTest,
+                           DetermineBrowserOpenBehavior_NotStartup);
 
   enum class WelcomeRunType {
     NONE,                // Do not inject the welcome page for this run.
@@ -80,6 +89,7 @@ class StartupBrowserCreatorImpl {
     FIRST_RUN_LAST_TAB,  // Inject the welcome page as the last first-run tab.
   };
 
+  // Window behaviors possible when opening Chrome.
   enum class BrowserOpenBehavior {
     NEW,                  // Open in a new browser.
     SYNCHRONOUS_RESTORE,  // Attempt a synchronous session restore.
@@ -138,7 +148,7 @@ class StartupBrowserCreatorImpl {
   // 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,
+                                  SessionRestore::Behavior restore_options,
                                   bool process_startup,
                                   bool is_post_crash_launch);
 
@@ -154,18 +164,25 @@ class StartupBrowserCreatorImpl {
   // Records Rappor metrics on startup URLs.
   void RecordRapporOnStartupURLs(const std::vector<GURL>& urls_to_open);
 
+  // Boolean flags used to indicate state for DetermineBrowserOpenBehavior.
+  enum BehaviorFlags {

[Peter Kasting, 2017/02/06 23:26:40]

Nit: Technically typedefs and enums belong at the top of the section, per the
styleguide.

+    PROCESS_STARTUP       = (1 << 0),
+    IS_POST_CRASH_LAUNCH  = (1 << 1),
+    HAS_RESTORE_SWITCH    = (1 << 2),
+    HAS_NEW_WINDOW_SWITCH = (1 << 3),
+    HAS_CMD_LINE_TABS     = (1 << 4),
+  };
+
+  typedef uint32_t BrowserOpenBehaviorOptions;

[Peter Kasting, 2017/02/06 23:26:40]

I'm of two minds on this set of flags.

On one hand, it's nice for the unittesting code to have named constants here. 
Makes it more readable.

OTOH, it makes the .cc file strictly uglier.  And it's inconsistent with a lot
of the other startup code, including everything in StartupTabProvider, which
goes the bag-of-bools route.

I'm not sure whether it's a win.  I might end up killing it if it were me.

[tmartino, 2017/02/06 23:52:19]

I originally wrote the tests using the bools approach, so having seen it both
ways, I think it's quite a big win. Frankly, I'm much more confident not just in
the readability of the tests this way, but the correctness of them as well, and
solid unit tests were a big goal in this refactor.

Re: .cc ugliness, WDYT of using kProcessStartup naming instead? We'd still add a
few extra lines compared to the inline bools, but it wouldn't take the
readability hit of having all these BIG_MACRO_STYLE_IDENTIFIERS in the middle of
the function. I'm pretty sure I've seen bitmask flags written both ways, and
because these "act like" a grouping of consts, rather than a list of mutually
exclusive choices, I think it's in the spirit of the style guide.

Re: consistency, I was already mulling over changing StartupTabProvider
functions with 3 or more bools and could add that on here if you think it would
be a win. WDYT?

[Peter Kasting, 2017/02/07 00:11:59]

OK.  I think if you're confident, your instinct should override mine.
For me, it would make no difference either way; it's code like this:

  if (foo_bar)
    options |= FOO_BAR;

...that reads as ugly, and it's no simpler with a different name.
It sounds like my instinct and yours may not match, so... :)

I think the biggest thing that feels bad is when we have the same "concept"
passed to several functions but as a bool in some places and an enum in others. 
It would be nice to represent particular concepts the same way everywhere, but
that might require converting everything (few args or not) and having the "big
union of random concepts" enum, which has its own readability issues.

+
   // Determines how the launch flow should obtain a Browser.
   static BrowserOpenBehavior 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);
+      BrowserOpenBehaviorOptions options);
 
   // Returns the relevant bitmask options which must be passed when restoring a
   // session.
-  static uint32_t DetermineSynchronousRestoreOptions(
+  static SessionRestore::Behavior DetermineSynchronousRestoreOptions(
       bool has_create_browser_default,
       bool has_create_browser_switch,
       bool was_mac_login_or_resume);