Welcome page changes for Windows 10 and over.

chrome/browser/ui/startup/startup_browser_creator_browsertest.cc

Index: chrome/browser/ui/startup/startup_browser_creator_browsertest.cc
diff --git a/chrome/browser/ui/startup/startup_browser_creator_browsertest.cc b/chrome/browser/ui/startup/startup_browser_creator_browsertest.cc
index 1e1a055a550f0f221d0f53349bfb6dc77de50fcb..6af2c799c91ee4949de5501782336a721886abcc 100644
--- a/chrome/browser/ui/startup/startup_browser_creator_browsertest.cc
+++ b/chrome/browser/ui/startup/startup_browser_creator_browsertest.cc
@@ -42,6 +42,7 @@
 #include "chrome/test/base/test_switches.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/test/test_utils.h"
 #include "extensions/browser/extension_system.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
@@ -68,6 +69,10 @@ using testing::Return;
 #include "chrome/browser/supervised_user/supervised_user_service_factory.h"
 #endif
 
+#if defined(OS_WIN)
+#include "base/win/windows_version.h"
+#endif
+
 using extensions::Extension;
 
 namespace {
@@ -87,6 +92,27 @@ Browser* FindOneOtherBrowser(Browser* browser) {
   return other_browser;
 }
 
+// Returns true if the platform supports showing the sync promo on first run.
+bool PlatformSupportsSyncPromo() {
+  // Presently, this is exactly the same set of platforms that support

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

But the condition is simple, and changes to ShowWelcomePageAfterFirstRun might
unwittingly alter this behavior too... Why not just keep the logic separate, or
nix both for something like IsWindowsVersion10OrGreater() instead (not that I
think such a helper is needed)?

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

s/that support/that don't support/ ?

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

Done.

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

Good point. I agree that it's not good to couple them together like this.
I've made the platform and version check explicit here.

+  // re-showing the welcome page outside of first run.
+  return !internals::ShowWelcomePageAfterFirstRun();
+}
+
+// Returns the index of the welcome page for normal first-run startup.
+// Ordinarily it is the second tab. It is the first tab for platforms that do
+// not show the sync promo on first run.
+int GetWelcomePageIndex() {
+  return PlatformSupportsSyncPromo() ? 1 : 0;
+}
+
+// Returns the index of the NTP/sync promo for normal first-run startup.
+// Ordinarily it is the first tab. It is the second tab for platforms that do
+// not show the sync promo on first run.
+int GetNTPIndex() {
+  return PlatformSupportsSyncPromo() ? 0 : 1;
+}
+
 }  // namespace
 
 class StartupBrowserCreatorTest : public ExtensionBrowserTest {
@@ -211,10 +237,16 @@ IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorTest,
   urls.push_back(test_server()->GetURL("files/title1.html"));
   urls.push_back(test_server()->GetURL("files/title2.html"));
 
+  Profile* profile = browser()->profile();
+  chrome::HostDesktopType host_desktop_type = browser()->host_desktop_type();
+
   // Set the startup preference to open these URLs.
   SessionStartupPref pref(SessionStartupPref::URLS);
   pref.urls = urls;
-  SessionStartupPref::SetStartupPref(browser()->profile(), pref);
+  SessionStartupPref::SetStartupPref(profile, pref);
+
+  // Keep the browser process running while browsers are closed.
+  g_browser_process->AddRefModule();

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

Why isn't this needed for StartupURLsOnNewWindow which also closes the window?

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

StartupURLsOnNewWindow closes the second browser (new_browser), but not the
initial one created by the test fixture (browser()), so that initial window is
open throughout the test and keeping the browser process alive.

 
   // Close the browser.
   browser()->window()->Close();
@@ -223,26 +255,70 @@ IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorTest,
   base::CommandLine dummy(base::CommandLine::NO_PROGRAM);
   chrome::startup::IsFirstRun first_run = first_run::IsChromeFirstRun() ?
       chrome::startup::IS_FIRST_RUN : chrome::startup::IS_NOT_FIRST_RUN;
-  StartupBrowserCreatorImpl launch(base::FilePath(), dummy, first_run);
-  ASSERT_TRUE(launch.Launch(browser()->profile(), std::vector<GURL>(), false,
-                            browser()->host_desktop_type()));
+  {
+    StartupBrowserCreatorImpl launch(base::FilePath(), dummy, first_run);
+    ASSERT_TRUE(
+        launch.Launch(profile, std::vector<GURL>(), false, host_desktop_type));
+  }
 
   // This should have created a new browser window.  |browser()| is still
   // around at this point, even though we've closed its window.
   Browser* new_browser = FindOneOtherBrowser(browser());
   ASSERT_TRUE(new_browser);
 
-  // The new browser should have one tab for each URL.
-  TabStripModel* tab_strip = new_browser->tab_strip_model();
-  ASSERT_EQ(static_cast<int>(urls.size()), tab_strip->count());
-  for (size_t i=0; i < urls.size(); i++) {
-    EXPECT_EQ(urls[i], tab_strip->GetWebContentsAt(i)->GetURL());
+  if (internals::ShowWelcomePageAfterFirstRun()) {
+    // The new browser should have the welcome tab and one tab for each URL.
+    TabStripModel* tab_strip = new_browser->tab_strip_model();
+    ASSERT_EQ(static_cast<int>(urls.size()) + 1, tab_strip->count());
+    EXPECT_EQ(internals::GetWelcomePageURL(),
+              tab_strip->GetWebContentsAt(0)->GetURL());
+    for (size_t i = 0; i < urls.size(); i++) {
+      EXPECT_EQ(urls[i], tab_strip->GetWebContentsAt(i + 1)->GetURL());
+    }
+  } else {
+    // The new browser should have one tab for each URL.
+    TabStripModel* tab_strip = new_browser->tab_strip_model();
+    ASSERT_EQ(static_cast<int>(urls.size()), tab_strip->count());
+    for (size_t i = 0; i < urls.size(); i++) {
+      EXPECT_EQ(urls[i], tab_strip->GetWebContentsAt(i)->GetURL());
+    }
+
+    // The two tabs, despite having the same site, should be in different
+    // SiteInstances.
+    EXPECT_NE(tab_strip->GetWebContentsAt(0)->GetSiteInstance(),
+              tab_strip->GetWebContentsAt(1)->GetSiteInstance());
+  }
+
+  // The test is complete if the welcome page wasn't included above.
+  if (!internals::ShowWelcomePageAfterFirstRun()) {
+    g_browser_process->ReleaseModule();

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

(orthogonal: Wouldn't it be nice to have scoped objects for these refcounts,
eh...)

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

Acknowledged.

+    return;
+  }
+
+  // Close the browser opened above.
+  {
+    content::WindowedNotificationObserver observer(
+        chrome::NOTIFICATION_BROWSER_CLOSED,
+        content::Source<Browser>(new_browser));
+    new_browser->window()->Close();
+    observer.Wait();
+  }
+
+  // Try again to ensure that the welcome page isn't shown the second time.
+  {
+    StartupBrowserCreatorImpl launch(base::FilePath(), dummy, first_run);
+    ASSERT_TRUE(
+        launch.Launch(profile, std::vector<GURL>(), false, host_desktop_type));
   }
 
-  // The two tabs, despite having the same site, should be in different
-  // SiteInstances.
-  EXPECT_NE(tab_strip->GetWebContentsAt(0)->GetSiteInstance(),
-            tab_strip->GetWebContentsAt(1)->GetSiteInstance());
+  // Find the new browser and ensure that it has only the expected URLs this
+  // time.
+  new_browser = FindTabbedBrowser(profile, true, host_desktop_type);

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

The initial assignment above uses FindOneOtherBrowser(), I don't care which is
used, but we should be consistent (or comment on why it differs if it truly
does).

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

Comment added.

+  ASSERT_TRUE(new_browser);
+  ASSERT_EQ(static_cast<int>(urls.size()),
+            new_browser->tab_strip_model()->count());
+
+  g_browser_process->ReleaseModule();
 }
 
 // Verify that startup URLs aren't used when the process already exists
@@ -268,15 +344,47 @@ IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorTest,
   base::CommandLine dummy(base::CommandLine::NO_PROGRAM);
   chrome::startup::IsFirstRun first_run = first_run::IsChromeFirstRun() ?
       chrome::startup::IS_FIRST_RUN : chrome::startup::IS_NOT_FIRST_RUN;
-  StartupBrowserCreatorImpl launch(base::FilePath(), dummy, first_run);
-  ASSERT_TRUE(launch.Launch(browser()->profile(), std::vector<GURL>(), false,
-                            browser()->host_desktop_type()));
+  {
+    StartupBrowserCreatorImpl launch(base::FilePath(), dummy, first_run);
+    ASSERT_TRUE(launch.Launch(browser()->profile(), std::vector<GURL>(), false,
+                              browser()->host_desktop_type()));
+  }
 
   // This should have created a new browser window.
   Browser* new_browser = FindOneOtherBrowser(browser());
   ASSERT_TRUE(new_browser);
 
-  // The new browser should have exactly one tab (not the startup URLs).
+  if (internals::ShowWelcomePageAfterFirstRun()) {
+    // The new browser should have two tabs (not the startup URLs).
+    ASSERT_EQ(2, new_browser->tab_strip_model()->count());
+  } else {
+    // The new browser should have exactly one tab (not the startup URLs).
+    ASSERT_EQ(1, new_browser->tab_strip_model()->count());
+  }
+
+  // The test is complete if the welcome page wasn't included above.
+  if (!internals::ShowWelcomePageAfterFirstRun())
+    return;
+
+  // Close the browser opened above.
+  {
+    content::WindowedNotificationObserver observer(
+        chrome::NOTIFICATION_BROWSER_CLOSED,
+        content::Source<Browser>(new_browser));
+    new_browser->window()->Close();
+    observer.Wait();
+  }
+
+  // Try again to ensure that the welcome page isn't shown the second time.
+  {
+    StartupBrowserCreatorImpl launch(base::FilePath(), dummy, first_run);
+    ASSERT_TRUE(launch.Launch(browser()->profile(), std::vector<GURL>(), false,
+                              browser()->host_desktop_type()));
+  }
+
+  // Find the new browser and ensure that it has only the one tab this time.
+  new_browser = FindOneOtherBrowser(browser());
+  ASSERT_TRUE(new_browser);
   ASSERT_EQ(1, new_browser->tab_strip_model()->count());
 }
 
