Adding Pinned Tab and Preferences-specified Tab logic to Startup Flow refactor

This patch fills in the TODOs in StartupTabProvider regarding pinned tabs and user preferences-specified default tabs.

BUG=656139, 248883, 517248
Committed: https://crrev.com/f51ecc9beca2aadad233c44ed098406361d128a8
Cr-Commit-Position: refs/heads/master@{#426339}

[tmartino, 2016-10-06 18:35:54]

Description was changed from

==========
it's done mr. frodo


done pending refactor feedback

BUG=
==========

to

==========
This patch fills in the TODOs in StartupTabProvider regarding pinned tabs and
user preferences-specified default tabs.

BUG=618454, 248883, 517248
==========

[tmartino, 2016-10-14 21:24:15]

Description was changed from

==========
This patch fills in the TODOs in StartupTabProvider regarding pinned tabs and
user preferences-specified default tabs.

BUG=618454, 248883, 517248
==========

to

==========
This patch fills in the TODOs in StartupTabProvider regarding pinned tabs and
user preferences-specified default tabs.

BUG=656139, 248883, 517248
==========

[tmartino, 2016-10-17 18:38:08]

tmartino@chromium.org changed reviewers:
+ pkasting@chromium.org

[tmartino, 2016-10-17 18:38:09]

Hi Peter,

Here's the first follow-up to my refactor CL. It's fairly straightforward,
filling in TODOs I've carved out before.

There is one intentional behavior change I've made here, which is removing the
deduplication of URLs which appear in both the Pinned and Preferences URLs. I'm
of course open to being overridden on this, but I think it's a good
simplification. I've outlined my rationale for this change:
https://docs.google.com/a/google.com/document/d/1X1KRlF8S28EklRU_PXXpRNd-AJqJ...

If we proceed with this change, it would allow me to deprecate the old
AddUniqueUrls method, which is currently unused in this CL and which you and grt
both raised some issues with post-submit on the other CL. (It was unchanged in
that CL.)

Thanks!
Tommy

[Peter Kasting, 2016-10-18 00:53:42]

I left some thoughts on your doc about eliminating the deduping.  I'm not sure
whether we can get rid of it.

Everything else LGTM.

https://codereview.chromium.org/2396133002/diff/40001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_tab_provider.h (right):

https://codereview.chromium.org/2396133002/diff/40001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_tab_provider.h:51: // The static
Check*TabPolicy methods below enforce the policies relevant to
Random thought (not related to this CL, I'm just noticing it): I dunno that
"CheckXPolicy" is a great name for these.  To me that sounds like a function
that checks that a particular policy is being properly enforced.  It seems like
in the case of these functions, they're kinda more like:

  // Implementations of the base class functions, these call the functions below
  // with the appropriate system state.
  virtual StartupTabs GetABCTabs();
  virtual StartupTabs GetXYZTabs();

  // Helpers used to implement the above, given all necessary state as
  // arguments.  Exposed separately for easy testing.
  static StartupTabs GetABCTabsForState(...);
  static StartupTabs GetXYZTabsForState(...);

In other words, these are still about "get the XYZ tabs", they're just about
doing so for specific, provided state rather than "whatever the current state of
the system is".

https://codereview.chromium.org/2396133002/diff/40001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_tab_provider_unittest.cc (right):

https://codereview.chromium.org/2396133002/diff/40001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_tab_provider_unittest.cc:74: 
Nit: No blank here?  (You don't do similar blanks in the next test)

https://codereview.chromium.org/2396133002/diff/40001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_tab_provider_unittest.cc:84:
std::vector<GURL>({GURL(base::ASCIIToUTF16("https://www.google.com"))});
Nit: I think you can omit "std::vector<GURL>()" (2 places)

[Peter Kasting, 2016-10-19 21:17:56]

Eliminating deduping also LGTM.

https://codereview.chromium.org/2396133002/diff/40001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/2396133002/diff/40001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:684: for (size_t j =
0; j < num_existing_tabs; ++j) {
Note: In this CL or a future one, consider applying improvement suggestions from
https://codereview.chromium.org/2164033002/diff/400001/chrome/browser/ui/star...
.

[tmartino, 2016-10-19 22:01:29]

https://codereview.chromium.org/2396133002/diff/40001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/2396133002/diff/40001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:684: for (size_t j =
0; j < num_existing_tabs; ++j) {
On 2016/10/19 at 21:17:56, Peter Kasting wrote:
> Note: In this CL or a future one, consider applying improvement suggestions
from
https://codereview.chromium.org/2164033002/diff/400001/chrome/browser/ui/star...
.

Haven't forgotten about this! However, this is part of the dedupe logic that
we've decided not to carry forward, so we can just deprecate this chunk of code
entirely.

Since I'm viewing this CL as a merge candidate, I'm going to hold off marking
the deprecation for the moment as a matter of scoping. (Any sort of reordering
touches a lot of code and therefore makes me nervous.)

https://codereview.chromium.org/2396133002/diff/40001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_tab_provider.h (right):

https://codereview.chromium.org/2396133002/diff/40001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_tab_provider.h:51: // The static
Check*TabPolicy methods below enforce the policies relevant to
On 2016/10/18 at 00:53:41, Peter Kasting wrote:
> Random thought (not related to this CL, I'm just noticing it): I dunno that
"CheckXPolicy" is a great name for these.  To me that sounds like a function
that checks that a particular policy is being properly enforced.  It seems like
in the case of these functions, they're kinda more like:
> 
>   // Implementations of the base class functions, these call the functions
below
>   // with the appropriate system state.
>   virtual StartupTabs GetABCTabs();
>   virtual StartupTabs GetXYZTabs();
> 
>   // Helpers used to implement the above, given all necessary state as
>   // arguments.  Exposed separately for easy testing.
>   static StartupTabs GetABCTabsForState(...);
>   static StartupTabs GetXYZTabsForState(...);
> 
> In other words, these are still about "get the XYZ tabs", they're just about
doing so for specific, provided state rather than "whatever the current state of
the system is".

Noted; this is a good candidate for cleanup but I'd prefer to keep scope limited
on this change.

https://codereview.chromium.org/2396133002/diff/40001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_tab_provider_unittest.cc (right):

https://codereview.chromium.org/2396133002/diff/40001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_tab_provider_unittest.cc:74: 
On 2016/10/18 at 00:53:41, Peter Kasting wrote:
> Nit: No blank here?  (You don't do similar blanks in the next test)

I did use blanks in the above tests, so I've added them below instead.

https://codereview.chromium.org/2396133002/diff/40001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_tab_provider_unittest.cc:84:
std::vector<GURL>({GURL(base::ASCIIToUTF16("https://www.google.com"))});
On 2016/10/18 at 00:53:41, Peter Kasting wrote:
> Nit: I think you can omit "std::vector<GURL>()" (2 places)

Done

[tmartino, 2016-10-19 22:01:51]

The CQ bit was checked by tmartino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-19 22:03:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-19 23:18:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-19 23:18:38]

Dry run: This issue passed the CQ dry run.

[tmartino, 2016-10-19 23:20:50]

The CQ bit was checked by tmartino@chromium.org

[tmartino, 2016-10-19 23:20:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2396133002/#ps60001
(title: "Correcting initializer nit")

[commit-bot: I haz the power, 2016-10-19 23:21:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-19 23:42:29]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-21 13:13:22]

Description was changed from

==========
This patch fills in the TODOs in StartupTabProvider regarding pinned tabs and
user preferences-specified default tabs.

BUG=656139, 248883, 517248
==========

to

==========
This patch fills in the TODOs in StartupTabProvider regarding pinned tabs and
user preferences-specified default tabs.

BUG=656139, 248883, 517248

Committed: https://crrev.com/f51ecc9beca2aadad233c44ed098406361d128a8
Cr-Commit-Position: refs/heads/master@{#426339}
==========

[commit-bot: I haz the power, 2016-10-21 13:13:23]

Patchset 4 (id:??) landed as
https://crrev.com/f51ecc9beca2aadad233c44ed098406361d128a8
Cr-Commit-Position: refs/heads/master@{#426339}

[grt (UTC plus 2), 2016-10-24 06:55:36]

post-commit drive-by

https://codereview.chromium.org/2396133002/diff/60001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_tab_provider.cc (right):

https://codereview.chromium.org/2396133002/diff/60001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_tab_provider.cc:110: SessionStartupPref pref)
{
should this be passed by const-ref since it contains a vector of GURLs?

https://codereview.chromium.org/2396133002/diff/60001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_tab_provider.cc:113: for (auto& url :
pref.urls)
nit: "const auto&"