Refactoring startup logic for upcoming FRE changes (non-Win 10).

chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc

+// Copyright 2016 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 "base/command_line.h"
+#include "chrome/browser/ui/startup/startup_tab_provider.h"
+#include "chrome/common/url_constants.cc"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+// Bits for FakeStartupTabProvider options.
+constexpr uint32_t kOnboardingTabs = 0x01;
+constexpr uint32_t kDistributionFirstRunTabs = 0x02;
+constexpr uint32_t kResetTriggerTabs = 0x04;
+constexpr uint32_t kPinnedTabs = 0x08;
+constexpr uint32_t kPreferencesTabs = 0x10;
+
+class FakeStartupTabProvider : public StartupTabProvider {
+ public:
+  // For each option passed, the corresponding adder below will add a sentinel
+  // tab and return true. For options not passed, the adder will return false.
+  explicit FakeStartupTabProvider(uint32_t options) : options_(options) {}

[Peter Kasting, 2016/09/29 07:23:34]

Nit: The options bitfield feels a little clever; would separate bools, maybe
with /* onboarding tabs */ type comments at the constructor callsites, be any
simpler?

[tmartino, 2016/09/30 20:55:01]

Actually, I had it that way originally, back in patchset 9 :)

I was reluctant to change it because I generally find bitmasks to be a bit
overkill. But after trying it and comparing the two, I did think it was an
improvement. I'd rather sacrifice some gross code in the body of the fake, in
exchange for cleaner and more expressive code when we're specifying behavior in
the tests.

+
+  bool AddOnboardingTabs(StartupTabs* tabs) const override {
+    if (options_ & kOnboardingTabs) {

[Peter Kasting, 2016/09/29 07:23:34]

Nit: All these methods can drop the {} if you reverse the conditionals (because
the bodies will be one line).

[tmartino, 2016/09/30 20:55:02]

Ack. When it's not too much of a hassle to structure it this way, I prefer to
use positive conditionals (if (foo) instead of if (!foo)) for readability's
sake--it's something I learned working in Java (where readability reviewers
insist on it) and I grew to really appreciate it. It reduces the need to think
about double negatives ("ok, so if we reach the else case, that means not-foo
was false..."). I particularly prefer it when the conditions are more complex
than a simple bool (like the bitmask here). Personally I think that
consideration outweighs bracket-hunting here, but I'm still trying to learn
expectations and priorities for Chromium and C++ readers.

[Peter Kasting, 2016/09/30 21:50:03]

Definitely avoid negative conditionals if there's ever an "else". 
Double-negatives are bad.  Always lead with the positive there.

When there is no else, I think there's no real advantage to having a positive
versus negative conditional.  Some reviewers are biased towards or against early
returns, or expressing conditionals in terms of "likelihood"/"which case is the
error case", but I find these to not be helpful.  My rule of thumb is to
minimize LOC, use of symbols, and net amount of indentation.  So I suggested
reversing these conditionals on those bases:

* There is no "else", so either way is readable
* Reversed condition is one fewer line
* Reversed condition avoids the noise of one set of {}
* Reversed condition indents one fewer line

To me, that makes the reversed condition a small win.

There is nowhere near enough win in such a change to justify the length of
message I just wrote, but I figured if you said you're feeling your way out
here, some possible consideration factors might be useful :)

[tmartino, 2016/10/03 23:26:06]

No longer applies anyway :)

+      tabs->push_back(StartupTab(GURL("https://onboarding"), false));

[Peter Kasting, 2016/09/29 07:23:34]

Nit: Comment in startup_browser_creator_impl.cc about emplace_back() also
applicable a number of places in this file

