Welcome page changes for Windows 10 and over.

chrome/browser/ui/startup/startup_browser_creator_impl.cc

 // Copyright (c) 2012 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_browser_creator_impl.h"
 
 #include <algorithm>
 #include <vector>
 
 #include "apps/app_restore_service.h"
 #include "apps/app_restore_service_factory.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/environment.h"
 #include "base/lazy_instance.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/statistics_recorder.h"
 #include "base/prefs/pref_service.h"
 #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"
 #include "chrome/browser/auto_launch_trial.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/custom_handlers/protocol_handler_registry.h"
 #include "chrome/browser/custom_handlers/protocol_handler_registry_factory.h"
 #include "chrome/browser/defaults.h"
 #include "chrome/browser/extensions/extension_creator.h"
@@ skipping 34 matching lines @@
 #include "chrome/browser/ui/startup/startup_browser_creator.h"
 #include "chrome/browser/ui/tabs/pinned_tab_codec.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_result_codes.h"
 #include "chrome/common/chrome_switches.h"
 #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"
 #include "components/google/core/browser/google_util.h"
 #include "content/public/browser/child_process_security_policy.h"
 #include "content/public/browser/dom_storage_context.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/storage_partition.h"
 #include "content/public/browser/web_contents.h"
 #include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/extension_registry.h"
 #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)
 #include "base/mac/mac_util.h"
 #include "chrome/browser/ui/cocoa/keystone_infobar_delegate.h"
 #endif
 
 #if defined(OS_WIN)
 #include "base/win/windows_version.h"
 #include "chrome/browser/apps/app_launch_for_metro_restart_win.h"
@@ skipping 176 matching lines @@
 
 }  // namespace
 
 namespace internals {
 
 GURL GetWelcomePageURL() {
   std::string welcome_url = l10n_util::GetStringUTF8(IDS_WELCOME_PAGE_URL);
   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(
     const base::FilePath& cur_dir,
     const base::CommandLine& command_line,
     chrome::startup::IsFirstRun is_first_run)
     : cur_dir_(cur_dir),
       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(
     const base::FilePath& cur_dir,
     const base::CommandLine& command_line,
     StartupBrowserCreator* browser_creator,
     chrome::startup::IsFirstRun is_first_run)
     : cur_dir_(cur_dir),
       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() {
 }
 
 bool StartupBrowserCreatorImpl::Launch(Profile* profile,
                                        const std::vector<GURL>& urls_to_open,
                                        bool process_startup,
                                        chrome::HostDesktopType desktop_type) {
   DCHECK(profile);
@@ skipping 205 matching lines @@
     chrome::HostDesktopType desktop_type) {
   // If we're starting up in "background mode" (no open browser window) then
   // don't open any browser windows, unless kAutoLaunchAtStartup is also
   // specified.
   if (process_startup &&
       command_line_.HasSwitch(switches::kNoStartupWindow) &&
       !command_line_.HasSwitch(switches::kAutoLaunchAtStartup)) {
     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) {
     // See if there are apps for this profile that should be launched on startup
     // due to a switch from Metro mode.
     app_metro_launch::HandleAppLaunchForMetroRestart(profile_);
   }
 #endif
 
   if (process_startup && ProcessStartupURLs(urls_to_open, desktop_type)) {
     // ProcessStartupURLs processed the urls, nothing else to do.
     return;
   }
 
   chrome::startup::IsProcessStartup is_process_startup = process_startup ?
       chrome::startup::IS_PROCESS_STARTUP :
       chrome::startup::IS_NOT_PROCESS_STARTUP;
   if (!process_startup) {
     // Even if we're not starting a new process, this may conceptually be
     // "startup" for the user and so should be handled in a similar way.  Eg.,
     // Chrome may have been running in the background due to an app with a
     // background page being installed, or running with only an app window
     // displayed.
     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) {
         AddInfoBarsIfNecessary(browser, is_process_startup);
         return;
       }
     }
   }
 
   // Session startup didn't occur, open the urls.
@@ skipping 56 matching lines @@
 #if defined(OS_MACOSX)
     // On Mac, when restoring a session with no windows, suppress the creation
     // of a new window in the case where the system is launching Chrome via a
     // login item or Lion's resume feature.
     if (base::mac::WasLaunchedAsLoginOrResumeItem()) {
       restore_behavior = restore_behavior &
                          ~SessionRestore::ALWAYS_CREATE_TABBED_BROWSER;
     }
 #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;
   }
 
   Browser* browser = ProcessSpecifiedURLs(urls_to_open, desktop_type);
   if (!browser)
     return false;
 
   AddInfoBarsIfNecessary(browser, chrome::startup::IS_PROCESS_STARTUP);
