Moving new First Run to use per-profile value, rather than simply checking IsChromeFirstRun.

chrome/browser/ui/startup/startup_tab_provider.cc

Index: chrome/browser/ui/startup/startup_tab_provider.cc
diff --git a/chrome/browser/ui/startup/startup_tab_provider.cc b/chrome/browser/ui/startup/startup_tab_provider.cc
index 7270275644e1d171f783803ac589aea2d65b709f..6b0beb9ddc87c86cc35a877b3100858d05ca6ad6 100644
--- a/chrome/browser/ui/startup/startup_tab_provider.cc
+++ b/chrome/browser/ui/startup/startup_tab_provider.cc
@@ -8,17 +8,25 @@
 #include "chrome/browser/first_run/first_run.h"
 #include "chrome/browser/profile_resetter/triggered_profile_resetter.h"
 #include "chrome/browser/profile_resetter/triggered_profile_resetter_factory.h"
+#include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/ui/chrome_pages.h"
 #include "chrome/browser/ui/tabs/pinned_tab_codec.h"
+#include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/grit/locale_settings.h"
+#include "components/prefs/pref_service.h"
+#include "components/signin/core/browser/signin_manager.h"
 #include "ui/base/l10n/l10n_util.h"
 
 #if defined(OS_WIN)
 #include "base/win/windows_version.h"
 #endif
 
-StartupTabs StartupTabProviderImpl::GetOnboardingTabs() const {
+namespace {
+constexpr char kVersionWelcomeIntroduced[] = "55.0.0.0";

[Peter Kasting, 2016/11/10 05:26:43]

I'm not a big fan of a version-number-based check for this.

You can use PrefService::HasPrefPath(prefs::kHasSeenWelcomeUI) to distinguish
between a pref that exists in a profile and one that doesn't.  Then just modify
the profile creation function in this CL to write "false" to that pref, and
HasPrefPath() will tell you if the profile is new enough or not.

That said, I think we can go even simpler: still have the profile creation code
write "false" for this pref, but register the pref with a default value of
"true".  Then you don't even need to check HasPrefPath(); just read the pref
value, and only show the UI if the pref is false.  (A clearer way of doing this
might be to reverse the sense of the pref to "should show welcome UI", default
to false, and have the profile creation function set it to true.)

[tmartino, 2016/11/15 00:16:30]

Agreed on the structural changes, but I've looked at this a fair amount and I
still can't place which function "the profile creation function in this CL" is
supposed to be. Could you elaborate on that?

[Peter Kasting, 2016/11/15 00:32:06]

Sorry, I think you parsed the phrase differently than I meant it.  Remove the
phrase "in this CL" entirely; I just meant that within this CL, you could do the
changes I'm describing to whatever the profile creation function is.  I didn't
mean "this CL already adds a function that creates profiles, you should modify
it" or the like.

+}  // namespace
+
+StartupTabs StartupTabProviderImpl::GetOnboardingTabs(Profile* profile) const {
 #if defined(OS_WIN)
   // Windows 10 has unique onboarding policies and content.
   if (base::win::GetVersion() >= base::win::VERSION_WIN10) {
@@ -31,7 +39,19 @@ StartupTabs StartupTabProviderImpl::GetOnboardingTabs() const {
   }
 #endif
 
-  return CheckStandardOnboardingTabPolicy(first_run::IsChromeFirstRun());
+  if (!profile)
+    return StartupTabs();
+
+  PrefService* prefs = profile->GetPrefs();
+  bool has_seen_welcome = prefs && prefs->GetBoolean(prefs::kHasSeenWelcomeUI);
+  SigninManager* signin_manager = SigninManagerFactory::GetForProfile(profile);
+  bool is_signed_in = signin_manager && signin_manager->IsAuthenticated();
+  bool created_after_welcome_ui =
+      profile->WasCreatedByVersionOrLater(kVersionWelcomeIntroduced);
+
+  return CheckStandardOnboardingTabPolicy(first_run::IsChromeFirstRun(),
+                                          has_seen_welcome, is_signed_in,
+                                          created_after_welcome_ui);
 }
 
 StartupTabs StartupTabProviderImpl::GetDistributionFirstRunTabs(
@@ -77,10 +97,13 @@ StartupTabs StartupTabProviderImpl::GetNewTabPageTabs(
 
 // static
 StartupTabs StartupTabProviderImpl::CheckStandardOnboardingTabPolicy(
-    bool is_first_run) {
+    bool is_first_run,
+    bool has_seen_welcome,
+    bool is_signed_in,
+    bool created_after_welcome_ui) {
   StartupTabs tabs;
-  if (is_first_run)
-    tabs.emplace_back(GetWelcomePageUrl(), false);
+  if (!has_seen_welcome && created_after_welcome_ui && !is_signed_in)
+    tabs.emplace_back(GetWelcomePageUrl(!is_first_run), false);
   return tabs;
 }
 
@@ -100,7 +123,7 @@ StartupTabs StartupTabProviderImpl::CheckMasterPrefsTabPolicy(
       if (url.host() == kNewTabUrlHost)
         url = GURL(chrome::kChromeUINewTabURL);
       else if (url.host() == kWelcomePageUrlHost)
-        url = GetWelcomePageUrl();
+        url = GetWelcomePageUrl(false);
       tabs.emplace_back(url, false);
     }
   }
@@ -145,8 +168,11 @@ StartupTabs StartupTabProviderImpl::CheckNewTabPageTabPolicy(
 }
 
 // static
-GURL StartupTabProviderImpl::GetWelcomePageUrl() {
-  return GURL(chrome::kChromeUIWelcomeURL);
+GURL StartupTabProviderImpl::GetWelcomePageUrl(bool use_later_run_variant) {
+  std::string url(chrome::kChromeUIWelcomeURL);
+  if (use_later_run_variant)
+    url.append("?variant=everywhere");

[Peter Kasting, 2016/11/10 05:26:43]

Nit: Safer in case someone changes the welcome URL to have a query param:

  GURL url(chrome::kChromeUIWelcomeURL);
  return use_later_run_variant
             ? net::AppendQueryParameter(url, "variant", "everywhere")
             : url;

(You can use AppendOrReplaceQueryParameter() instead if you think someone might
add a default value of "variant" to the base URL and you want to make sure this
forces it to the value you want.)

[tmartino, 2016/11/15 00:16:30]

Done

+  return GURL(url);
 }
 
 // static