@@ -484,12 +592,19 @@ IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorTest, SyncPromoNoWelcomePage) {
   ASSERT_TRUE(new_browser);
 
   TabStripModel* tab_strip = new_browser->tab_strip_model();
-  EXPECT_EQ(1, tab_strip->count());
 
   if (signin::ShouldShowPromoAtStartup(browser()->profile(), true)) {
+    ASSERT_EQ(1, tab_strip->count());
     EXPECT_EQ(signin::GetPromoURL(signin_metrics::SOURCE_START_PAGE, false),
               tab_strip->GetWebContentsAt(0)->GetURL());
+  } else if (internals::ShowWelcomePageAfterFirstRun()) {
+    ASSERT_EQ(2, tab_strip->count());
+    EXPECT_EQ(GURL(internals::GetWelcomePageURL()),
+              tab_strip->GetWebContentsAt(0)->GetURL());
+    EXPECT_EQ(GURL(chrome::kChromeUINewTabURL),
+              tab_strip->GetWebContentsAt(1)->GetURL());
   } else {
+    ASSERT_EQ(1, tab_strip->count());
     EXPECT_EQ(GURL(chrome::kChromeUINewTabURL),
               tab_strip->GetWebContentsAt(0)->GetURL());
   }
@@ -514,13 +629,13 @@ IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorTest, SyncPromoWithWelcomePage) {
 
   if (signin::ShouldShowPromoAtStartup(browser()->profile(), true)) {
     EXPECT_EQ(signin::GetPromoURL(signin_metrics::SOURCE_START_PAGE, false),
-              tab_strip->GetWebContentsAt(0)->GetURL());
+              tab_strip->GetWebContentsAt(GetNTPIndex())->GetURL());
   } else {
     EXPECT_EQ(GURL(chrome::kChromeUINewTabURL),
-              tab_strip->GetWebContentsAt(0)->GetURL());
+              tab_strip->GetWebContentsAt(GetNTPIndex())->GetURL());
   }
   EXPECT_EQ(internals::GetWelcomePageURL(),
-            tab_strip->GetWebContentsAt(1)->GetURL());
+            tab_strip->GetWebContentsAt(GetWelcomePageIndex())->GetURL());
 }
 
 IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorTest, SyncPromoWithFirstRunTabs) {
@@ -654,8 +769,17 @@ IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorTest, StartupURLsForTwoProfiles) {
   new_browser = FindOneOtherBrowserForProfile(default_profile, browser());
   ASSERT_TRUE(new_browser);
   TabStripModel* tab_strip = new_browser->tab_strip_model();
-  ASSERT_EQ(1, tab_strip->count());
-  EXPECT_EQ(urls1[0], tab_strip->GetWebContentsAt(0)->GetURL());
+  ASSERT_LT(0, tab_strip->count());

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

nit: not necessary with the checks in each case below.

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

Constant on RHS for inequalities (also >= 1 makes more sense here IMO).

But actually I think you should get rid of this line as it's already covered
more strictly be the ASSERT_EQ's below.

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

Done.
Done.

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

tab_strip->GetWebContentsAt(0) on the next line will cause the test to crash if
the tab strip is empty. I thought it would be better for the test to fail with
the ASSERT here rather than crash outright. I've rejiggered things now so that
it's no longer necessary.

+  content::WebContents* web_contents = tab_strip->GetWebContentsAt(0);
+  if (internals::ShowWelcomePageAfterFirstRun()) {

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

Add a comment here like:

// The first profile should get the welcome page on supported platforms, but the
second profile should not get a second instance of it.

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

Done.

+    ASSERT_EQ(2, tab_strip->count());
+    EXPECT_EQ(GURL(internals::GetWelcomePageURL()), web_contents->GetURL());
+    web_contents = tab_strip->GetWebContentsAt(1);

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

When reading this I first got confused thinking this was the Welcome Page's
index and wondering why it wasn't first...

I think this would be more readable if each WebContents* had its own explicitly
named variable then re-using a single one declared outside this scope

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

Done.

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

i went ahead and removed these web_contents variables altogether. i think it's
even more clear (and consistent) to have GetWebContentsAt() in the test
expectation.

+    EXPECT_EQ(urls1[0], web_contents->GetURL());
+  } else {
+    ASSERT_EQ(1, tab_strip->count());
+    EXPECT_EQ(urls1[0], web_contents->GetURL());
+  }
 
   ASSERT_EQ(1u, chrome::GetBrowserCount(other_profile,
                                         browser()->host_desktop_type()));
@@ -874,14 +998,23 @@ IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorTest,
 
   Browser* new_browser = NULL;
   // The last open profile (the profile_home1 in this case) will always be
-  // launched, even if it will open just the home page.
+  // launched, even if it will open just the home page (and the welcome page on

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

s/home page/new tab page/

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

Done.

+  // relevant platforms).
   ASSERT_EQ(1u, chrome::GetBrowserCount(profile_home1, original_desktop_type));
   new_browser = FindOneOtherBrowserForProfile(profile_home1, NULL);
   ASSERT_TRUE(new_browser);
   TabStripModel* tab_strip = new_browser->tab_strip_model();
-  ASSERT_EQ(1, tab_strip->count());
-  EXPECT_EQ(GURL(chrome::kChromeUINewTabURL),
-            tab_strip->GetWebContentsAt(0)->GetURL());
+  ASSERT_LT(0, tab_strip->count());

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

Redundant.

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

removed

+  content::WebContents* web_contents = tab_strip->GetWebContentsAt(0);
+  if (internals::ShowWelcomePageAfterFirstRun()) {
+    ASSERT_EQ(2, tab_strip->count());
+    EXPECT_EQ(GURL(internals::GetWelcomePageURL()), web_contents->GetURL());
+    web_contents = tab_strip->GetWebContentsAt(1);

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

Same comment as above, suggest using explicit variable names.

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

Done.

+    EXPECT_EQ(GURL(chrome::kChromeUINewTabURL), web_contents->GetURL());
+  } else {
+    ASSERT_EQ(1, tab_strip->count());
+    EXPECT_EQ(GURL(chrome::kChromeUINewTabURL), web_contents->GetURL());
+  }
 
   // profile_urls opened the urls.
   ASSERT_EQ(1u, chrome::GetBrowserCount(profile_urls, original_desktop_type));
@@ -987,15 +1120,24 @@ IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorTest, ProfilesLaunchedAfterCrash) {
   EXPECT_FALSE(SessionRestore::IsRestoring(profile_urls));
 
   // The profile which normally opens the home page displays the new tab page.
+  // The welcome page is also shown for relevant platforms.
   Browser* new_browser = NULL;
   ASSERT_EQ(1u, chrome::GetBrowserCount(profile_home,
                                         browser()->host_desktop_type()));
   new_browser = FindOneOtherBrowserForProfile(profile_home, NULL);
   ASSERT_TRUE(new_browser);
   TabStripModel* tab_strip = new_browser->tab_strip_model();
-  ASSERT_EQ(1, tab_strip->count());
+  ASSERT_LT(0, tab_strip->count());

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

Redundant.

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

removed

   content::WebContents* web_contents = tab_strip->GetWebContentsAt(0);
-  EXPECT_EQ(GURL(chrome::kChromeUINewTabURL), web_contents->GetURL());
+  if (internals::ShowWelcomePageAfterFirstRun()) {
+    ASSERT_EQ(2, tab_strip->count());
+    EXPECT_EQ(GURL(internals::GetWelcomePageURL()), web_contents->GetURL());
+    web_contents = tab_strip->GetWebContentsAt(1);

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

Explicit names.

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

Done.

+    EXPECT_EQ(GURL(chrome::kChromeUINewTabURL), web_contents->GetURL());
+  } else {
+    ASSERT_EQ(1, tab_strip->count());
+    EXPECT_EQ(GURL(chrome::kChromeUINewTabURL), web_contents->GetURL());
+  }
   EnsureRestoreUIWasShown(web_contents);
 
   // The profile which normally opens last open pages displays the new tab page.
@@ -1139,9 +1281,9 @@ IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorFirstRunTest,
   TabStripModel* tab_strip = new_browser->tab_strip_model();
   ASSERT_EQ(2, tab_strip->count());
   EXPECT_EQ(GURL(chrome::kChromeUINewTabURL),
-            tab_strip->GetWebContentsAt(0)->GetURL());
+            tab_strip->GetWebContentsAt(GetNTPIndex())->GetURL());
   EXPECT_EQ(internals::GetWelcomePageURL(),
-            tab_strip->GetWebContentsAt(1)->GetURL());
+            tab_strip->GetWebContentsAt(GetWelcomePageIndex())->GetURL());
 }
 
 #if defined(GOOGLE_CHROME_BUILD) && defined(OS_MACOSX)