@@ skipping 32 matching lines @@
     // If urls were specified on the command line, use them.
     UrlsToTabs(urls_to_open, &tabs);
   } else if (pref.type == SessionStartupPref::DEFAULT ||
              (is_first_run_ &&
               browser_creator_ && !browser_creator_->first_run_tabs_.empty())) {
     std::vector<GURL> urls;
     AddStartupURLs(&urls);
     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.
     AddUniqueURLs(pref.urls, &tabs);
   } else if (pref.type == SessionStartupPref::HOMEPAGE) {
     // If 'homepage' selected, either by the user or by a policy, we should
     // have migrated them to another value.
     NOTREACHED() << "SessionStartupPref has deprecated type HOMEPAGE";
   }
 
@@ skipping 44 matching lines @@
   // If we don't yet have a profile, try to use the one we're given from
   // |browser|. While we may not end up actually using |browser| (since it
   // could be a popup window), we can at least use the profile.
   if (!profile_ && browser)
     profile_ = browser->profile();
 
   if (!browser || !browser->is_type_tabbed())
     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) {
     // We skip URLs that we'd have to launch an external protocol handler for.
     // This avoids us getting into an infinite loop asking ourselves to open
     // a URL, should the handler be (incorrectly) configured to be us. Anyone
     // asking us to open such a URL should really ask the handler directly.
     bool handled_by_chrome = ProfileIOData::IsHandledURL(tabs[i].url) ||
         (registry && registry->IsHandledProtocol(tabs[i].url.scheme()));
     if (!process_startup && !handled_by_chrome)
@@ skipping 10 matching lines @@
     params.disposition = first_tab ? NEW_FOREGROUND_TAB : NEW_BACKGROUND_TAB;
     params.tabstrip_add_types = add_types;
 
 #if defined(ENABLE_RLZ) && !defined(OS_IOS)
     if (process_startup && google_util::IsGoogleHomePageUrl(tabs[i].url)) {
       params.extra_headers = RLZTracker::GetAccessPointHttpHeader(
           RLZTracker::ChromeHomePage());
     }
 #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;
   }
   if (!browser->tab_strip_model()->GetActiveWebContents()) {
     // TODO: this is a work around for 110909. Figure out why it's needed.
     if (!browser->tab_strip_model()->count())
       chrome::AddTabAt(browser, GURL(), -1, true);
     else
       browser->tab_strip_model()->ActivateTabAt(0, false);
@@ skipping 67 matching lines @@
           startup_urls->push_back(internals::GetWelcomePageURL());
         } else {
           startup_urls->push_back(*it);
         }
         ++it;
       }
       browser_creator_->first_run_tabs_.clear();
     }
   }
 
-  // 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());
   }
 
   if (signin::ShouldShowPromoAtStartup(profile_, is_first_run_)) {
     signin::DidShowPromoAtStartup(profile_);
 
     const GURL sync_promo_url = signin::GetPromoURL(
         signin_metrics::SOURCE_START_PAGE, false);
 
     // No need to add if the sync promo is already in the startup list.
@@ skipping 11 matching lines @@
       // behavior is desired because completing or skipping the sync promo
       // causes a redirect to the NTP.
       if (!startup_urls->empty() &&
           startup_urls->at(0) == GURL(chrome::kChromeUINewTabURL))
         startup_urls->at(0) = sync_promo_url;
       else
         startup_urls->insert(startup_urls->begin(), sync_promo_url);
     }
   }
 }
+
+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
+}