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

chrome/browser/ui/startup/startup_tab_provider.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/startup/startup_tab_provider.h"
+
+#include "chrome/browser/first_run/first_run.h"
+#include "chrome/browser/ui/chrome_pages.h"
+#include "chrome/common/url_constants.h"
+#include "chrome/grit/locale_settings.h"
+#include "ui/base/l10n/l10n_util.h"
+
+#if defined(OS_WIN)

[grt (UTC plus 2), 2016/09/12 07:28:46]

#include "build/build_config.h" to define OS_WIN

+#include "base/win/windows_version.h"
+#include "chrome/browser/profile_resetter/triggered_profile_resetter.h"
+#include "chrome/browser/profile_resetter/triggered_profile_resetter_factory.h"
+#endif
+
+namespace {
+// Constants: Magic words used by Master Preferences 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";
+}  // namespace
+
+bool StartupTabProviderImpl::AddOnboardingTabs(StartupTabs* tabs) const {
+#if defined(OS_WIN)
+  // Windows 10 has unique onboarding policies and content.
+  if (base::win::GetVersion() >= base::win::VERSION_WIN10) {
+    // TODO(tmartino): * Add a function, GetWin10SystemState, which gathers
+    //                   system state relevant to Win10 Onboarding and returns
+    //                   a struct.
+    //                 * Add a function, CheckWin10OnboardingTabPolicy, which
+    //                   takes such a struct as input and returns StartupTabs.
+    return false;
+  }
+#endif
+
+  StartupTabs insert_tabs =
+      CheckStandardOnboardingTabPolicy(first_run::IsChromeFirstRun());
+  tabs->insert(tabs->end(), insert_tabs.begin(), insert_tabs.end());
+  return !insert_tabs.empty();
+}
+
+bool StartupTabProviderImpl::AddDistributionFirstRunTabs(
+    StartupBrowserCreator* browser_creator,
+    StartupTabs* tabs) const {
+  StartupTabs insert_tabs;

[grt (UTC plus 2), 2016/09/12 07:28:46]

how about shortcutting the special case:
  if (!browser_creator)
    return false;
  StartupTabs insert_tabs = ...

+  if (browser_creator) {
+    insert_tabs = CheckMasterPrefsTabPolicy(first_run::IsChromeFirstRun(),
+                                            browser_creator->first_run_tabs_);
+    browser_creator->first_run_tabs_.clear();
+  }
+  tabs->insert(tabs->end(), insert_tabs.begin(), insert_tabs.end());
+  return !insert_tabs.empty();
+}
+
+bool StartupTabProviderImpl::AddResetTriggerTabs(Profile* profile,
+                                                 StartupTabs* tabs) const {
+  bool has_reset_trigger = false;
+#if defined(OS_WIN)
+  TriggeredProfileResetter* triggered_profile_resetter =
+      TriggeredProfileResetterFactory::GetForBrowserContext(profile_);
+  if (triggered_profile_resetter) {

[grt (UTC plus 2), 2016/09/12 07:28:46]

nit: omit braces

+    has_reset_trigger = triggered_profile_resetter->HasResetTrigger();
+  }
+#endif  // defined(OS_WIN)
+  StartupTabs insert_tabs = CheckResetTriggerTabPolicy(has_reset_trigger);
+  tabs->insert(tabs->end(), insert_tabs.begin(), insert_tabs.end());
+  return !insert_tabs.empty();
+}
+
+bool StartupTabProviderImpl::AddPinnedTabs(StartupTabs* tabs) const {
+  // TODO(tmartino): Copy/clean up logic from
+  // StartupBrowserCreatorImpl::ProcessSpecifiedUrls.
+  return false;
+}
+
+bool StartupTabProviderImpl::AddPreferencesTabs(StartupTabs* tabs) const {
+  // TODO(tmartino): Copy/clean up logic from
+  // StartupBrowserCreatorImpl::ProcessStartupUrls.
+  return false;
+}
+
+// static
+StartupTabs StartupTabProviderImpl::CheckStandardOnboardingTabPolicy(

[grt (UTC plus 2), 2016/09/12 07:28:46]

there's room to get rid of additional copies by making these static methods
append to a given StartupTabs instance. will that somehow make things much
uglier?

[tmartino, 2016/09/13 23:22:23]

Initially I was hesitant to use the adder pattern anywhere--I like using a
return because it structurally enforces that policy functions can't depending on
prior functions to change their own behavior. Adding the StartupTabs* argument
also means someone can come in and start reading from that value. That said, I
think we've reached a structure that at least discourages this, and I do agree
that the performance concerns are legitimate.

I'll also rename the arguments from "tabs" to "output" to drive the point home
:)

+    bool is_first_run) {
+  StartupTabs tabs;
+  if (is_first_run)
+    tabs.push_back(StartupTab(GetWelcomePageUrl(), false));
+  return tabs;
+}
+
+// static
+StartupTabs StartupTabProviderImpl::CheckMasterPrefsTabPolicy(
+    bool is_first_run,
+    const std::vector<GURL>& first_run_tabs) {
+  StartupTabs processed_tabs;
+  if (is_first_run) {
+    processed_tabs.reserve(first_run_tabs.size());
+    for (const GURL& url : first_run_tabs) {
+      if (url.host() == kNewTabUrlHost) {
+        processed_tabs.push_back(
+            StartupTab(GURL(chrome::kChromeUINewTabURL), false));
+      } else if (url.host() == kWelcomePageUrlHost) {
+        processed_tabs.push_back(StartupTab(GetWelcomePageUrl(), false));
+      } else {
+        processed_tabs.push_back(StartupTab(url, false));
+      }
+    }
+  }
+  return processed_tabs;
+}
+
+// static
+StartupTabs StartupTabProviderImpl::CheckResetTriggerTabPolicy(
+    bool profile_has_trigger) {
+  StartupTabs tabs;
+  if (profile_has_trigger)
+    tabs.push_back(StartupTab(GetTriggeredResetSettingsUrl(), false));
+  return tabs;
+}
+
+GURL GetWelcomePageUrl() {
+  return GURL(l10n_util::GetStringUTF8(IDS_WELCOME_PAGE_URL));
+}
+
+GURL GetTriggeredResetSettingsUrl() {
+  return GURL(
+      chrome::GetSettingsUrl(chrome::kTriggeredResetProfileSettingsSubPage));
+}