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 "build/build_config.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)
+#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";

[Peter Kasting, 2016/09/29 07:23:35]

Nit: Prefer to declare constants in the narrowest scope possible, in this case
in the one function below that uses these rather than at file scope.

+constexpr char kWelcomePageUrlHost[] = "welcome_page";

[Peter Kasting, 2016/09/29 07:23:35]

Oh dear... these are valid hostnames for intranets.  We really should not use
this method of specifying magic constants unless these are only stored in a
place we know intranet URLs could never go, e.g. the URL is
"chrome://new_tab_page/" with the chrome:// scheme clearly indicating this is an
internal thing (but then why not just use chrome://newtab/ directly?).

If fixing this involves some kind of migration, do it separately -- but if at
all possible please do fix this during this process.

[tmartino, 2016/09/30 20:55:02]

I hate, hate, hate this too. I was horrified when I came across this logic when
untangling the spaghetti. Unfortunately, I don't think there's anything that can
be done about it. These don't seem to be defined anywhere else--it's entirely by
convention--but they're used in Master Preferences files created by external
partners and administrators, and bundled with Chrome in various distributions.

https://support.google.com/chrome/a/answer/187948?hl=en

[Peter Kasting, 2016/09/30 21:50:03]

That's something we can change over time.  It would require asking the master
prefs folks how we would go about making a breaking change (e.g. ship support
for both, announce deprecation somehow, set timelines, then remove the old
thing).  I think this is worth doing, but it will require some intentionality on
your part if you want to make it happen.

[tmartino, 2016/10/03 23:26:06]

Added a line to the tech debt phase of my project plan.

+}  // namespace
+
+bool StartupTabProviderImpl::AddOnboardingTabs(StartupTabs* tabs) const {
+#if defined(OS_WIN)

[Peter Kasting, 2016/09/29 07:23:35]

Nit: If it wouldn't add excessive indirection/boilerplate/etc., consider a
startup_tab_provider_win.cc that defines Windows-specific functionality in
addition to/in place of things in this file, done in such a way that we don't
end up using ifdefs in this file (there are a couple ways to do this).

[tmartino, 2016/09/30 20:55:02]

We discussed this quite a few patchsets ago (first_run.cc in #2) and I wasn't a
fan of that option. The fact that the remainder of this function is still needed
on Windows builds (for versions < 10) means that we'd be adding a lot of
indirection to get "back" here from the Windows version.

As an alternative, I'd be OK with doing all the work described in the TODO in a
_win.cc and having that call be the only if-def'd portion.

[Peter Kasting, 2016/09/30 21:50:03]

I'm open to any working solution, and admittedly it's hard to design without the
context of the full eventual implementation here.  Possibilities:

* Add a PlatformSpecificAddOnboardingTabs() function which Windows implements as
non-empty and other platforms implement as empty; call that here.

* Make people who want this impl class get it via a factory function; return a
Windows-specific subclass from that factory function, implement that to do the
Windowsy stuff and then call the base.  These functions are already virtual, so
this is less boilerplatey than it might sound.

It's possible neither of these are wins.  It's hard for me to tell.  I probably
would have been inclined to go with the second bullet.

[tmartino, 2016/10/03 23:26:06]

I think I'm OK with the second; for reasons of scope and sanity, I'll hold off
and add that file in the followup CL that actually implements the relevant
logic.

+  // 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
+
+  return CheckStandardOnboardingTabPolicy(first_run::IsChromeFirstRun(), tabs);
+}
+
+bool StartupTabProviderImpl::AddDistributionFirstRunTabs(
+    StartupBrowserCreator* browser_creator,
+    StartupTabs* tabs) const {
+  if (!browser_creator)
+    return false;
+  bool added_tabs = CheckMasterPrefsTabPolicy(
+      first_run::IsChromeFirstRun(), browser_creator->first_run_tabs_, tabs);
+  browser_creator->first_run_tabs_.clear();
+  return added_tabs;
+}
+
+bool StartupTabProviderImpl::AddResetTriggerTabs(Profile* profile,
+                                                 StartupTabs* tabs) const {
+  bool has_reset_trigger = false;
+#if defined(OS_WIN)

[Peter Kasting, 2016/09/29 07:23:35]

Why does this code only run for Windows? it seems like HasResetTrigger() will
never return true on other platforms, but is properly implemented on all and
should be safe to Just Call Everywhere.

That would simplify this function a bit.

+  TriggeredProfileResetter* triggered_profile_resetter =

[Peter Kasting, 2016/09/29 07:23:35]

Nit: Can this be const?

auto* might reduce redundancy here

+      TriggeredProfileResetterFactory::GetForBrowserContext(profile);
+  if (triggered_profile_resetter)
+    has_reset_trigger = triggered_profile_resetter->HasResetTrigger();

[Peter Kasting, 2016/09/29 07:23:35]

Nit: Or this.  Not really different in this implementation, but useful to avoid
needing a temp at all if you eliminate the #ifdef OS_WIN.

  has_reset_trigger = triggered_profile_resetter &&
                      triggered_profile_resetter->HasResetTrigger();

+#endif  // defined(OS_WIN)
+  return CheckResetTriggerTabPolicy(has_reset_trigger, tabs);
+}
+
+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
+bool StartupTabProviderImpl::CheckStandardOnboardingTabPolicy(
+    bool is_first_run,
+    StartupTabs* tabs) {
+  if (is_first_run) {
+    tabs->push_back(StartupTab(GetWelcomePageUrl(), false));

[Peter Kasting, 2016/09/29 07:23:35]

Nit: Same emplace_back() comment applies in this file too

+    return true;
+  }
+  return false;

[Peter Kasting, 2016/09/29 07:23:35]

Nit: Shorter:

  if (is_first_run)
    tabs->...
  return is_first_run;

+}
+
+// static
+bool StartupTabProviderImpl::CheckMasterPrefsTabPolicy(
+    bool is_first_run,
+    const std::vector<GURL>& first_run_tabs,
+    StartupTabs* tabs) {
+  size_t initial_size = tabs->size();
+  if (is_first_run) {
+    tabs->reserve(initial_size + first_run_tabs.size());
+    for (const GURL& url : first_run_tabs) {
+      if (url.host() == kNewTabUrlHost) {
+        tabs->push_back(StartupTab(GURL(chrome::kChromeUINewTabURL), false));

[Peter Kasting, 2016/09/29 07:23:35]

Nit: Rather than duplicate the construction of StartupTab and pushing it back,
seems like it might be nicer to do something like:

    for (GURL url : first_run_tabs) {
      if (url.host() == kNewTabUrlHost)
        url = GURL(chrome::kChromeUINewTabURL);
      else if (url.host() == kWelcomePageUrlHost)
        url = GetWelcomePageUrl();
      tabs->emplace_back(url, false);
    }

+      } else if (url.host() == kWelcomePageUrlHost) {
+        tabs->push_back(StartupTab(GetWelcomePageUrl(), false));
+      } else {
+        tabs->push_back(StartupTab(url, false));
+      }
+    }
+  }
+  return tabs->size() > initial_size;
+}
+
+// static
+bool StartupTabProviderImpl::CheckResetTriggerTabPolicy(
+    bool profile_has_trigger,
+    StartupTabs* tabs) {
+  if (!profile_has_trigger)
+    return false;
+  tabs->push_back(StartupTab(GetTriggeredResetSettingsUrl(), false));
+  return true;

[Peter Kasting, 2016/09/29 07:23:35]

Nit: Shorter:

  if (profile_has_trigger)
    tabs->...
  return profile_has_trigger;

+}
+
+GURL GetWelcomePageUrl() {
+  return GURL(l10n_util::GetStringUTF8(IDS_WELCOME_PAGE_URL));
+}
+
+GURL GetTriggeredResetSettingsUrl() {
+  return GURL(
+      chrome::GetSettingsUrl(chrome::kTriggeredResetProfileSettingsSubPage));
+}