Welcome page changes for Windows 10 and over.

chrome/browser/ui/startup/startup_browser_creator_impl.cc

Index: chrome/browser/ui/startup/startup_browser_creator_impl.cc
diff --git a/chrome/browser/ui/startup/startup_browser_creator_impl.cc b/chrome/browser/ui/startup/startup_browser_creator_impl.cc
index b6a1fdde98ad4522137de8b3d1e13ac83510e9c2..871073add80d9a74807080219951374ca6b62a06 100644
--- a/chrome/browser/ui/startup/startup_browser_creator_impl.cc
+++ b/chrome/browser/ui/startup/startup_browser_creator_impl.cc
@@ -22,6 +22,7 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
+#include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_restrictions.h"
 #include "chrome/browser/apps/install_chrome_app.h"
@@ -76,6 +77,7 @@
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "chrome/common/extensions/extension_metrics.h"
+#include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/grit/locale_settings.h"
 #include "chrome/installer/util/browser_distribution.h"
@@ -91,6 +93,7 @@
 #include "extensions/common/constants.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_set.h"
+#include "net/base/network_change_notifier.h"
 #include "ui/base/l10n/l10n_util.h"
 
 #if defined(OS_MACOSX)
@@ -297,7 +300,8 @@ StartupBrowserCreatorImpl::StartupBrowserCreatorImpl(
       command_line_(command_line),
       profile_(NULL),
       browser_creator_(NULL),
-      is_first_run_(is_first_run == chrome::startup::IS_FIRST_RUN) {
+      is_first_run_(is_first_run == chrome::startup::IS_FIRST_RUN),
+      welcome_run_type_(WelcomeRunType::NONE) {
 }
 
 StartupBrowserCreatorImpl::StartupBrowserCreatorImpl(
@@ -309,7 +313,8 @@ StartupBrowserCreatorImpl::StartupBrowserCreatorImpl(
       command_line_(command_line),
       profile_(NULL),
       browser_creator_(browser_creator),
-      is_first_run_(is_first_run == chrome::startup::IS_FIRST_RUN) {
+      is_first_run_(is_first_run == chrome::startup::IS_FIRST_RUN),
+      welcome_run_type_(WelcomeRunType::NONE) {
 }
 
 StartupBrowserCreatorImpl::~StartupBrowserCreatorImpl() {
@@ -535,6 +540,9 @@ void StartupBrowserCreatorImpl::ProcessLaunchURLs(
     return;
   }
 
+  // Determine whether or not this launch must include the welcome page.
+  InitializeWelcomeRunType(urls_to_open);
+
 // TODO(tapted): Move this to startup_browser_creator_win.cc after refactor.
 #if defined(OS_WIN)
   if (base::win::GetVersion() >= base::win::VERSION_WIN8) {
@@ -561,11 +569,18 @@ void StartupBrowserCreatorImpl::ProcessLaunchURLs(
     SessionService* service =
         SessionServiceFactory::GetForProfileForSessionRestore(profile_);
     if (service && service->ShouldNewWindowStartSession()) {
-      // Restore the last session if any.
-      if (!HasPendingUncleanExit(profile_) &&
-          service->RestoreIfNecessary(urls_to_open)) {
-        return;
+      // Restore the last session if any, optionally including the welcome page
+      // if desired.
+      if (!HasPendingUncleanExit(profile_)) {
+        std::vector<GURL> adjusted_urls(urls_to_open);
+        if (welcome_run_type_ != WelcomeRunType::NONE) {
+          adjusted_urls.insert(adjusted_urls.begin(),

[msw, 2015/07/10 22:07:32]

Could this be inserting the page at the front for a last case? I'd suggest
adding a helper like void AddWelcomePageIfNeeded(std::vector<GURL>*
urls_to_open) that would address the code duplication, inconsistency of
supported enum values, and potential re-addition of the welcome page (by not
setting |welcome_run_type_| to NONE after adding...), but I guess you need to
potentially add the welcome page to the url vector multiple times (because
restore might not take hold), and really, I'd like the existing helper to just
add the page, as I requested before, but I guess that's not possible... It's
hard to make sense of this mess.

[grt (UTC plus 2), 2015/07/11 12:16:08]

This change started with a simple helper like that, but blossomed into the
beautiful flower it has become because it had to handle all of the odd cases
spread about this file. :-(
Agreed.

+                               internals::GetWelcomePageURL());
+        }
+        if (service->RestoreIfNecessary(adjusted_urls))
+          return;
       }
+
       // Open user-specified URLs like pinned tabs and startup tabs.
       Browser* browser = ProcessSpecifiedURLs(urls_to_open, desktop_type);
       if (browser) {
@@ -577,9 +592,9 @@ void StartupBrowserCreatorImpl::ProcessLaunchURLs(
 
   // Session startup didn't occur, open the urls.
   Browser* browser = NULL;
-  std::vector<GURL> adjust_urls = urls_to_open;
-  if (adjust_urls.empty()) {
-    AddStartupURLs(&adjust_urls);
+  std::vector<GURL> adjusted_urls = urls_to_open;
+  if (adjusted_urls.empty()) {
+    AddStartupURLs(&adjusted_urls);
   } else if (!command_line_.HasSwitch(switches::kOpenInNewWindow)) {
     // Always open a list of urls in a window on the native desktop.
     browser = chrome::FindTabbedBrowser(profile_, false,
@@ -587,8 +602,8 @@ void StartupBrowserCreatorImpl::ProcessLaunchURLs(
   }
   // This will launch a browser; prevent session restore.
   StartupBrowserCreator::in_synchronous_profile_launch_ = true;
-  browser = OpenURLsInBrowser(browser, process_startup, adjust_urls,
-                              desktop_type);
+  browser =
+      OpenURLsInBrowser(browser, process_startup, adjusted_urls, desktop_type);
   StartupBrowserCreator::in_synchronous_profile_launch_ = false;
   AddInfoBarsIfNecessary(browser, is_process_startup);
 }
@@ -642,11 +657,17 @@ bool StartupBrowserCreatorImpl::ProcessStartupURLs(
     }
 #endif
 
+    // Optionally include the welcome page.
+    std::vector<GURL> adjusted_urls(urls_to_open);
+    if (welcome_run_type_ != WelcomeRunType::NONE) {
+      adjusted_urls.insert(adjusted_urls.begin(),

[msw, 2015/07/10 22:07:32]

Why is this always inserting first, why doesn't this need to handle *_LAST?

[grt (UTC plus 2), 2015/07/11 12:16:08]

This is handling session restore rather than first run.

+                           internals::GetWelcomePageURL());
+    }
+
     // The startup code only executes for browsers launched in desktop mode.
     // i.e. HOST_DESKTOP_TYPE_NATIVE. Ash should never get here.
     Browser* browser = SessionRestore::RestoreSession(
-        profile_, NULL, desktop_type, restore_behavior,
-        urls_to_open);
+        profile_, NULL, desktop_type, restore_behavior, adjusted_urls);
 
     AddInfoBarsIfNecessary(browser, chrome::startup::IS_PROCESS_STARTUP);
     return true;
@@ -699,6 +720,9 @@ Browser* StartupBrowserCreatorImpl::ProcessSpecifiedURLs(
     UrlsToTabs(urls, &tabs);
   } else if (pref.type == SessionStartupPref::URLS && !pref.urls.empty() &&
              !HasPendingUncleanExit(profile_)) {
+    // Optionally include the welcome page first.
+    if (welcome_run_type_ == WelcomeRunType::ANY_RUN_FIRST)

[msw, 2015/07/10 22:07:32]

Ditto: Why doesn't this need to handle FIRST_RUN_*?

[grt (UTC plus 2), 2015/07/11 12:16:08]

Because this isn't a first-run codepath.

+      UrlsToTabs(std::vector<GURL>(1, internals::GetWelcomePageURL()), &tabs);
     // Only use the set of urls specified in preferences if nothing was
     // specified on the command line. Filter out any urls that are to be
     // restored by virtue of having been previously pinned.
@@ -880,12 +904,16 @@ void StartupBrowserCreatorImpl::AddStartupURLs(
     }
   }
 
-  // Otherwise open at least the new tab page (and the welcome page, if this
-  // is the first time the browser is being started), or the set of URLs
+  // Otherwise open at least the new tab page (and optionally the welcome page
+  // at the front or back as indicated by welcome_run_type_), or the set of URLs
   // specified on the command line.
   if (startup_urls->empty()) {
+    if (welcome_run_type_ == WelcomeRunType::FIRST_RUN_FIRST ||
+        welcome_run_type_ == WelcomeRunType::ANY_RUN_FIRST) {
+      startup_urls->push_back(internals::GetWelcomePageURL());
+    }
     startup_urls->push_back(GURL(chrome::kChromeUINewTabURL));
-    if (first_run::ShouldShowWelcomePage())
+    if (welcome_run_type_ == WelcomeRunType::FIRST_RUN_LAST)
       startup_urls->push_back(internals::GetWelcomePageURL());
   }
 
@@ -917,3 +945,74 @@ void StartupBrowserCreatorImpl::AddStartupURLs(
     }
   }
 }
+
+// For first-run, the type will be FIRST_RUN_LAST for all systems except for
+// Windows 10+, where it will be FIRST_RUN_FIRST. For non-first run, the type
+// will be NONE for all systems except for Windows 10+, where it will be
+// ANY_RUN_FIRST if this is the first somewhat normal launch since an OS
+// upgrade.
+void StartupBrowserCreatorImpl::InitializeWelcomeRunType(
+    const std::vector<GURL>& urls_to_open) {
+  DCHECK_EQ(static_cast<int>(WelcomeRunType::NONE),
+            static_cast<int>(welcome_run_type_));
+#if defined(OS_WIN)
+  // Do not welcome if there are any URLs to open.
+  if (!urls_to_open.empty())
+    return;
+
+  base::win::OSInfo* const os_info = base::win::OSInfo::GetInstance();
+  const base::win::OSInfo::VersionNumber v(os_info->version_number());
+  const std::string this_version(base::StringPrintf("%d.%d", v.major, v.minor));
+  PrefService* const local_state = g_browser_process->local_state();
+
+  // Always welcome on first run.
+  if (first_run::ShouldShowWelcomePage()) {
+    // First tab for Win10+, last tab otherwise.
+    welcome_run_type_ = os_info->version() >= base::win::VERSION_WIN10
+                            ? WelcomeRunType::FIRST_RUN_FIRST

[msw, 2015/07/10 22:07:32]

It would be better to reduce the enum to NONE/FIRST/LAST now, rather than in a
followup. Forgive me for being a bit skeptical that anyone would want to come
back and resolve some technical debt in this file.

[grt (UTC plus 2), 2015/07/11 12:16:08]

I'll take a stab at it in the next patch set to see if/how it simplifies things.

I will add my thoughts on how this class could be simplified and made
unit-testable to the existing "the creator is too damn complex" bug.

[grt (UTC plus 2), 2015/07/13 15:10:07]

It turned out to be quite simple. PTAL.

+                            : WelcomeRunType::FIRST_RUN_LAST;

[msw, 2015/07/10 22:07:32]

I don't think you ever addressed my question: Better yet, why don't we just
always show the welcome tab first? Is there any reason to have distinct welcome
tab ordering for different cases?

[grt (UTC plus 2), 2015/07/11 12:16:08]

Because this is what the product deciders have decided: prior to this change,
the signin promo is king and takes the first slot in place of the NTP, while the
welcome page takes the second slot. following this change, Win10 is special in
that there is no signin promo and the welcome page takes the first slot while
the NTP takes the second.

[msw, 2015/07/13 19:16:05]

Then let's double check with the deciders [sic]. When design preferences
substantially increase code complexity and maintainability costs, it's worth
circling back. At best, you might find out that the preference wasn't strong
enough to merit such hassle, and simplicity wins out. At worst, we get this
beautiful flower of a CL. Either way, the costs are revealed and there's more
recognition of the technical debt here.

+  } else {
+    // Otherwise, do not welcome pre-Win10.
+    if (base::win::GetVersion() < base::win::VERSION_WIN10)
+      return;
+
+    // Do not welcome if launching into incognito.
+    if (IncognitoModePrefs::ShouldLaunchIncognito(command_line_,
+                                                  profile_->GetPrefs())) {
+      return;
+    }
+
+    // Do not welcome if there is no local state (tests).
+    if (!local_state)
+      return;
+
+    // Do not welcome if disabled by policy or master_preferences.
+    if (!local_state->GetBoolean(prefs::kWelcomePageOnOSUpgradeEnabled))
+      return;
+
+    // Do not welcome if already shown for this OS version.
+    if (local_state->GetString(prefs::kLastWelcomedOSVersion) == this_version)
+      return;
+
+    // Do not welcome if offline.
+    if (net::NetworkChangeNotifier::IsOffline())
+      return;
+
+    // Do not welcome if Chrome was the default browser at startup.
+    if (g_browser_process->CachedDefaultWebClientState() ==
+        ShellIntegration::IS_DEFAULT) {
+      return;
+    }
+
+    // Show the welcome page in the first tab.
+    welcome_run_type_ = WelcomeRunType::ANY_RUN_FIRST;
+  }
+
+  // Remember that the welcome page was shown for this OS version.
+  local_state->SetString(prefs::kLastWelcomedOSVersion, this_version);

[msw, 2015/07/10 22:07:32]

By inlining RecordWelcomeRunComplete here, that prevents addressing gab's point:

Should we also reset |welcome_run_type_ = WelcomeRunType::NONE| here? i.e. I'm
not 100% convinced by reading the above code that some other piece of code won't
potentially end up handling something a second time based on this stale value
(SO MUCH code potentially having access to this member..
startup/rendez-vous/other types of restores?/remote sync session
restores?/bookmark folder restore?).

Could the welcome page now be added twice in the spaghetti above?

[grt (UTC plus 2), 2015/07/11 12:16:08]

A potentially stale value is only alive for the lifetime of the
StartupBrowserCreatorImpl, which is a stack object used to handle a single
launch. It does not live for the lifetime of the browser process. There is no
risk of the value being used for a later rendez-vous, other restore, etc.

That said, if recording the welcome page is done in this function, then either:
1) welcome_run_type_ must retain its initial value so that if it isn't handled
during session restore it can be handled in the fallback cases, or
2) extra handling is needed to revert the pref value and reset welcome_run_type_
when session restore fails so that it can be handled in the fallback case.
Which takes us back to why I'd pulled it out into its own function in the first
place. I'm happy to go back to using RecordWelcomeRunComplete if you find that
cleaner (I did, since it was an attempt to record what had actually happened
rather than what was expected to happen).

[msw, 2015/07/13 19:16:05]

I think it's okay as-is, but I don't feel too strongly either way.

+#else  // OS_WIN
+  // Show the welcome page as the last tab only on first-run.
+  if (first_run::ShouldShowWelcomePage())
+    welcome_run_type_ = WelcomeRunType::FIRST_RUN_LAST;
+#endif  // !OS_WIN
+}