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

chrome/browser/ui/startup/startup_tab_provider.h

+// 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.
+
+#ifndef CHROME_BROWSER_UI_STARTUP_STARTUP_TAB_PROVIDER_H_
+#define CHROME_BROWSER_UI_STARTUP_STARTUP_TAB_PROVIDER_H_
+
+#include <vector>
+
+#include "base/gtest_prod_util.h"
+#include "chrome/browser/first_run/first_run.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/startup/startup_browser_creator.h"
+#include "chrome/browser/ui/startup/startup_tab.h"
+#include "url/gurl.h"
+
+// Provides the sets of tabs to be shown at startup for given sets of policy.
+// For instance, this class answers the question, "which tabs, if any, need to
+// be shown for first run/onboarding?"

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

Nit: May want to note why this class is pure virtual and not just implemented
here directly.

+class StartupTabProvider {
+ public:
+  StartupTabProvider() = default;
+  virtual ~StartupTabProvider() = default;

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

Nit: Not sure if we have a policy on how to handle pure abstract classes like
this, but I might avoid adding these, or else go ahead and explicitly
DISALLOW_COPY... on this class as well.

+
+  // Determines which tabs which should be shown according to onboarding/first
+  // run policy, and appends them to |tabs|. Returns true iff any tabs were
+  // added.
+  virtual bool AddOnboardingTabs(StartupTabs* tabs) const = 0;

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

Nit: I feel like this class' API might be clearer if all these functions were
more like:

virtual StartupTabs GetOnboardingTabs() const = 0;

This allows callers to call these in any order and append the results together
using their own logic, instead of forcing them to order things carefully so the
resulting vector comes out right.  It will simplify the implementations and test
code slightly and possibly also give you a little more freedom to address my
question in startup_browser_creator_impl.cc about reordering code to avoid
duplication.

It also just feels more right to me in general.  We shouldn't need to tell the
caller whether we added tabs, that information is already available just by
looking at the vector.  But it's more annoying for callers to check the vector
size before and after the call than to do something like:

  StartupTabs onboarding_tabs = GetOnboardingTabs();
  if (!onboarding_tabs.empty()) ...

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

I actually had implemented it that way several revisions ago, following the same
logic: nicer in tests, simpler at the level where it's being called. I think I'm
still open to being swayed either way, but here are the arguments against:

1. First and foremost, grt was concerned about the amount of stack churn and
copying this introduced, and suggested writing to an out-param instead.

2. The bool return more directly expresses what callers are actually interested
in. The metapolicy follows the form (e.g.) "if we show onboarding content, we
shouldn't show the NTP" and the resulting code mirrors that relationship
exactly:
  if (!onboarding_added)
    tabs.push_back(NTP);

  whereas the old spaghetti code was full of yucky things like:
  if (tabs.empty()) // Not showing first run
    tabs.push_back(NTP);

This also nudges the caller in the direction of not knowing about policy
specifics: if we ever reach the point of the caller doing things like:
  if (onboarding_tabs[0].url == "some_specific_first_run_variant")
then our nice new separation of logic will have fallen back apart.

3. grt raised some concerns about enforcing the proper order of these tabs: the
relative positions of tabs (that may or may not appear) should always be stable.
In this case, the flexibility you're talking about might not actually be a
feature. As it stands, reading the calling function top-to-bottom expresses
exactly that strict relative ordering we were talking about.

As I said, I definitely see the merit in the other approach and even started out
that way, so I'm open to weighing these factors. WDYT?

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

IMO, this is a non-issue.  You're not working with hundreds of thousands of
objects.  C++11 already minimizes copying when doing things like creating a
vector and returning it by value, so the perf cost of this is very low, and when
we're dealing with small vectors used during one phase in startup, this won't be
measurable.
That is true, but that's why I suggested using temps for this as needed in the
caller:

  startup_tabs.push_back(GetFooTabs());  // No one cares if we added Foo tabs

  StartupTabs bar_tabs = GetBarTabs();
  if (bar_tabs.empty())
    startup_tabs.push_back(NTP);

This is not really worse than using |added_bar_tabs| to track this, I think.
That is fair, though that's possible in either route (it is a bit more annoying
the way you have things currently structured than the way I'm suggesting). 
Without knowing why callers would end up adding such a thing, if any such use is
legit (that we aren't thinking of), etc., it's hard for me to judge how much win
this distinction buys.
I view this from a separation-of-logic perspective: it is not the job of the
called functions to know that the resulting tabs should be appended to some
other vector.  They shouldn't see whether there are already other startup tabs
or know where these go relative to them; that all belongs on the caller side.

Making the functions append forces the caller to implement "put these in the
right order" by actually calling the functions in that order, which IMO doesn't
make it more likely that the caller avoids bugs with the overall ordering, it
just makes it harder to restructure code if needed for decision making (e.g. my
concerns about the caller ordering that you noted you hadn't found a better way
to address).

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

OK, switched back to the Getter pattern. I think I'm happier with it now, even
if it's slightly less concise.

+
+  // Gathers tabs from a Master Preferences file indicating first run logic
+  // specific to this distribution, and appends them to |tabs|. Also clears
+  // the value of first_run_urls_ in the provided BrowserCreator. Returns true

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

Hmm, should clearing the |browser_creator|'s member be the job of this function?
 I kinda feel like these functions should act more like const getters that have
no side effects.

That would also (presumably?) let us pass |browser_creator| (if still necessary)
by const ref or const pointer.

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

I was torn on this one as well, but in the end the reason I settled on this was
about division-of-logic. I want the caller to be interested only in
relationships between policies, and never details of the policies themselves.
This method encapsulates all the reading, writing, and mutating of state related
to Master Prefs. Basically our lasagna code looks like:
- DetermineStartupTabs: Only enforces relationships between adders
- Add*Tabs: Interacts with system state, gathers meaningful status variables
- Check*TabPolicy: Expresses policy by statically determining what those status
variables mean
The upshot is that we consolidate all stateful actions into that middle layer,
and the other two layers can then be unit-tested expressively and exhaustively.

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

I kinda think clearing the first run URLs shouldn't happen anywhere in this flow
at all.  That is, we should get the startup tabs, put them in order, tell the
browser to display them, etc., and then, if startup completes successfully and
we make it to a running state, something somewhere else should be like "hey, we
had a successful first launch, let's go clear this variable so these don't get
shown again".  It's not really the job of the code that is trying to collect
"things to show on startup" at all, and changing it there risks never showing
these if e.g. we crash right after this call.

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

Do you have a good suggestion for the "something somewhere else"? In this case
it's a chunk of logic I migrated from the old flow and I'm not 100% certain what
the rationale for putting it here specifically was, so I don't entirely have the
background to argue for one place or another.

(Incidentally, this is and always has been gated on IsChromeFirstRun, plus we
don't show anything policy-related on crash recovery, so the crash use case is
not particularly relevant, though I appreciate the effort to try to solve the
"what if" more generally.)

[Peter Kasting, 2016/10/06 05:49:08]

I do not -- not very familiar with startup.

I think this is worth looking into enough to fix (but see continued response
below), and I would suggest a TODO and/or bug to handle it after this patch (I
wouldn't worry about moving it elsewhere in this CL).
Are you saying that, even if this weren't cleared, we wouldn't show it during
future runs anyway, because IsChromeFirstRun() would return false for those?  If
so, why does this function need to clear the first run URLs at all?

If we can avoid the need to clear this variable, then of course that eliminates
the need to worry about the above question.

[tmartino, 2016/10/06 18:27:28]

crbug.com/653590

+  // iff any tabs were added.
+  virtual bool AddDistributionFirstRunTabs(
+      StartupBrowserCreator* browser_creator,
+      StartupTabs* tabs) const = 0;
+
+  // Determines which tabs should be shown as a result of a Reset Trigger
+  // present on this profile, and appends them to |tabs|. Returns true iff any
+  // tabs were added.
+  virtual bool AddResetTriggerTabs(Profile* profile,

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

Nit: Can |profile| be const?

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

Nope. TriggeredProfileResetterFactory::GetForBrowserContext takes a non-const.

+                                   StartupTabs* tabs) const = 0;
+
+  // Reads tabs from the user's pinned tabs, and appends them to |tabs|.

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

Nit: Second place you've talked about "reading from pinned tabs" that makes me
think as a reader that you mean some special thing distinct from just "Appends
the user's pinned tabs to |tabs|."  If that's what you mean, go for the less
verbose version :)

+  // Returns true iff any tabs were added.
+  virtual bool AddPinnedTabs(StartupTabs* tabs) const = 0;
+
+  // Reads tabs specified for new windows from the user's preferences, and

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

Nit: What is a "tab specified for [a] new window"?  As distinct, presumably,
from tabs specified for other things?

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

Done. In this case I've retained the "reads" phrasing because it helps to break
this up into multiple clauses.

+  // appends them to |tabs|. Returns true iff any tabs were added.
+  virtual bool AddPreferencesTabs(StartupTabs* tabs) const = 0;
+};
+
+class StartupTabProviderImpl : public StartupTabProvider {
+ public:
+  StartupTabProviderImpl() = default;
+  bool AddOnboardingTabs(StartupTabs* tabs) const override;

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

Nit: Common practice is to do something like:

  StartupTabProviderImpl() = default;

  // StartupTabProvider:
  ...overrides

I would also suggest putting static methods above everything but the constructor
and destructor, but the Google style guide is flexible here.

+  bool AddDistributionFirstRunTabs(StartupBrowserCreator* browser_creator,
+                                   StartupTabs* tabs) const override;
+  bool AddResetTriggerTabs(Profile* profile, StartupTabs* tabs) const override;
+  bool AddPinnedTabs(StartupTabs* tabs) const override;
+  bool AddPreferencesTabs(StartupTabs* tabs) const override;
+
+  static bool CheckStandardOnboardingTabPolicy(bool is_first_run,

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

Nit: What do these functions do?  Add comments.

+                                               StartupTabs* tabs);
+  static bool CheckMasterPrefsTabPolicy(bool is_first_run,
+                                        const std::vector<GURL>& first_run_tabs,
+                                        StartupTabs* tabs);
+  static bool CheckResetTriggerTabPolicy(bool profile_has_trigger,
+                                         StartupTabs* tabs);
+
+  DISALLOW_COPY_AND_ASSIGN(StartupTabProviderImpl);

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

Nit: Technically, now that this uses =delete, it is OK to put it in the public
section.  However, current common practice is to put it in the private section,
and I would continue to do that for now at least.

+};
+
+// Gets the URL for the "Welcome to Chrome" dialog.
+// TODO(tmartino): Update to return new Welcome page when complete.
+GURL GetWelcomePageUrl();

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

Nit: Never been a big fan of globals.  Would it make sense to scope these as
public statics on StartupTabProvider[Impl]?  Put them in some other file?

+GURL GetTriggeredResetSettingsUrl();

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

Nit: Blank line + comment above this?  I don't know what a "triggered reset
settings URL" is.

+
+#endif  // CHROME_BROWSER_UI_STARTUP_STARTUP_TAB_PROVIDER_H_