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..b6b991671be9fee06766ff915eca8b499b591dba 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)
@@ -287,6 +290,24 @@ GURL GetWelcomePageURL() {
   return GURL(welcome_url);
 }
 
+bool ShowWelcomePageAfterFirstRun() {
+#if defined(OS_WIN)
+  return base::win::GetVersion() >= base::win::VERSION_WIN10;
+#else
+  return false;
+#endif
+}
+
+void RecordWelcomeRunComplete() {
+#if defined(OS_WIN)
+  using base::win::OSInfo;
+  OSInfo::VersionNumber v(OSInfo::GetInstance()->version_number());

[gab, 2015/07/09 18:05:12]

const

+  std::string os_version(base::StringPrintf("%d.%d", v.major, v.minor));

[gab, 2015/07/09 18:05:13]

const

+  g_browser_process->local_state()->SetString(prefs::kLastWelcomedOSVersion,
+                                              os_version);
+#endif
+}
+
 }  // namespace internals
 
 StartupBrowserCreatorImpl::StartupBrowserCreatorImpl(
@@ -297,7 +318,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 +331,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 +558,9 @@ void StartupBrowserCreatorImpl::ProcessLaunchURLs(
     return;
   }
 
+  // Determine whether or not this launch must include the welcome page.
+  InitializeWelcomeRunType(urls_to_open);

[msw, 2015/07/09 17:51:31]

I feel like there's a lot of unnecessary complexity here... Why doesn't this
member function simply add the welcome page to |urls_to_open| and record it as
shown, as needed? I don't see the value in having a separate state to
initialize, having the codepaths below actually add the page, and waiting until
the page is actually shown to record the prefs.

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

I originally had it as you describe, but found that it was inadequate for the
session restore case. Specifically, SessionService::RestoreIfNecessary is called
in cases where there may or may not be a session restore. Either the pref update
needs to be backed out if RestoreIfNecessary returns false, or the state in the
pref needs to be committed after RestoreIfNecessary returns true.

[msw, 2015/07/10 19:40:45]

But if RestoreIfNecessary returns false, the code falls through to open the urls
in another manner, so the welcome page would still be shown so long as it's
included in |urls_to_open|, right? There must be some way to reasonably avoid
the duplicated code across ProcessLaunchURLs, ProcessStartupURLs,
ProcessSpecifiedURLs, and AddStartupURLs. At some point, the costs of added
complexity aren't warranted by the corner cases it's covering (showing
absolutely first/last in every scenario)... I wholeheartedly second atwilson's
TODO ("Simplify the logic that decides which tabs to open on start-up and make
it more consistent. http://crbug.com/248883") and wonder if you ought to find a
reviewer more familiar with this code.

[grt (UTC plus 2), 2015/07/10 20:35:12]

Ah, yes, I think I can get rid of RecordWelcomeRunComplete now that state is
kept in the instance. I like this. Done.
Unfortunately, the corner cases are effectively all of the main use cases:
startup with the default tabs, startup with a specific set of urls, and session
restore. The existing tests are fairly comprehensive (and caught a regression
I'd inadvertently introduced), so I don't think that this change is a risk of
introducing a catastrophic break.
Me, too! It's a wreck.
I think one reason this code is so complicated now is that there is no true
owner. PK was unable to suggest anyone, so I'm not sure who to turn to, other
than the last people who have made edits. From what I've seen looking at the
logs, most changes have been spot additions related to other things, so these
people likely do not feel great ownership over it.

+
 // 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 +587,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> adjust_urls(urls_to_open);

[gab, 2015/07/09 18:05:12]

s/adjust_urls/adjusted_urls/

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

Done.

+        if (welcome_run_type_ != WelcomeRunType::NONE)
+          adjust_urls.push_back(internals::GetWelcomePageURL());

[gab, 2015/07/09 18:05:12]

Doesn't this unconditionally put it last?

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

adjust_urls will be empty, so it'll do the right thing. since that seems a bit
subtle to me now that you point it out, i'll make it explicitly add it to the
front (which will be the same, in practice).

+        if (service->RestoreIfNecessary(adjust_urls)) {
+          RecordWelcomeRunComplete();
+          return;
+        }
       }
+
       // Open user-specified URLs like pinned tabs and startup tabs.
       Browser* browser = ProcessSpecifiedURLs(urls_to_open, desktop_type);
       if (browser) {
@@ -642,11 +675,17 @@ bool StartupBrowserCreatorImpl::ProcessStartupURLs(
     }
 #endif
 
+    // Optionally include the welcome page.
+    std::vector<GURL> adjust_urls(urls_to_open);

[gab, 2015/07/09 18:05:13]

s/adjust_urls/adjusted_urls/

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

Done.

+    if (welcome_run_type_ != WelcomeRunType::NONE)
+      adjust_urls.push_back(internals::GetWelcomePageURL());

[gab, 2015/07/09 18:05:12]

Again, why is unconditionally pushing to the back okay here?

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

Done.

+
     // 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, adjust_urls);
+
+    RecordWelcomeRunComplete();
 
     AddInfoBarsIfNecessary(browser, chrome::startup::IS_PROCESS_STARTUP);
     return true;
@@ -699,6 +738,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)

[gab, 2015/07/09 18:05:12]

Why not also FIRST_RUN_FIRST? I guess I don't understand why you need the 4th
Type (why is NONE, FIRST, LAST not sufficient?)

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

Here's the explanation I gave to msw:

I didn't differentiate these initially, but it turned out to be
significant. The handling of SessionStartupPref::URLS in
StartupBrowserCreatorImpl::ProcessSpecifiedURLs shouldn't add the
welcome page during first run since this would add it when an enterprise
uses the kRestoreOnStartup policy to specify the tabs to be used for
startup.

An alternative is for InitializeWelcomeRunType to set the type to NONE
if the startup URLs are specified by policy. I think that would get rid
of the need for the extra value here and would potentially be safer for
enterprises. What do you think?

+      UrlsToTabs(std::vector<GURL>(1, internals::GetWelcomePageURL()), &tabs);

[gab, 2015/07/09 18:05:12]

UrlsToTabs()'s name is misleading :-( -- it's really AppendTabsFromUrls()

(I'm not suggesting you change it here, just stating my confusion)

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

Acknowledged.

     // 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.
@@ -763,6 +805,7 @@ Browser* StartupBrowserCreatorImpl::OpenTabsInBrowser(
     browser = new Browser(Browser::CreateParams(profile_, desktop_type));
 
   bool first_tab = true;
+  const GURL welcome_url(internals::GetWelcomePageURL());
   ProtocolHandlerRegistry* registry = profile_ ?
       ProtocolHandlerRegistryFactory::GetForBrowserContext(profile_) : NULL;
   for (size_t i = 0; i < tabs.size(); ++i) {
@@ -793,6 +836,9 @@ Browser* StartupBrowserCreatorImpl::OpenTabsInBrowser(
     }
 #endif  // defined(ENABLE_RLZ) && !defined(OS_IOS)
 
+    if (welcome_run_type_ != WelcomeRunType::NONE && tabs[i].url == welcome_url)
+      RecordWelcomeRunComplete();
+
     chrome::Navigate(&params);
 
     first_tab = false;
@@ -880,12 +926,18 @@ 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
-  // specified on the command line.
+  // 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 first time following an
+  // upgrade past Windows 10), or the set of URLs specified on the command line.
   if (startup_urls->empty()) {
+    // Put the welcome page before the new tab page on Windows 10.

[msw, 2015/07/09 17:51:31]

nit: remove the comment about win10. This code here isn't win10 specific, even
if the code setting |welcome_run_type_| is os/version specific.

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

Done.

+    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())
+    // Put the welcome page after the new tab page elsewhere.
+    if (welcome_run_type_ == WelcomeRunType::FIRST_RUN_LAST)
       startup_urls->push_back(internals::GetWelcomePageURL());
   }
 
@@ -917,3 +969,82 @@ void StartupBrowserCreatorImpl::AddStartupURLs(
     }
   }
 }
+
+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* os_info = base::win::OSInfo::GetInstance();

[gab, 2015/07/09 18:05:12]

const

[grt (UTC plus 2), 2015/07/09 18:59:37]

which thing, the pointer or the thing being pointed to? consting the thing being
pointed to would work, although it would mean this code couldn't use one of the
getters if it wanted to. consting the pointer would look odd. i find both
displeasing for different reasons.

+
+  // 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
+                            : WelcomeRunType::FIRST_RUN_LAST;
+    return;
+  }
+
+  // Otherwise, do not welcome pre-Win10.
+  if (!internals::ShowWelcomePageAfterFirstRun())
+    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).
+  PrefService* local_state = g_browser_process->local_state();

