Welcome page changes for Windows 10 and over.

chrome/browser/ui/startup/startup_browser_creator_impl.h

 // 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.
 
 #ifndef CHROME_BROWSER_UI_STARTUP_STARTUP_BROWSER_CREATOR_IMPL_H_
 #define CHROME_BROWSER_UI_STARTUP_STARTUP_BROWSER_CREATOR_IMPL_H_
 
 #include <string>
 #include <vector>
 
@@ skipping 11 matching lines @@
 namespace base {
 class CommandLine;
 class FilePath;
 }
 
 namespace content {
 class WebContents;
 }
 
 namespace internals {
+
 GURL GetWelcomePageURL();
-}
+
+// On Windows 10 and higher, the welcome page is shown once following each OS
+// upgrade, even when not in first-run. When this is true, the welcome page

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

"even when not" suggests it *may also* happen during First Run; while the method
name "AfterFirstRun" suggests it's *only after* First Run (I still haven't read
further to figure out which is the truth, but this comment/method name could be
clearer).

(addition) Ah, now that I'm reading the impl I see what this is checking, how
about "ShowWelcomePageAfterFirstRunAllowed()"? Also I think this is perhaps not
the best place to describe the behavior (i.e. this method only checks whether
it's allowed but doesn't enforce the behavior it documents)

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

comment removed

+// precedes the NTP in the startup tabs, and the sync promo is not shown.
+bool ShowWelcomePageAfterFirstRun();

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

Inline this in StartupBrowserCreatorImpl::InitializeWelcomeRunType, and make the
tests similarly check the os/version inline. I don't see a good reason to
abstract this decision in this manner, nor explain this logic in the header.

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

Done.

+
+// Records that welcome page was shown for the current run.
+void RecordWelcomeRunComplete();
+
+}  // namespace internals
 
 // Assists launching the application and appending the initial tabs for a
 // browser window.
 class StartupBrowserCreatorImpl {
  public:
   // There are two ctors. The first one implies a NULL browser_creator object
   // and thus no access to distribution-specific first-run behaviors. The
   // second one is always called when the browser starts even if it is not
   // the first run.  |is_first_run| indicates that this is a new profile.
   StartupBrowserCreatorImpl(const base::FilePath& cur_dir,
@@ skipping 28 matching lines @@
   // browser.
   Browser* OpenTabsInBrowser(Browser* browser,
                              bool process_startup,
                              const StartupTabs& tabs,
                              chrome::HostDesktopType desktop_type);
 
  private:
   FRIEND_TEST_ALL_PREFIXES(BrowserTest, RestorePinnedTabs);
   FRIEND_TEST_ALL_PREFIXES(BrowserTest, AppIdSwitch);
 
+  enum class WelcomeRunType {
+    NONE,             // Do not inject the welcome page for this run.
+    FIRST_RUN_FIRST,  // Inject the welcome page as the first first-run tab.

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

It doesn't seem like the enum needs to convey the reason for showing the welcome
page as the first tab... Even though the *FIRST value might be set for different
reasons, nothing actually triggers distinct behavior for FIRST_RUN_FIRST vs.
ANY_RUN_FIRST, afaict... So why not just have 3 enum values:
WELCOME_TAB_[NONE|FIRST|LAST]. 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/09 18:59:38]

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?

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

That sounds plausible, but I might have to see it to understand it fully...

+    FIRST_RUN_LAST,   // Inject the welcome page as the last first-run tab.
+    ANY_RUN_FIRST,    // Inject the welcome page as the first tab.
+  };
+
   // If the process was launched with the web application command line flags,
   // e.g. --app=http://www.google.com/ or --app_id=... return true.
   // In this case |app_url| or |app_id| are populated if they're non-null.
   bool IsAppLaunch(std::string* app_url, std::string* app_id);
 
   // If IsAppLaunch is true, tries to open an application window.
   // If the app is specified to start in a tab, or IsAppLaunch is false,
   // returns false to specify default processing. |out_app_contents| is an
   // optional argument to receive the created WebContents for the app.
   bool OpenApplicationWindow(Profile* profile,
@@ skipping 42 matching lines @@
   void AddUniqueURLs(const std::vector<GURL>& urls, StartupTabs* tabs);
 
   // Adds any startup infobars to the selected tab of the given browser.
   void AddInfoBarsIfNecessary(
       Browser* browser,
       chrome::startup::IsProcessStartup is_process_startup);
 
   // Adds additional startup URLs to the specified vector.
   void AddStartupURLs(std::vector<GURL>* startup_urls) const;
 
-  // Checks whether the Preferences backup is invalid and notifies user in
-  // that case.
-  void CheckPreferencesBackup(Profile* profile);
+  // Initializes |welcome_run_type_| for this launch. For first-run, the type

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

Don't explain the nuanced impl details here, move it to the code file.

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

Done.

+  // will be LAST_TAB for all systems except for Windows 10+, where it will be
+  // FIRST_TAB. For non-first run, the type will be NONE for all systems

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

FIRST_TAB and LAST_TAB don't match the values of the WelcomeRunType enum
(assuming you changed them after writing the comment and didn't update here?)

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

Done.

+  // except for Windows 10+, where it will be FIRST_TAB if this is the first
+  // somewhat normal launch since an OS upgrade.
+  void InitializeWelcomeRunType(const std::vector<GURL>& urls_to_open);
+
+  // Records that the welcome page was shown.
+  void RecordWelcomeRunComplete() const;
 
   const base::FilePath cur_dir_;
   const base::CommandLine& command_line_;
   Profile* profile_;
   StartupBrowserCreator* browser_creator_;
   bool is_first_run_;
+  WelcomeRunType welcome_run_type_;
   DISALLOW_COPY_AND_ASSIGN(StartupBrowserCreatorImpl);
 };
 
 #endif  // CHROME_BROWSER_UI_STARTUP_STARTUP_BROWSER_CREATOR_IMPL_H_