Completing refactor of startup_browser_creator_impl

chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc

Index: chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc
diff --git a/chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc b/chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc
index 3b456b5972dd3f80eb27d3a750dc7242c9609159..9dfe9a36c09a36339cb9a10476981dc84cd824be 100644
--- a/chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc
+++ b/chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc
@@ -17,6 +17,7 @@ constexpr uint32_t kDistributionFirstRunTabs = 0x02;
 constexpr uint32_t kResetTriggerTabs = 0x04;
 constexpr uint32_t kPinnedTabs = 0x08;
 constexpr uint32_t kPreferencesTabs = 0x10;
+constexpr uint32_t kNewTabPageTabs = 0x20;

[Peter Kasting, 2016/10/31 19:24:06]

Nit: Might be good to make it clearer these are successive bits via formatting:

constexpr uint32_t kOnboardingTabs           = 1 << 0;
constexpr uint32_t kDistributionFirstRunTabs = 1 << 1;
...

[tmartino, 2016/11/02 16:34:00]

Done

 
 class FakeStartupTabProvider : public StartupTabProvider {
  public:
@@ -46,7 +47,8 @@ class FakeStartupTabProvider : public StartupTabProvider {
     return tabs;
   }
 
-  StartupTabs GetPinnedTabs(Profile* profile) const override {
+  StartupTabs GetPinnedTabs(const base::CommandLine& command_line_,
+                            Profile* profile) const override {
     StartupTabs tabs;
     if (options_ & kPinnedTabs)
       tabs.emplace_back(GURL("https://pinned"), true);
@@ -61,6 +63,14 @@ class FakeStartupTabProvider : public StartupTabProvider {
     return tabs;
   }
 
+  StartupTabs GetNewTabPageTabs(const base::CommandLine& command_line_,
+                                Profile* profile) const override {
+    StartupTabs tabs;
+    if (options_ & kNewTabPageTabs)
+      tabs.emplace_back(GURL("https://new-tab"), false);
+    return tabs;
+  }
+
  private:
   const uint32_t options_;
 };
@@ -72,7 +82,8 @@ class FakeStartupTabProvider : public StartupTabProvider {
 // command line. Reset trigger always appears first.
 TEST(StartupBrowserCreatorImplTest, DetermineStartupTabs) {
   FakeStartupTabProvider provider(kOnboardingTabs | kResetTriggerTabs |
-                                  kPinnedTabs | kPreferencesTabs);
+                                  kPinnedTabs | kPreferencesTabs |
+                                  kNewTabPageTabs);
   StartupBrowserCreatorImpl impl(
       base::FilePath(), base::CommandLine(base::CommandLine::NO_PROGRAM),
       chrome::startup::IS_FIRST_RUN);
@@ -87,11 +98,13 @@ TEST(StartupBrowserCreatorImplTest, DetermineStartupTabs) {
 }
 
 // All content is blocked in Incognito mode, or when recovering from a crash.
-// Only the New Tab Page should appear in either case.
+// Only the New Tab Page should appear in either case. Test logic checks for the
+// actual NTP URL, rather than the sentinel returned by the fake, because the
+// Provider is ignored entirely when short-circuited by incognito/crash.

[Peter Kasting, 2016/10/31 19:24:06]

Nit: Perhaps this comment (reworded slightly as "Check for...") belongs e.g.
just above line 116.  It seems like an explanation of an implementation detail,
rather than an explanation of the overall purpose of the test.

[tmartino, 2016/11/02 16:34:00]

Done

 TEST(StartupBrowserCreatorImplTest, DetermineStartupTabs_IncognitoOrCrash) {
   FakeStartupTabProvider provider(kOnboardingTabs | kDistributionFirstRunTabs |
                                   kResetTriggerTabs | kPinnedTabs |
-                                  kPreferencesTabs);
+                                  kPreferencesTabs | kNewTabPageTabs);
   StartupBrowserCreatorImpl impl(
       base::FilePath(), base::CommandLine(base::CommandLine::NO_PROGRAM),
       chrome::startup::IS_FIRST_RUN);
@@ -113,7 +126,7 @@ TEST(StartupBrowserCreatorImplTest, DetermineStartupTabs_IncognitoOrCrash) {
 TEST(StartupBrowserCreatorImplTest, DetermineStartupTabs_MasterPrefs) {
   FakeStartupTabProvider provider(kOnboardingTabs | kDistributionFirstRunTabs |
                                   kResetTriggerTabs | kPinnedTabs |
-                                  kPreferencesTabs);
+                                  kPreferencesTabs | kNewTabPageTabs);
   StartupBrowserCreatorImpl impl(
       base::FilePath(), base::CommandLine(base::CommandLine::NO_PROGRAM),
       chrome::startup::IS_FIRST_RUN);
@@ -129,7 +142,7 @@ TEST(StartupBrowserCreatorImplTest, DetermineStartupTabs_MasterPrefs) {
 TEST(StartupBrowserCreatorImplTest, DetermineStartupTabs_CommandLine) {
   FakeStartupTabProvider provider(kOnboardingTabs | kDistributionFirstRunTabs |
                                   kResetTriggerTabs | kPinnedTabs |
-                                  kPreferencesTabs);
+                                  kPreferencesTabs | kNewTabPageTabs);
   StartupBrowserCreatorImpl impl(
       base::FilePath(), base::CommandLine(base::CommandLine::NO_PROGRAM),
       chrome::startup::IS_FIRST_RUN);
@@ -159,7 +172,8 @@ TEST(StartupBrowserCreatorImplTest, DetermineStartupTabs_CommandLine) {
 // New Tab Page should appear alongside pinned tabs and the reset trigger, but
 // should be superseded by onboarding tabs and by tabs specified in preferences.
 TEST(StartupBrowserCreatorImplTest, DetermineStartupTabs_NewTabPage) {
-  FakeStartupTabProvider provider_allows_ntp(kPinnedTabs | kResetTriggerTabs);
+  FakeStartupTabProvider provider_allows_ntp(kPinnedTabs | kResetTriggerTabs |
+                                             kNewTabPageTabs);
   StartupBrowserCreatorImpl impl(
       base::FilePath(), base::CommandLine(base::CommandLine::NO_PROGRAM),
       chrome::startup::IS_FIRST_RUN);
@@ -168,6 +182,6 @@ TEST(StartupBrowserCreatorImplTest, DetermineStartupTabs_NewTabPage) {
                                                  StartupTabs(), false, false);
   ASSERT_EQ(3U, output.size());
   EXPECT_EQ("reset-trigger", output[0].url.host());
-  EXPECT_EQ(GURL(chrome::kChromeUINewTabURL), output[1].url);
+  EXPECT_EQ("new-tab", output[1].url.host());
   EXPECT_EQ("pinned", output[2].url.host());
 }