+      return true;
+    }
+    return false;
+  }
+
+  bool AddDistributionFirstRunTabs(StartupBrowserCreator* browser_creator,
+                                   StartupTabs* tabs) const override {
+    if (options_ & kDistributionFirstRunTabs) {
+      tabs->push_back(StartupTab(GURL("https://distribution"), false));
+      return true;
+    }
+    return false;
+  }
+
+  bool AddResetTriggerTabs(Profile* profile, StartupTabs* tabs) const override {
+    if (options_ & kResetTriggerTabs) {
+      tabs->push_back(StartupTab(GURL("https://reset-trigger"), false));
+      return true;
+    }
+    return false;
+  }
+
+  bool AddPinnedTabs(StartupTabs* tabs) const override {
+    if (options_ & kPinnedTabs) {
+      tabs->push_back(StartupTab(GURL("https://pinned"), true));
+      return true;
+    }
+    return false;
+  }
+
+  bool AddPreferencesTabs(StartupTabs* tabs) const override {
+    if (options_ & kPreferencesTabs) {
+      tabs->push_back(StartupTab(GURL("https://prefs"), false));
+      return true;
+    }
+    return false;
+  }
+
+ private:
+  const uint32_t options_;
+};
+
+StartupTabs GetSentinelCommandLineTabs() {
+  StartupTabs cmd_line_tabs;
+  cmd_line_tabs.push_back(StartupTab(GURL("https://cmd-line"), false));

[Peter Kasting, 2016/09/29 07:23:34]

Nit: In this case it seems like you could just do

  StartupTabs cmd_line_tabs = {...};

(Or even return some thing directly as you did in
startup_browser_creator_impl.cc)

But since this is only called once, why not just inline that into the one
callsite?

+  return cmd_line_tabs;
+}
+
+}  // namespace
+
+// "Standard" case: Tabs specified in onboarding, reset trigger, pinned tabs, or
+// preferences shouldn't interfere with each other. Nothing specified on the
+// command line. Reset trigger always appears first.
+TEST(StartupBrowserCreatorImplTest, DetermineStartupTabs) {
+  FakeStartupTabProvider provider(kOnboardingTabs | kResetTriggerTabs |
+                                  kPinnedTabs | kPreferencesTabs);
+  StartupBrowserCreatorImpl impl(
+      base::FilePath(), base::CommandLine(base::CommandLine::NO_PROGRAM),
+      chrome::startup::IS_FIRST_RUN);
+  StartupTabs no_tabs;

[Peter Kasting, 2016/09/29 07:23:34]

Nit: Personally, rather than declare this, I'd call "StartupTabs()" inline in
the function call below; that reads more clearly to me because it's clearly not
an outparam in that case.  (Several places)

+
+  StartupTabs output =
+      impl.DetermineStartupTabs(provider, no_tabs, false, false);
+  ASSERT_EQ(4U, output.size());
+  EXPECT_EQ("reset-trigger", output[0].url.host());
+  EXPECT_EQ("onboarding", output[1].url.host());
+  EXPECT_EQ("prefs", output[2].url.host());
+  EXPECT_EQ("pinned", output[3].url.host());
+}
+
+// All content is blocked in Incognito mode, or when recovering from a crash.
+// Only the New Tab Page should appear in either case.
+TEST(StartupBrowserCreatorImplTest, DetermineStartupTabs_IncognitoOrCrash) {
+  FakeStartupTabProvider provider(kOnboardingTabs | kDistributionFirstRunTabs |
+                                  kResetTriggerTabs | kPinnedTabs |
+                                  kPreferencesTabs);
+  StartupBrowserCreatorImpl impl(
+      base::FilePath(), base::CommandLine(base::CommandLine::NO_PROGRAM),
+      chrome::startup::IS_FIRST_RUN);
+  StartupTabs no_tabs;
+
+  // Incognito

[Peter Kasting, 2016/09/29 07:23:34]

Nit: Trailing period (several places)

[tmartino, 2016/09/30 20:55:01]

Fragment, so went with colon instead.

+  StartupTabs output =
+      impl.DetermineStartupTabs(provider, no_tabs, true, false);
+  ASSERT_EQ(1U, output.size());
+  EXPECT_EQ(GURL(chrome::kChromeUINewTabURL).host(), output[0].url.host());

[Peter Kasting, 2016/09/29 07:23:34]

Nit: Maybe omit .host() calls? (3 different places)

+
+  // Crash Recovery
+  output = impl.DetermineStartupTabs(provider, no_tabs, false, true);
+  ASSERT_EQ(1U, output.size());
+  EXPECT_EQ(GURL(chrome::kChromeUINewTabURL).host(), output[0].url.host());
+}
+
+// If Master Preferences specifies content, this should block all other
+// policies. The only exception is command line URLs, tested below.
+TEST(StartupBrowserCreatorImplTest, DetermineStartupTabs_MasterPrefs) {
+  FakeStartupTabProvider provider(kOnboardingTabs | kDistributionFirstRunTabs |
+                                  kResetTriggerTabs | kPinnedTabs |
+                                  kPreferencesTabs);
+  StartupBrowserCreatorImpl impl(
+      base::FilePath(), base::CommandLine(base::CommandLine::NO_PROGRAM),
+      chrome::startup::IS_FIRST_RUN);
+  StartupTabs no_tabs;
+
+  StartupTabs output =
+      impl.DetermineStartupTabs(provider, no_tabs, false, false);
+  ASSERT_EQ(1U, output.size());
+  EXPECT_EQ("distribution", output[0].url.host());
+}
+
+// URLs specified on the command line should always appear, and should block
+// all other tabs except the Reset Trigger tab.
+TEST(StartupBrowserCreatorImplTest, DetermineStartupTabs_CommandLine) {
+  FakeStartupTabProvider provider(kOnboardingTabs | kDistributionFirstRunTabs |
+                                  kResetTriggerTabs | kPinnedTabs |
+                                  kPreferencesTabs);
+  StartupBrowserCreatorImpl impl(
+      base::FilePath(), base::CommandLine(base::CommandLine::NO_PROGRAM),
+      chrome::startup::IS_FIRST_RUN);
+
+  StartupTabs cmd_line_tabs = GetSentinelCommandLineTabs();
+
+  StartupTabs output =
+      impl.DetermineStartupTabs(provider, cmd_line_tabs, false, false);
+  ASSERT_EQ(2U, output.size());
+  EXPECT_EQ("reset-trigger", output[0].url.host());
+  EXPECT_EQ("cmd-line", output[1].url.host());
+
+  // Also test that both incognito and crash recovery don't interfere with
+  // command line tabs.
+
+  // Incognito
+  output = impl.DetermineStartupTabs(provider, cmd_line_tabs, true, false);
+  ASSERT_EQ(1U, output.size());
+  EXPECT_EQ("cmd-line", output[0].url.host());
+
+  // Crash Recovery
+  output = impl.DetermineStartupTabs(provider, cmd_line_tabs, false, true);
+  ASSERT_EQ(1U, output.size());
+  EXPECT_EQ("cmd-line", output[0].url.host());
+}
+
+// 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);
+  StartupBrowserCreatorImpl impl(
+      base::FilePath(), base::CommandLine(base::CommandLine::NO_PROGRAM),
+      chrome::startup::IS_FIRST_RUN);
+  StartupTabs no_tabs;
+
+  StartupTabs output =
+      impl.DetermineStartupTabs(provider_allows_ntp, no_tabs, false, false);
+  ASSERT_EQ(3U, output.size());
+  EXPECT_EQ("reset-trigger", output[0].url.host());
+  EXPECT_EQ(GURL(chrome::kChromeUINewTabURL).host(), output[1].url.host());
+  EXPECT_EQ("pinned", output[2].url.host());
+}