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

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/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) {
     // 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 StartupTabs();
   }
 #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(
     StartupBrowserCreator* browser_creator) const {
   if (!browser_creator)
     return StartupTabs();
   StartupTabs tabs = CheckMasterPrefsTabPolicy(
       first_run::IsChromeFirstRun(), browser_creator->first_run_tabs_);
   browser_creator->first_run_tabs_.clear();
   return tabs;
@@ skipping 25 matching lines @@
 
 StartupTabs StartupTabProviderImpl::GetNewTabPageTabs(
     const base::CommandLine& command_line,
     Profile* profile) const {
   return CheckNewTabPageTabPolicy(
       StartupBrowserCreator::GetSessionStartupPref(command_line, profile));
 }
 
 // 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;
 }
 
 // static
 StartupTabs StartupTabProviderImpl::CheckMasterPrefsTabPolicy(
     bool is_first_run,
     const std::vector<GURL>& first_run_tabs) {
   // Constants: Magic words used by Master Preferences files in place of a URL
   // host to indicate that internal pages should appear on first run.
   static constexpr char kNewTabUrlHost[] = "new_tab_page";
   static constexpr char kWelcomePageUrlHost[] = "welcome_page";
 
   StartupTabs tabs;
   if (is_first_run) {
     tabs.reserve(first_run_tabs.size());
     for (GURL url : first_run_tabs) {
       if (url.host() == kNewTabUrlHost)
         url = GURL(chrome::kChromeUINewTabURL);
       else if (url.host() == kWelcomePageUrlHost)
-        url = GetWelcomePageUrl();
+        url = GetWelcomePageUrl(false);
       tabs.emplace_back(url, false);
     }
   }
   return tabs;
 }
 
 // static
 StartupTabs StartupTabProviderImpl::CheckResetTriggerTabPolicy(
     bool profile_has_trigger) {
   StartupTabs tabs;
@@ skipping 24 matching lines @@
 // static
 StartupTabs StartupTabProviderImpl::CheckNewTabPageTabPolicy(
     const SessionStartupPref& pref) {
   StartupTabs tabs;
   if (pref.type != SessionStartupPref::Type::LAST)
     tabs.emplace_back(GURL(chrome::kChromeUINewTabURL), false);
   return tabs;
 }
 
 // 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
 GURL StartupTabProviderImpl::GetTriggeredResetSettingsUrl() {
   return GURL(
       chrome::GetSettingsUrl(chrome::kTriggeredResetProfileSettingsSubPage));
 }