[gab, 2015/07/09 18:05:12]

const

+  if (!local_state)
+    return;
+
+  // Do not welcome if disabled by policy or master_preferences.
+  if (!local_state->GetBoolean(prefs::kWelcomePageOnOSUpgradeEnabled))
+    return;
+
+  base::win::OSInfo::VersionNumber v(os_info->version_number());

[gab, 2015/07/09 18:05:12]

const

[grt (UTC plus 2), 2015/07/09 18:59:37]

i find that consting all variables that don't happen to be modified after
initialization clutters up the function. is there a compelling reason to do
this?

+  std::string this_os_version(base::StringPrintf("%d.%d", v.major, v.minor));

[gab, 2015/07/09 18:05:12]

const

+
+  // Do not welcome if already shown for this OS version.
+  if (local_state->GetString(prefs::kLastWelcomedOSVersion) == this_os_version)
+    return;
+
+  // Do not welcome if offline.
+  if (net::NetworkChangeNotifier::IsOffline())
+    return;
+
+  // Do not welcome if Chrome is the default browser.
+  {
+    // Checking for default browser will touch the filesystem. This phase of
+    // launch is synchronous, so there is no choice to bounce to another thread
+    // now. Since this is the last condition checked before adding the welcome
+    // page, it will be hit only once per user.
+    base::ThreadRestrictions::ScopedAllowIO allow_io;
+    if (ShellIntegration::GetDefaultBrowser() == ShellIntegration::IS_DEFAULT)
+      return;
+  }
+
+  // Show the welcome page in the first tab.
+  welcome_run_type_ = WelcomeRunType::ANY_RUN_FIRST;
+#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
+}
+
+void StartupBrowserCreatorImpl::RecordWelcomeRunComplete() const {
+#if defined(OS_WIN)
+  if (welcome_run_type_ == WelcomeRunType::NONE)
+    return;
+  // Remember that the welcome page was added for this OS version.

[msw, 2015/07/09 17:51:31]

nit: s/added/shown/

[grt (UTC plus 2), 2015/07/09 18:59:38]

nice catch. done.

+  internals::RecordWelcomeRunComplete();

[gab, 2015/07/09 18:05:12]

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?).

[grt (UTC plus 2), 2015/07/09 18:59:37]

Good point. Done.

+#endif
+}