Welcome page changes for Windows 10 and over.

chrome/browser/ui/startup/startup_browser_creator_impl.h

Index: chrome/browser/ui/startup/startup_browser_creator_impl.h
diff --git a/chrome/browser/ui/startup/startup_browser_creator_impl.h b/chrome/browser/ui/startup/startup_browser_creator_impl.h
index 8103e208e2145bb205fba476f4540029cfd4c912..1ec4758806baa560406dc873dc9a9f6262433553 100644
--- a/chrome/browser/ui/startup/startup_browser_creator_impl.h
+++ b/chrome/browser/ui/startup/startup_browser_creator_impl.h
@@ -29,8 +29,18 @@ 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.
@@ -79,6 +89,13 @@ class StartupBrowserCreatorImpl {
   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.
@@ -141,15 +158,22 @@ class StartupBrowserCreatorImpl {
   // 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);
 };