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 186 matching lines @@
 }  // 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);
+
 // 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> 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) {
         AddInfoBarsIfNecessary(browser, is_process_startup);
         return;
       }
     }
   }
 
   // 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,
                                         chrome::HOST_DESKTOP_TYPE_NATIVE);
   }
   // 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);
 }
 
 bool StartupBrowserCreatorImpl::ProcessStartupURLs(
     const std::vector<GURL>& urls_to_open,
     chrome::HostDesktopType desktop_type) {
   VLOG(1) << "StartupBrowserCreatorImpl::ProcessStartupURLs";
   SessionStartupPref pref =
       StartupBrowserCreator::GetSessionStartupPref(command_line_, profile_);
@@ skipping 33 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> 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;
   }
 
   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)

[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.
     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 161 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
+  // 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());
   }
 
   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);
     }
   }
 }
+
+// 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
+}