Refactoring startup logic for upcoming FRE changes (non-Win 10).

chrome/browser/first_run/first_run.cc

Index: chrome/browser/first_run/first_run.cc
diff --git a/chrome/browser/first_run/first_run.cc b/chrome/browser/first_run/first_run.cc
index f56eb8c1f3afa9cf986d38fc6511c323a8448c24..ce57dc925bf639f8ac938f06a51d52c5344c3c0a 100644
--- a/chrome/browser/first_run/first_run.cc
+++ b/chrome/browser/first_run/first_run.cc
@@ -25,6 +25,7 @@
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/updater/extension_updater.h"
+#include "chrome/browser/first_run/first_run_features.h"
 #include "chrome/browser/first_run/first_run_internal.h"
 #include "chrome/browser/google/google_brand.h"
 #include "chrome/browser/importer/external_process_importer_host.h"
@@ -74,6 +75,10 @@
 #include "ui/base/l10n/l10n_util.h"
 #include "url/gurl.h"
 
+#if defined(OS_WIN)
+#include "base/win/windows_version.h"
+#endif
+
 namespace content {
 class BrowserContext;
 }
@@ -82,6 +87,11 @@ using base::UserMetricsAction;
 
 namespace {
 
+// Constants: Magic words used by Master Prefs files in place of a URL host
+// to indicate that internal pages should appear on first run.
+constexpr char kNewTabUrlHost[] = "new_tab_page";
+constexpr char kWelcomePageUrlHost[] = "welcome_page";
+
 // A bitfield formed from values in AutoImportState to record the state of
 // AutoImport. This is used in testing to verify import startup actions that
 // occur before an observer can be registered in the test.
@@ -633,6 +643,45 @@ MasterPrefs::MasterPrefs()
 
 MasterPrefs::~MasterPrefs() {}
 
+FirstRunSystemStatus GetFirstRunSystemStatus() {

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

i don't see the benefit of this function and FirstRunSystemStatus. what does it
add?

+  FirstRunSystemStatus status;
+
+#if defined(OS_WIN)
+  status.os_class = base::win::GetVersion() >= base::win::VERSION_WIN10
+                        ? FirstRunSystemStatus::OperatingSystemClass::WINDOWS_10
+                        : FirstRunSystemStatus::OperatingSystemClass::STANDARD;
+#else
+  status.os_class = FirstRunSystemStatus::OperatingSystemClass::STANDARD;
+#endif
+
+  const base::CommandLine* command_line =
+      base::CommandLine::ForCurrentProcess();
+
+  status.is_sentinel_present = internal::IsFirstRunSentinelPresent();
+  status.is_force_flag_on = command_line->HasSwitch(switches::kForceFirstRun);
+  status.is_suppress_flag_on = command_line->HasSwitch(switches::kNoFirstRun);
+
+  return status;
+}
+
+std::vector<GURL> GetOnboardingTabs(FirstRunSystemStatus status) {
+  std::vector<GURL> tabs;
+  if (status.os_class ==
+      FirstRunSystemStatus::OperatingSystemClass::WINDOWS_10) {
+    // TODO(tmartino): Win 10 logic is more complex and will be added in its

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

on win10, the welcome page is shown on first run as well as
first-run-after-os-upgrade. i'm not sure that this is the correct place for the
latter logic.

at a higher level, i think the logic for which URLs are shown when belongs more
with the SBC than here.

+    // own change.
+  } else {

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

else if

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

n/a, removed

+    if (status.is_force_flag_on ||

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

what is the benefit of putting this policy logic here? this is much easier to
understand if it's "if (IsChromeFirstRun())". the fact that a few conditions are
used to decide whether or not this is the case should be of no concern to this
code here. alternatively, pass in first_run as a bool to the function so it can
be unit-tested with the only thing that matters. a separate test should cover
whether or not IsChromeFirstRun returns true under the correct conditions.

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

I'm all right with replacing the struct with a bool that is the result of
IsChromeFirstRun. That change is reflected in the new function
GetStandardOnboardingTabs in StartupTabProvider.

I've also gone ahead and extracted the interesting logic in IsChromeFirstRun
into its own function, DetermineFirstRunState. Having two layers of functions is
necessary because IsChromeFirstRun relies on state (checks cached value of
g_first_run, and makes calls to get switches and files), while
DetermineFirstRunState is static. I've adapted my test cases to verify that new
function's logic.

+        (!status.is_sentinel_present && !status.is_suppress_flag_on)) {
+      tabs.push_back(GetWelcomePageURL());
+    }
+  }
+  return tabs;
+}
+
+// TODO(tmartino): Extend refactor to apply to this logic, removing the
+// internal::g_first_run value and depending on FirstRunSystemStatus, possibly
+// cached.
 bool IsChromeFirstRun() {
   if (internal::g_first_run == internal::FIRST_RUN_UNKNOWN) {
     internal::g_first_run = internal::FIRST_RUN_FALSE;
@@ -714,6 +763,28 @@ bool ShouldShowWelcomePage() {
   return retval;
 }
 
+GURL GetWelcomePageURL() {
+  // TODO(tmartino): Once Consolidated FRE page is implemented, return that
+  // here when kUseConsolidatedFirstRun is true.
+  return GURL(l10n_util::GetStringUTF8(IDS_WELCOME_PAGE_URL));
+}
+
+std::vector<GURL> ProcessMasterPrefsTabs(const std::vector<GURL>& tabs) {
+  std::vector<GURL> processed_tabs;
+  if (IsChromeFirstRun()) {

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

if anything, i think this should be a DCHECK. why should this function ever be
called outside of first-run?

+    for (const GURL& tab : tabs) {
+      if (tab.host() == kNewTabUrlHost) {

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

i think of this magic value-to-tab mapping as being a feature of the SBC rather
than of first run. i suppose it's a bit of a coin-toss since the pref is
"first_run_tabs".

+        processed_tabs.push_back(GURL(chrome::kChromeUINewTabURL));
+      } else if (tab.host() == kWelcomePageUrlHost) {
+        processed_tabs.push_back(GetWelcomePageURL());
+      } else {
+        processed_tabs.push_back(tab);
+      }
+    }
+  }
+  return processed_tabs;
+}
+
 void SetShouldDoPersonalDataManagerFirstRun() {
   g_should_do_autofill_personal_data_manager_first_run = true;
 }