@@ -1152,6 +1294,8 @@ IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorFirstRunTest,
 #endif
 IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorFirstRunTest,
                        MAYBE_SyncPromoAllowed) {
+  if (!PlatformSupportsSyncPromo())
+    return;
   // Consistently enable the welcome page on all platforms.
   first_run::SetShouldShowWelcomePage();
 
@@ -1193,6 +1337,8 @@ IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorFirstRunTest,
 #endif
 IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorFirstRunTest,
                        MAYBE_FirstRunTabsPromoAllowed) {
+  if (!PlatformSupportsSyncPromo())
+    return;
   // Simulate the following master_preferences:
   // {
   //  "first_run_tabs" : [
@@ -1236,6 +1382,8 @@ IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorFirstRunTest,
 #endif
 IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorFirstRunTest,
                        MAYBE_FirstRunTabsContainSyncPromo) {
+  if (!PlatformSupportsSyncPromo())
+    return;
   // Simulate the following master_preferences:
   // {
   //  "first_run_tabs" : [
@@ -1285,6 +1433,8 @@ IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorFirstRunTest,
 #endif
 IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorFirstRunTest,
                        MAYBE_FirstRunTabsContainNTPSyncPromoAllowed) {
+  if (!PlatformSupportsSyncPromo())
+    return;
   // Simulate the following master_preferences:
   // {
   //  "first_run_tabs" : [
@@ -1332,6 +1482,8 @@ IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorFirstRunTest,
 #endif
 IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorFirstRunTest,
                        MAYBE_FirstRunTabsContainNTPSyncPromoForbidden) {
+  if (!PlatformSupportsSyncPromo())
+    return;
   // Simulate the following master_preferences:
   // {
   //  "first_run_tabs" : [
@@ -1378,6 +1530,8 @@ IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorFirstRunTest,
 #endif
 IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorFirstRunTest,
                        MAYBE_FirstRunTabsSyncPromoForbidden) {
+  if (!PlatformSupportsSyncPromo())
+    return;
   // Simulate the following master_preferences:
   // {
   //  "first_run_tabs" : [
@@ -1421,6 +1575,8 @@ IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorFirstRunTest,
 #endif
 IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorFirstRunTest,
                        MAYBE_RestoreOnStartupURLsPolicySpecified) {
+  if (!PlatformSupportsSyncPromo())
+    return;
   // Simulate the following master_preferences:
   // {
   //  "sync_promo": {