Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2164033002:
Refactoring startup logic for upcoming FRE changes (non-Win 10). (Closed)
DescriptionRefactoring startup logic for upcoming FRE changes (non-Win 10).
Refactor goals (in descending order):
1. Consolidate the various places throughout startup where onboarding and other special-case tabs are added.
2. Reduce the massive number of branches, spaghetti calls, and seemingly-redundant checks into a manageable, linear flow.
3. Offload all policy logic (e.g., "Should we show the Welcome page?") for FRE to the more-appropriate first_run directory.
4. Where possible, consolidate code into testable units.
BUG=618454, 248883, 517248
Committed: https://crrev.com/b930f6e90137299cc399cd35bcf7e7f89c19587e
Cr-Commit-Position: refs/heads/master@{#423636}
Patch Set 1 #Patch Set 2 : Expanding refactor (WIP) #
Total comments: 12
Patch Set 3 : Addressing rogerta feedback #
Total comments: 16
Patch Set 4 : Addressing comments from grt, excluding structural changes #
Total comments: 2
Patch Set 5 : Further nits #Patch Set 6 : Preview of struct-based refactor #Patch Set 7 : Adding first unit tests #
Total comments: 35
Patch Set 8 : Restructuring to use Provider class #Patch Set 9 : Adding missing constructor #
Total comments: 38
Patch Set 10 : Changing getters to adders #
Total comments: 12
Patch Set 11 : Misc changes to argument passing #
Total comments: 5
Patch Set 12 : Additional Nits #
Total comments: 6
Patch Set 13 : Fixing side-effect behavior changes #Patch Set 14 : Line break nit #
Total comments: 4
Patch Set 15 : Further tweaks to conditionals #
Total comments: 3
Patch Set 16 : Test nits #Patch Set 17 : Win build issue #
Total comments: 75
Patch Set 18 : Addressing pkasting feedback #Patch Set 19 : Further feedback from pkasting #Patch Set 20 : Using new welcome page URL #
Total comments: 18
Patch Set 21 : Attempting submission #
Total comments: 3
Messages
Total messages: 67 (24 generated)
Description was changed from ========== Refactoring logic for upcoming FRE changes (non-Win 10). BUG=618454, 248883 ========== to ========== Refactoring startup logic for upcoming FRE changes (non-Win 10). Specifically, the goal is to consolidate the various places throughout startup where onboarding and other special-case tabs are added. BUG=618454, 248883 ==========
tmartino@chromium.org changed reviewers: + rogerta@chromium.org
Sending WIP version of refactor for sanity check.
Description was changed from ========== Refactoring startup logic for upcoming FRE changes (non-Win 10). Specifically, the goal is to consolidate the various places throughout startup where onboarding and other special-case tabs are added. BUG=618454, 248883 ========== to ========== Refactoring startup logic for upcoming FRE changes (non-Win 10). Refactor goals (in descending order): 1. Consolidate the various places throughout startup where onboarding and other special-case tabs are added. 2. Reduce the massive number of branches, spaghetti calls, and seemingly-redundant checks into a manageable, linear flow. 3. Offload all policy logic (e.g., "Should we show the Welcome page?") for FRE to the more-appropriate first_run directory. 4. Where possible, consolidate code into testable units. BUG=618454, 248883 ==========
Looks fine so far Tommy. Some minor comments below. https://codereview.chromium.org/2164033002/diff/20001/chrome/browser/first_ru... File chrome/browser/first_run/first_run.cc (right): https://codereview.chromium.org/2164033002/diff/20001/chrome/browser/first_ru... chrome/browser/first_run/first_run.cc:661: } Nit: internal::g_first_run = is_first_run ? internal::FIRST_RUN_TRUE : internal::FIRST_RUN_FALSE; https://codereview.chromium.org/2164033002/diff/20001/chrome/browser/first_ru... chrome/browser/first_run/first_run.cc:756: } Might be better to move some parts of GetOnboardingTabs() to internal:: so that each platform can have its own logic. Probably shouldn't NOTREACHED() if the win10 changes will be implemented in a followup CL :-) https://codereview.chromium.org/2164033002/diff/20001/chrome/browser/first_ru... File chrome/browser/first_run/first_run.h (right): https://codereview.chromium.org/2164033002/diff/20001/chrome/browser/first_ru... chrome/browser/first_run/first_run.h:148: bool IsWin10(); Should put this inside #ifdef OS_WIN, or somehow refactor code so that windows specific code only appears in first_run_internal_win.cc https://codereview.chromium.org/2164033002/diff/20001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2164033002/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:586: #if defined(OS_WIN) Don't forget to copy TODO from line 517. https://codereview.chromium.org/2164033002/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:641: return NULL; Is it possible for session restore to restore multiple top level windows? I think so, but not sure. https://codereview.chromium.org/2164033002/diff/20001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator_impl.h (right): https://codereview.chromium.org/2164033002/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.h:110: const std::vector<GURL>& urls_to_open); The word "new" is discouraged in the code base, since this feature won't be new eventually. I was going to suggest "use the feature name", but that also uses "new" :-) So I'd suggest coming up with a meaningful name and use here an in feature.
https://codereview.chromium.org/2164033002/diff/20001/chrome/browser/first_ru... File chrome/browser/first_run/first_run.cc (right): https://codereview.chromium.org/2164033002/diff/20001/chrome/browser/first_ru... chrome/browser/first_run/first_run.cc:661: } On 2016/07/26 at 15:00:26, Roger Tawa wrote: > Nit: > > internal::g_first_run = > is_first_run ? internal::FIRST_RUN_TRUE > : internal::FIRST_RUN_FALSE; Done. https://codereview.chromium.org/2164033002/diff/20001/chrome/browser/first_ru... chrome/browser/first_run/first_run.cc:756: } On 2016/07/26 at 15:00:26, Roger Tawa wrote: > Might be better to move some parts of GetOnboardingTabs() to internal:: so that each platform can have its own logic. Probably shouldn't NOTREACHED() if the win10 changes will be implemented in a followup CL :-) Hmm. I've thought about this a bit and the best way to structure it using internal:: still isn't clear to me. The issue is that our division is not actually "here is the mac code, here is the windows code" etc. To use platform-specific logic, it'd look like: _win.cc: if (IsWin10()) { // Do special flow } else { // Same code on all platforms } _mac.cc: // Same code on all platforms _linux.cc: // Same code on all platforms _posix.cc: // Same code on all platforms Now, when the time comes to resolve the TODO here and add Win10-specific logic, I'm completely OK with having that logic live in _win.cc and simply invoking it from here. https://codereview.chromium.org/2164033002/diff/20001/chrome/browser/first_ru... File chrome/browser/first_run/first_run.h (right): https://codereview.chromium.org/2164033002/diff/20001/chrome/browser/first_ru... chrome/browser/first_run/first_run.h:148: bool IsWin10(); On 2016/07/26 at 15:00:26, Roger Tawa wrote: > Should put this inside #ifdef OS_WIN, or somehow refactor code so that windows specific code only appears in first_run_internal_win.cc For the time being, where we want to use this both in the policy logic and as a guard against using the new flow at all, things are a lot cleaner this way. I've added a TODO and will make that move in the CL which adds the Win10 logic (and therefore removes the guard from startup_browser_creator_impl). Note that on non-Windows platforms this currently just returns false, so it doesn't particularly introduce any risk. https://codereview.chromium.org/2164033002/diff/20001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2164033002/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:586: #if defined(OS_WIN) On 2016/07/26 at 15:00:26, Roger Tawa wrote: > Don't forget to copy TODO from line 517. Done. https://codereview.chromium.org/2164033002/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:641: return NULL; On 2016/07/26 at 15:00:26, Roger Tawa wrote: > Is it possible for session restore to restore multiple top level windows? I think so, but not sure. I've clarified my skeleton. There are two types of session restore. Synchronous session restore (which is what MaybeRestoreSession will handle) returns the last active Browser as a pointer. (My understanding is that it may restore more than one window, but we only get a handle on the last active one.) https://codereview.chromium.org/2164033002/diff/20001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator_impl.h (right): https://codereview.chromium.org/2164033002/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.h:110: const std::vector<GURL>& urls_to_open); On 2016/07/26 at 15:00:26, Roger Tawa wrote: > The word "new" is discouraged in the code base, since this feature won't be new eventually. I was going to suggest "use the feature name", but that also uses "new" :-) So I'd suggest coming up with a meaningful name and use here an in feature. Done.
Description was changed from ========== Refactoring startup logic for upcoming FRE changes (non-Win 10). Refactor goals (in descending order): 1. Consolidate the various places throughout startup where onboarding and other special-case tabs are added. 2. Reduce the massive number of branches, spaghetti calls, and seemingly-redundant checks into a manageable, linear flow. 3. Offload all policy logic (e.g., "Should we show the Welcome page?") for FRE to the more-appropriate first_run directory. 4. Where possible, consolidate code into testable units. BUG=618454, 248883 ========== to ========== Refactoring startup logic for upcoming FRE changes (non-Win 10). Refactor goals (in descending order): 1. Consolidate the various places throughout startup where onboarding and other special-case tabs are added. 2. Reduce the massive number of branches, spaghetti calls, and seemingly-redundant checks into a manageable, linear flow. 3. Offload all policy logic (e.g., "Should we show the Welcome page?") for FRE to the more-appropriate first_run directory. 4. Where possible, consolidate code into testable units. BUG=618454, 248883, 517248 ==========
tmartino@chromium.org changed reviewers: + grt@chromium.org
+grt Adding bug 517248.
some nits and a suggestion below. thanks for hacking on this! https://codereview.chromium.org/2164033002/diff/40001/chrome/browser/first_ru... File chrome/browser/first_run/first_run.cc (left): https://codereview.chromium.org/2164033002/diff/40001/chrome/browser/first_ru... chrome/browser/first_run/first_run.cc:494: nit: retain this blank line https://codereview.chromium.org/2164033002/diff/40001/chrome/browser/first_ru... File chrome/browser/first_run/first_run.cc (right): https://codereview.chromium.org/2164033002/diff/40001/chrome/browser/first_ru... chrome/browser/first_run/first_run.cc:88: const char* kNewTabMagicWord = "new_tab_page"; constexpr char kNewTabMagicWord[] = ... https://codereview.chromium.org/2164033002/diff/40001/chrome/browser/first_ru... chrome/browser/first_run/first_run.cc:88: const char* kNewTabMagicWord = "new_tab_page"; use "UrlHost" or something rather than "MagicWord" https://codereview.chromium.org/2164033002/diff/40001/chrome/browser/first_ru... chrome/browser/first_run/first_run.cc:735: return os_info->version() >= base::win::VERSION_WIN10; base::win::GetVersion() and #include "base/win/windows_version.h" wrapped in #if defined(OS_WIN) up above https://codereview.chromium.org/2164033002/diff/40001/chrome/browser/first_ru... chrome/browser/first_run/first_run.cc:755: for (GURL tab : tabs) { const GURL& tab to avoid a copy here https://codereview.chromium.org/2164033002/diff/40001/chrome/browser/first_ru... File chrome/browser/first_run/first_run_features.cc (right): https://codereview.chromium.org/2164033002/diff/40001/chrome/browser/first_ru... chrome/browser/first_run/first_run_features.cc:6: namespace features { blank line before this https://codereview.chromium.org/2164033002/diff/40001/chrome/browser/first_ru... File chrome/browser/first_run/first_run_features.h (right): https://codereview.chromium.org/2164033002/diff/40001/chrome/browser/first_ru... chrome/browser/first_run/first_run_features.h:12: extern const base::Feature kUseConsolidatedFirstRun; is chrome/common/chrome_features.h an appropriate place for this? https://codereview.chromium.org/2164033002/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2164033002/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:505: !first_run::IsWin10()) { i don't think "IsWin10" should be a public method in first_run. https://codereview.chromium.org/2164033002/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:591: app_metro_launch::HandleAppLaunchForMetroRestart(profile_); metro mode is gone. i don't think this dead code needs to be here any longer if it truly is only for metro->desktop transitions https://codereview.chromium.org/2164033002/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:627: Browser* browser = MaybeRestoreSession(adjusted_urls, process_startup); this is mixing policy with functionality. can you tease these apart? is it possible to have different phases (even classes, if that makes sense) such as: - gather inputs - make policy decisions that result in outputs such as: - whether or not any browser windows should be created - the ordered list of URLs to open - whether or not any infobars (and which ones) should be included - act on the policy decisions unittesting the second step will then be a simple matter of craft inputs, run the policy function, and verify the outputs. https://codereview.chromium.org/2164033002/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator_impl.h (right): https://codereview.chromium.org/2164033002/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.h:113: Browser* MaybeRestoreSession(const std::vector<GURL>& urls, be consistent with arg ordering
Addressed the straightforward comments from grt. The input for the structural changes makes sense (I think :) but I want to leave it as a separate patchset, so I'll respond with that shortly. Open question: How do you think this should be split up, cl-wise? My weak preference would be to do the restructuring and then fix the remaining TODOs separately, for sanity's sake. However, that does mean introducing incomplete (but not broken) codepaths behind the flag. https://codereview.chromium.org/2164033002/diff/40001/chrome/browser/first_ru... File chrome/browser/first_run/first_run_features.h (right): https://codereview.chromium.org/2164033002/diff/40001/chrome/browser/first_ru... chrome/browser/first_run/first_run_features.h:12: extern const base::Feature kUseConsolidatedFirstRun; On 2016/07/29 at 06:56:15, grt (UTC plus 2) wrote: > is chrome/common/chrome_features.h an appropriate place for this? The PMs on this have indicated a desire to do a fair amount of future fiddling/tweaking/general experimentation (for instance, the Win10 portion of this refresh will be tested in a few different layouts) so I thought it made sense to group these new features together as they appear, to avoid including them all over the place, and to reduce the overhead of going to top-level /chrome/ OWNERS for such changes. Weak preference though.
https://codereview.chromium.org/2164033002/diff/40001/chrome/browser/first_ru... File chrome/browser/first_run/first_run.cc (right): https://codereview.chromium.org/2164033002/diff/40001/chrome/browser/first_ru... chrome/browser/first_run/first_run.cc:88: const char* kNewTabMagicWord = "new_tab_page"; On 2016/07/29 06:56:15, grt (UTC plus 2) wrote: > use "UrlHost" or something rather than "MagicWord" I find that "Keyword" is still vague. Neither the constant's name nor the doc comment that accompanies it convey that the constant is the host part of an URL. By replacing "Keyword" with "UrlHost" (and updating the comment), I think this is fairly clear. https://codereview.chromium.org/2164033002/diff/40001/chrome/browser/first_ru... File chrome/browser/first_run/first_run_features.h (right): https://codereview.chromium.org/2164033002/diff/40001/chrome/browser/first_ru... chrome/browser/first_run/first_run_features.h:12: extern const base::Feature kUseConsolidatedFirstRun; On 2016/07/29 15:28:12, tmartino wrote: > On 2016/07/29 at 06:56:15, grt (UTC plus 2) wrote: > > is chrome/common/chrome_features.h an appropriate place for this? > > The PMs on this have indicated a desire to do a fair amount of future > fiddling/tweaking/general experimentation (for instance, the Win10 portion of > this refresh will be tested in a few different layouts) so I thought it made > sense to group these new features together as they appear, to avoid including > them all over the place, and to reduce the overhead of going to top-level > /chrome/ OWNERS for such changes. > > Weak preference though. Acknowledged. https://codereview.chromium.org/2164033002/diff/60001/chrome/browser/first_ru... File chrome/browser/first_run/first_run_features.cc (right): https://codereview.chromium.org/2164033002/diff/60001/chrome/browser/first_ru... chrome/browser/first_run/first_run_features.cc:11: } // namespace features nit: blank line before this one, too. :-) https://codereview.chromium.org/2164033002/diff/60001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2164033002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:576: // TODO(tapted): Move this to startup_browser_creator_win.cc after refactor. nuke this one, too?
On 2016/07/29 15:28:12, tmartino wrote: > Addressed the straightforward comments from grt. The input for the structural > changes makes sense (I think :) but I want to leave it as a separate patchset, > so I'll respond with that shortly. > > Open question: How do you think this should be split up, cl-wise? My weak > preference would be to do the restructuring and then fix the remaining TODOs > separately, for sanity's sake. However, that does mean introducing incomplete > (but not broken) codepaths behind the flag. I think this is okay if you can guarantee that there are no changes to the product while you work on your feature, but I have a reservation. My concern with adding to the spaghetti before restructuring is that it may make it more difficult to ensure that there are no behavioral changes after the refactor. I think it would be cleaner to land a refactor that makes your addition more straightforward and then make your addition. You may wish to consult the OWNERS you'll eventually need to review your changes to see what they think. Ultimately, it is they who will have the final say.
Re: Structure ------------ I've looked into this in more detail and I think I have a plan now. I've updated the design doc to describe this as well: https://docs.google.com/document/d/1Lmn3C9Y8OQEfMwJmQlI-NSvcDgNrqz_TyPE3_5woD... Basically, I'm thinking: - Gather inputs for onboarding tabs. Method outputs a struct describing various system statuses. - Pass this struct to revised version of GetOnboardingTabs which depends only on this struct and outputs a list of URLs. Unit test extensively. - Use a similarly-structured pair of functions for determining non-onboarding special tabs (e.g., profile reset). - Maybe async restore. The only input for this that we have visibility on is whether or not this is process startup; otherwise, all this logic is already encapsulated by the session service. If the external logic decides it's necessary, our work here is done. - Gather inputs for a possible synchronous restore. Outputs a struct describing relevant statuses. - Pass this struct to a new method which encapsulates policy for deciding whether or not to do a synchronous restore. Method outputs a boolean. - Pass this boolean and our URLs to a method which gets a window, either by restoring or by opening a new one. - Pass our window to the existing InfoBar method. It does not have the nice input/policy/action divide, and I may later refactor it accordingly, but for the time being it is at least self-contained and working, and I have to keep the scope of these changes sane. WDYT? Do you think data-only structs make sense in this case? Re: Landing ------------ So, we have a product requirement that the old flow, with all its intricacies, remain accessible in the interim. We want to do comparison testing, and we want to be able to disable the new changes entirely in the unlikely scenario that we aren't happy with the metrics. That's why I'm basically implementing this as a wholesale replacement for the existing code path, rather than making changes in-place. Obviously once that flag is on-by-default, there will be a slash-and-burn of the methods I've marked as deprecated here. My intention was never to reimplement the current policy under this refactor; the new policy is dramatically simpler and that simplification is one of the motivating factors for the startup refactor. https://codereview.chromium.org/2164033002/diff/40001/chrome/browser/first_ru... File chrome/browser/first_run/first_run.cc (right): https://codereview.chromium.org/2164033002/diff/40001/chrome/browser/first_ru... chrome/browser/first_run/first_run.cc:88: const char* kNewTabMagicWord = "new_tab_page"; On 2016/08/01 at 06:23:14, grt (UTC plus 2) wrote: > On 2016/07/29 06:56:15, grt (UTC plus 2) wrote: > > use "UrlHost" or something rather than "MagicWord" > > I find that "Keyword" is still vague. Neither the constant's name nor the doc comment that accompanies it convey that the constant is the host part of an URL. By replacing "Keyword" with "UrlHost" (and updating the comment), I think this is fairly clear. Aha! In fact, I've been pointedly avoiding calling these UrlHost because they explicitly are *not* hosts, or any other part of an actual URL. They're constant strings, defined by convention, which partners include *in place* of a valid URL in their Master Prefs file, and which we are replacing with something resolvable in this function.
Update: I spoke to pkasting@ (who will be doing OWNERS review) about landing and TODOs, and he expressed that he was ok with landing incrementally so long as: (1) The structure of the new code remains relatively linear, and (2) Additional documentation is added to indicate which chunks of existing code correspond to which skeletal portions.
On 2016/08/02 23:46:50, tmartino wrote: > Re: Structure > ------------ > I've looked into this in more detail and I think I have a plan now. I've updated > the design doc to describe this as well: > https://docs.google.com/document/d/1Lmn3C9Y8OQEfMwJmQlI-NSvcDgNrqz_TyPE3_5woD... > > Basically, I'm thinking: > - Gather inputs for onboarding tabs. Method outputs a struct describing various > system statuses. > - Pass this struct to revised version of GetOnboardingTabs which depends only on > this struct and outputs a list of URLs. Unit test extensively. > - Use a similarly-structured pair of functions for determining non-onboarding > special tabs (e.g., profile reset). > - Maybe async restore. The only input for this that we have visibility on is > whether or not this is process startup; otherwise, all this logic is already > encapsulated by the session service. If the external logic decides it's > necessary, our work here is done. > - Gather inputs for a possible synchronous restore. Outputs a struct describing > relevant statuses. > - Pass this struct to a new method which encapsulates policy for deciding > whether or not to do a synchronous restore. Method outputs a boolean. > - Pass this boolean and our URLs to a method which gets a window, either by > restoring or by opening a new one. > - Pass our window to the existing InfoBar method. It does not have the nice > input/policy/action divide, and I may later refactor it accordingly, but for the > time being it is at least self-contained and working, and I have to keep the > scope of these changes sane. > > WDYT? Do you think data-only structs make sense in this case? Seems reasonable if I'm understanding. You might want to play with writing the unit tests first to see what these structs would look like and how they could be tested. The landing plan sgtm. Thanks.
https://codereview.chromium.org/2164033002/diff/40001/chrome/browser/first_ru... File chrome/browser/first_run/first_run.cc (right): https://codereview.chromium.org/2164033002/diff/40001/chrome/browser/first_ru... chrome/browser/first_run/first_run.cc:88: const char* kNewTabMagicWord = "new_tab_page"; On 2016/08/02 23:46:50, tmartino wrote: > On 2016/08/01 at 06:23:14, grt (UTC plus 2) wrote: > > On 2016/07/29 06:56:15, grt (UTC plus 2) wrote: > > > use "UrlHost" or something rather than "MagicWord" > > > > I find that "Keyword" is still vague. Neither the constant's name nor the doc > comment that accompanies it convey that the constant is the host part of an URL. > By replacing "Keyword" with "UrlHost" (and updating the comment), I think this > is fairly clear. > > Aha! In fact, I've been pointedly avoiding calling these UrlHost because they > explicitly are *not* hosts, or any other part of an actual URL. They're constant > strings, defined by convention, which partners include *in place* of a valid URL > in their Master Prefs file, and which we are replacing with something resolvable > in this function. I see it from the other side: they are magic values placed in the host portion of a URL. The fact that they don't resolve is irrelevant. An URL is still an URL if the host doesn't resolve, and the host portion of the URL is still the host portion of the URL even if it's gibberish.
Hey Greg, Thanks again for doing so many (and such helpful) iterations on this! Sorry for my latency--I've been tangled up in MTV travel, OOO time, and working on the client side (HTML, etc) of this feature. I wanted to give a quick preview of where this is headed before I dive too far into the, uh...re-refactoring, so I uploaded a patchset that just changes the structure of GetOnboardingTabs (and addresses one or two other lingering nits). I haven't updated the tests yet, but the format will pretty straightforward--populate various permutations of values in the struct, pass to function, verify output. Then come similar-shaped changes for the other special tabs, and getting a browser window. Cheers!
I'll take a look tomorrow. Ciao
On 2016/08/22 at 19:11:13, grt wrote: > I'll take a look tomorrow. Ciao Sounds good, thanks. I had a few minutes so I added the unit tests relating to this chunk of code, as well.
https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/first_r... File chrome/browser/first_run/first_run.cc (right): https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/first_r... chrome/browser/first_run/first_run.cc:510: FirstRunState g_first_run = FIRST_RUN_UNKNOWN; please move this into the unnamed namespace while you're here https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/first_r... chrome/browser/first_run/first_run.cc:646: FirstRunSystemStatus GetFirstRunSystemStatus() { i don't see the benefit of this function and FirstRunSystemStatus. what does it add? https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/first_r... chrome/browser/first_run/first_run.cc:671: // TODO(tmartino): Win 10 logic is more complex and will be added in its on win10, the welcome page is shown on first run as well as first-run-after-os-upgrade. i'm not sure that this is the correct place for the latter logic. at a higher level, i think the logic for which URLs are shown when belongs more with the SBC than here. https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/first_r... chrome/browser/first_run/first_run.cc:673: } else { else if https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/first_r... chrome/browser/first_run/first_run.cc:674: if (status.is_force_flag_on || what is the benefit of putting this policy logic here? this is much easier to understand if it's "if (IsChromeFirstRun())". the fact that a few conditions are used to decide whether or not this is the case should be of no concern to this code here. alternatively, pass in first_run as a bool to the function so it can be unit-tested with the only thing that matters. a separate test should cover whether or not IsChromeFirstRun returns true under the correct conditions. https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/first_r... chrome/browser/first_run/first_run.cc:774: if (IsChromeFirstRun()) { if anything, i think this should be a DCHECK. why should this function ever be called outside of first-run? https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/first_r... chrome/browser/first_run/first_run.cc:776: if (tab.host() == kNewTabUrlHost) { i think of this magic value-to-tab mapping as being a feature of the SBC rather than of first run. i suppose it's a bit of a coin-toss since the pref is "first_run_tabs". https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/first_r... File chrome/browser/first_run/first_run.h (right): https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/first_r... chrome/browser/first_run/first_run.h:107: // Makes policy decisions based on the given system status, and returns nit: "Returns the (possibly empty) collection of first-run-related tabs to be shown to the user based on the given system status" or something like that; see https://google.github.io/styleguide/cppguide.html#Function_Comments. https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/first_r... chrome/browser/first_run/first_run.h:162: GURL GetWelcomePageURL(); URL -> Url (http://google.github.io/styleguide/cppguide.html#Function_Names) https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/first_r... chrome/browser/first_run/first_run.h:165: // use in Master Prefs files with corresponding URLs. "Master Prefs" -> "master preferences" https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/first_r... File chrome/browser/first_run/first_run_features.cc (right): https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/first_r... chrome/browser/first_run/first_run_features.cc:9: const base::Feature kUseConsolidatedFirstRun{"UseConsolidatedFirstRun", it appears that this feature controls all launches, not only first-run. is this the intention? if so, please move it into browser/ui/startup and rename it. otherwise, change ProcessLaunchURLs so that the new code is only reached on first-run. https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:574: // Don't open any browser windows if we're starting up in "background mode". nit: i'm in the "no personal pronouns" camp (https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/BYMa_...). if it makes sense, please see if any comments with "we" and such in them can be reworded for clarity without the pronoun. for example, this could be: // Don't open any browser windows when the process is being started in background mode. https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:578: // If Master Prefs contains any first run tabs, use only those. in the old logic, URLs on the command-line suppress first_run_tabs. https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:580: if (browser_creator_) { && !browser_creator_->first_run_tabs_.empty() https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:581: tabs_to_insert = if you're assuming that first_run_tabs_ will never be non-empty on non-first-run, please add a DCHECK for this: DCHECK(first_run_); https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:581: tabs_to_insert = also add: DCHECK(process_startup); to document/assert the expectation that first_run_tabs_ will only be non-empty during process startup and not during rendezvous https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:592: is it intentional that the NTP is no longer opened? https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:612: Browser* browser = MaybeRestoreSession(process_startup, adjusted_urls); can you break this function into two: one that computes the urls and what to do, and another that opens them in a Browser? this way, you can exhaustively unittest the former without having to rely on browser tests. not sure how much of a gain that would be, but it's worth considering. https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:615: if (!browser) { nit: omit braces https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:630: return NULL; nit: nullptr
I took a few days to grok your comments and decided there was still some work to do on the structure here. In particular, I wanted to address your feedback regarding: * Feeling that URL decision logic belongs with SBC. * The struct setup I proposed providing little value. * The suggestion several iterations ago of using a class to encapsulate decision logic. * The desire to entirely separate out the decision logic of URLs, and the decision logic of Browser behavior, from the place where we act on those decisions. * Lack of clarity about why certain decisions were being made in certain locations. After going back and listing all the factors that affect URL selection, I decided it would be unwieldy to pass all those factors around as inputs to one monolithic testable function. I grouped the factors logically and distilled the resulting policy down into two “tiers” of policy: 1. Smaller, self-contained “chunks” of policy (e.g., onboarding promos are shown depending on whether this is first run, and eventually other factors). 2. “Meta”-policy, describing the *interactions* between these policies, such as the fact that Master Prefs content should override any other tabs that would otherwise be displayed, or the fact that NTP should be shown when nothing else is specified. From this perspective, I decided on a simple Provider class that would encapsulate the first set of policies. (Note that I expect this class to grow as our policies become more complex, such as the forthcoming Win 10 policy.) This class is structured around functions like GetOnboardingTabs, which gather relevant bits of system status and pass them along to static, unit-tested policy functions like CheckStandardOnboardingTabPolicy. The advantage of encapsulating this logic in a class, other than organization, is that it enabled me to mock this class and therefore exhaustively test the meta-policies, which are now defined in StartupBrowserCreatorImpl::DetermineStartupTabs. (Note that because they have the desired impact of “never show any content in these modes,” I have classified the Incognito and Crash Recovery states as meta-policies.) My hope is that this restructure addresses any of your prior comments that I haven't explicitly commented on. Thanks again for putting so much thought into reviewing this; your comments have been really helpful and have prompted a lot of good hard thinking and iterations. https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/first_r... File chrome/browser/first_run/first_run.cc (right): https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/first_r... chrome/browser/first_run/first_run.cc:510: FirstRunState g_first_run = FIRST_RUN_UNKNOWN; On 2016/08/23 at 10:36:02, grt (UTC plus 2) wrote: > please move this into the unnamed namespace while you're here Done https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/first_r... chrome/browser/first_run/first_run.cc:673: } else { On 2016/08/23 at 10:36:01, grt (UTC plus 2) wrote: > else if n/a, removed https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/first_r... chrome/browser/first_run/first_run.cc:674: if (status.is_force_flag_on || On 2016/08/23 at 10:36:01, grt (UTC plus 2) wrote: > what is the benefit of putting this policy logic here? this is much easier to understand if it's "if (IsChromeFirstRun())". the fact that a few conditions are used to decide whether or not this is the case should be of no concern to this code here. alternatively, pass in first_run as a bool to the function so it can be unit-tested with the only thing that matters. a separate test should cover whether or not IsChromeFirstRun returns true under the correct conditions. I'm all right with replacing the struct with a bool that is the result of IsChromeFirstRun. That change is reflected in the new function GetStandardOnboardingTabs in StartupTabProvider. I've also gone ahead and extracted the interesting logic in IsChromeFirstRun into its own function, DetermineFirstRunState. Having two layers of functions is necessary because IsChromeFirstRun relies on state (checks cached value of g_first_run, and makes calls to get switches and files), while DetermineFirstRunState is static. I've adapted my test cases to verify that new function's logic. https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/first_r... File chrome/browser/first_run/first_run.h (right): https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/first_r... chrome/browser/first_run/first_run.h:162: GURL GetWelcomePageURL(); On 2016/08/23 at 10:36:02, grt (UTC plus 2) wrote: > URL -> Url (http://google.github.io/styleguide/cppguide.html#Function_Names) Done (here and throughout) https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/first_r... chrome/browser/first_run/first_run.h:165: // use in Master Prefs files with corresponding URLs. On 2016/08/23 at 10:36:02, grt (UTC plus 2) wrote: > "Master Prefs" -> "master preferences" Done https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/first_r... File chrome/browser/first_run/first_run_features.cc (right): https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/first_r... chrome/browser/first_run/first_run_features.cc:9: const base::Feature kUseConsolidatedFirstRun{"UseConsolidatedFirstRun", On 2016/08/23 10:36:02, grt (UTC plus 2) wrote: > it appears that this feature controls all launches, not only first-run. is this > the intention? if so, please move it into browser/ui/startup and rename it. > otherwise, change ProcessLaunchURLs so that the new code is only reached on > first-run. Done. https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:574: // Don't open any browser windows if we're starting up in "background mode". On 2016/08/23 at 10:36:02, grt (UTC plus 2) wrote: > nit: i'm in the "no personal pronouns" camp (https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/BYMa_...). if it makes sense, please see if any comments with "we" and such in them can be reworded for clarity without the pronoun. for example, this could be: > // Don't open any browser windows when the process is being started in background mode. Done. https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:578: // If Master Prefs contains any first run tabs, use only those. On 2016/08/23 10:36:02, grt (UTC plus 2) wrote: > in the old logic, URLs on the command-line suppress first_run_tabs. Good catch; done. https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:580: if (browser_creator_) { On 2016/08/23 at 10:36:02, grt (UTC plus 2) wrote: > && !browser_creator_->first_run_tabs_.empty() Ack, see thread about ProcessMasterPrefsTabs in first_run.cc. https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:581: tabs_to_insert = Note that this logic has been moved to startup_tab_provider, but I'll respond here anyway. On 2016/08/23 at 10:36:02, grt (UTC plus 2) wrote: > if you're assuming that first_run_tabs_ will never be non-empty on non-first-run, please add a DCHECK for this: > DCHECK(first_run_); My goal here is to enforce the policy "master preferences only apply on first run." I'm not assuming that they're empty at other times--they should be, but I'm not relying on that assumption. On 2016/08/23 at 10:36:02, grt (UTC plus 2) wrote: > also add: > DCHECK(process_startup); > to document/assert the expectation that first_run_tabs_ will only be non-empty during process startup and not during rendezvous Can you elaborate on that assertion? Is this based on the idea that we clear their values in this function, and the first call to this function should always occur during process startup? Or is there some other reason to believe they're only populated at process startup that would be risky to ignore? https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:592: On 2016/08/23 at 10:36:02, grt (UTC plus 2) wrote: > is it intentional that the NTP is no longer opened? No, good catch. Thanks. https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:615: if (!browser) { On 2016/08/23 at 10:36:02, grt (UTC plus 2) wrote: > nit: omit braces N/A https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:630: return NULL; On 2016/08/23 at 10:36:02, grt (UTC plus 2) wrote: > nit: nullptr Done (throughout)
i like where this is headed. i can see how individual pieces can be more throughly tested, which is a good thing(TM). do you plan to add the session restore and other logic now or later? https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:581: tabs_to_insert = On 2016/09/08 21:33:16, tmartino wrote: > Note that this logic has been moved to startup_tab_provider, but I'll respond > here anyway. > > On 2016/08/23 at 10:36:02, grt (UTC plus 2) wrote: > > if you're assuming that first_run_tabs_ will never be non-empty on > non-first-run, please add a DCHECK for this: > > DCHECK(first_run_); > > My goal here is to enforce the policy "master preferences only apply on first > run." I'm not assuming that they're empty at other times--they should be, but > I'm not relying on that assumption. > > On 2016/08/23 at 10:36:02, grt (UTC plus 2) wrote: > > also add: > > DCHECK(process_startup); > > to document/assert the expectation that first_run_tabs_ will only be non-empty > during process startup and not during rendezvous > > Can you elaborate on that assertion? Is this based on the idea that we clear > their values in this function, and the first call to this function should always > occur during process startup? Or is there some other reason to believe they're > only populated at process startup that would be risky to ignore? when i wrote that, i must have been thinking "how can you have first_run_tabs if this isn't process startup since you'd have already gone through first run?" i can imagine it's possible that the first launch of the browser does something like start background mode or launch an app or something, and that it's only during rendezvous that the user has what we think of as the "first run" browser experience. it's a bit odd to think about, but as long as there are ways to start the browser without bringing the user through the FRE then you're right, first_run_tabs may be non-empty in the case of rendezvous. this also means we shouldn't drop the First Run sentinel until we actually bring the user through the experience. maybe this is already the case, i don't remember. https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/first_r... File chrome/browser/first_run/first_run.cc (right): https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/first_r... chrome/browser/first_run/first_run.cc:75: #include "base/win/windows_version.h" unused? https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/first_r... chrome/browser/first_run/first_run.cc:653: bool has_force_switch, nit: has_force_switch -> force_first_run, has_suppress_switch -> no_first_run the fact that they're obtained from command line switches is immaterial for this function. https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/first_r... File chrome/browser/first_run/first_run.h (right): https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/first_r... chrome/browser/first_run/first_run.h:96: internal::FirstRunState DetermineFirstRunState(bool has_sentinel, can you move this into first_run_internal.h and put the impl inside the internal namespace in first_run.cc? the unittest already #includes _internal.h and tests various internal:: funcs https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/first_r... File chrome/browser/first_run/first_run_unittest.cc (right): https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/first_r... chrome/browser/first_run/first_run_unittest.cc:97: internal::FirstRunState result = DetermineFirstRunState(true, true, true); should all permutations of (x, true, y) return FIRST_RUN_TRUE? if so, please test them all https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:578: std::unique_ptr<StartupTabProvider> provider(new StartupTabProviderImpl()); can you put this on the stack here: StartupTabProviderImpl provider; or just put the object in the call below: tabs = DetermineStartupTabs(StartupTabProviderImpl(), ...); the latter is my preference since it keeps the object in the narrowest scope possible or will this get more complicated with other provider impls? https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:582: if (cmd_line_urls.empty()) { nit: i think the code will follow the comment above more naturally if you invert the logic here: if (!empty) {...} else {...} https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:605: process_startup ? chrome::startup::IS_PROCESS_STARTUP nit: inline this below? https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:611: StartupTabProvider* provider, if this doesn't mutate the provider, pass it by constref https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:613: bool is_crash) { is_crash -> is_post_crash_launch? https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:621: provider->GetMasterPreferencesTabs(browser_creator_); i think it should be more clear here that this is the first_run_tabs and that they're only processed for first_run. how about changing "MasterPreferencesTabs" with "DistributionFirstRunTabs" since they are the first-run-tabs provided by a distribution? also, does it make sense to have the if (first run) check up here rather than hidden in the impl? https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:621: provider->GetMasterPreferencesTabs(browser_creator_); do you need a nullptr check on browser_creator_ either here or within GetMasterPreferencesTabs? https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:624: tabs.insert(tabs.end(), master_prefs_tabs.begin(), to avoid the extra churn here, what do you think of changing the method above so that it takes the container as an in/out param and returns a size_t of the # of items added (or just a bool if anything was added). then lines 620-627 would become: if (provider->AddDistributionFirstRunTabs(browser_creator_, &tabs)) { return tabs; } https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:629: StartupTabs onboarding_tabs = provider->GetOnboardingTabs(); how about all of these being Add...(&tabs);? lots of unneeded heap churn here. https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:642: StartupTabs pinned_tabs = provider->GetPinnedTabs(); can this be Add, too, with it happening before Onboarding tabs so that it doesn't need to insert at the beginning of the array? otherwise, if it really does need to happen the end of processing, how about calling it "PrependPinnedTabs". since inserting into the beginning of an array is so horrendous, the impl could do something like: void PrependPinnedTabs(StartupTabs* tabs) { StartupTabs pinned_tabs = ...whatever; pinned_tabs.insert(pinned_tabs.end(), tabs->begin(), tabs->end()); tabs->swap(pinned_tabs); } https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.h (right): https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.h:117: // is necessary, and opens the content a new or restored browser accordingly. "opens the content"? https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.h:124: // Returns the tabs to be shown on startup, based on the policy functions ...on startup when no URLs are provided on the command line, based on... https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc (right): https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:36: StartupTab onboarding_tab{GURL("https://onboarding"), false}; nit: prefer ctor syntax over uniform syntax as per https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a.... https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:37: StartupTabs onboarding_result; better yet, roll these three lines into one: StartupTabs onboarding_result = {StartupTab(GURL("https://onboarding"), false)} https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:39: ON_CALL(*provider, GetOnboardingTabs()) by using ON_CALL, this mock object isn't actually enforcing any expectations; i.e., the test won't fail on account of these methods not being called (though it may fail due to the wrong return values being used). if you really don't mean to assert that the method(s) are called, perhaps a fake would be simpler than a mock and easier to understand for those who dislike gmock (there are many). https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:80: PrepMockProvider(&provider, true, false, true, true, true); while PrepMockProvider may reduce pure lines-of-code, it unfortunately serves to obscure what the test is actually doing. in cases like this, i think it's better to make the test as clear as possible. this is in your best interest as well, since it makes it more likely that someone breaking the test in the future will be able to figure out what's going on without having to ask you. :-) i wonder if a fake would be easier to use and understand. imagine for example: // Bits for provider options. constexpr uint32_t kOnboardingTabs = 0x01; constexpr uint32_t kFirstRunTabs = 0x02; constexpr uint32_t kResetTrigger = 0x04; constexpr uint32_t kPinnedTabs = 0x08; constexpr uint32_t kPrefsTabs = 0x10; class FakeStartupTabProvider : public StartupTabProvider { public: explicit FakeStartupTabProvider(uint32_t options) : options_(options) {} StartupTabs GetOnboardingTabs() override { if (options_ & kOnboardingTabs) return StartupTabs{StartupTab(GURL("https://onboarding"), false)}; return StartupTabs(); } then in the test functions we have things like: FakeStartupTabProvider provider(kOnboardingTabs | kResetTrigger | kPinnedTabs | kPrefsTabs); then it's clear in the test what is being done, and the impls of the various provider functions are also quite straightforward. https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:81: base::CommandLine dummy(base::CommandLine::NO_PROGRAM); inline this in the call below rather than introducing a long-lived local var https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab.cc (right): https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab.cc:12: : url(url_), is_pinned(is_pinned_) {} i was going to suggest using std::move to get the url into place, but it looks to me like GURL isn't currently movable. how about taking the url as constref? https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab.h (right): https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab.h:8: #include <string> nit: remove this unused include while you're here https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab.h:16: StartupTab(GURL url_, bool is_pinned_); no '_' in parameter names. https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab_provider.cc (right): https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:73: // Private static implementation methods: remove this https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:75: StartupTabs StartupTabProviderImpl::CheckStandardOnboardingTabPolicy( nit: there's a convention of putting a simple // static comment on the line immediately preceding static member function definitions. https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:88: for (const GURL& url : first_run_tabs) { nit: processed_tabs.reserve(first_run_tabs.size()); https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab_provider.h (right): https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.h:22: StartupTabProvider() {} nit: = default; rather than {} here and on next line https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.h:26: virtual StartupTabs GetOnboardingTabs() = 0; could these all be const methods? https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.h:48: ~StartupTabProviderImpl() override {} this class has no state, so it doesn't need to override the dtor https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.h:63: FRIEND_TEST_ALL_PREFIXES(StartupTabProviderTest, since this is an "impl" class, i don't see a problem with making the fns above public so that this noise can go away. https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.h:73: }; DISALOW_COPY_AND_ASSIGN
Good stuff; assume everything is "done" unless I've replied otherwise. Re: session restore and other logic, I'm planning to fill in the remaining TODOs in subsequent CL(s). My reasoning is that I'm getting a little uneasy about letting the scope of this CL get any bigger, and there's no apparent risk to landing it in chunks. https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2164033002/diff/120001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:581: tabs_to_insert = On 2016/09/09 11:40:29, grt (UTC plus 2) wrote: > this also means we > shouldn't drop the First Run sentinel until we actually bring the user through > the experience. maybe this is already the case, i don't remember. Two things of note: 1. IsChromeFirstRun uses a cached value, so even after the sentinel is created, IsChromeFirstRun will return true until the process terminates. 2. For Onboarding Tabs, I'm increasingly convinced we don't actually want to base our logic entirely on this sentinel. We've got a lot of edge cases, for example: * We recently decided we want to show the welcome page to all new profiles, not just the first execution on this machine. * We want to defer until second run if URLs are passed in via command line. I think the proper logic might be to add a local state value that represents "has this profile been shown the welcome page?". The only problem with this approach is how we avoid showing this to upgrading users. WDYT? https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:578: std::unique_ptr<StartupTabProvider> provider(new StartupTabProviderImpl()); On 2016/09/09 at 11:40:30, grt (UTC plus 2) wrote: > can you put this on the stack here: > StartupTabProviderImpl provider; > or just put the object in the call below: > tabs = DetermineStartupTabs(StartupTabProviderImpl(), ...); > the latter is my preference since it keeps the object in the narrowest scope possible > > or will this get more complicated with other provider impls? My understanding is that this needs to be a pointer if we are to access the virtual methods in StartupTabProvider. (This is supported by the boatload of linker errors I get when I try changing it to pass an object.) https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:611: StartupTabProvider* provider, On 2016/09/09 at 11:40:30, grt (UTC plus 2) wrote: > if this doesn't mutate the provider, pass it by constref See discussion above re: pointers. However, I did change this to const (along with all the methods in the Provider). https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:621: provider->GetMasterPreferencesTabs(browser_creator_); On 2016/09/09 at 11:40:29, grt (UTC plus 2) wrote: > also, does it make sense to have the if (first run) check up here rather than hidden in the impl? I think it does. In my mind, the fact that this only applies on first run is a policy detail specific to DistributionFirstRun, because the is (first run) check applies to some policy categories (DistributionFirstRun, Onboarding) but not others (ResetTrigger, Pinned, Preferences). My hope is to reserve this function only for: * Cases where one set of policies affects another (e.g., DistributionFirstRun blocks all others) * Logic that applies to ALL policies (i.e., incognito/crash blocks everything) It also moves it down to a static function where it's more easily tested. https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab.h (right): https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab.h:16: StartupTab(GURL url_, bool is_pinned_); On 2016/09/09 at 11:40:30, grt (UTC plus 2) wrote: > no '_' in parameter names. Changed to url_val and is_pinned_val, which feels icky. Is there a convention for this situation?
It's difficult to validate that there's no behavior change here since the old code was such spaghetti. Do you have a doc where you have decomposed the pasta so that you know nothing is being left out or otherwise changed? https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:578: std::unique_ptr<StartupTabProvider> provider(new StartupTabProviderImpl()); On 2016/09/10 00:19:41, tmartino wrote: > On 2016/09/09 at 11:40:30, grt (UTC plus 2) wrote: > > can you put this on the stack here: > > StartupTabProviderImpl provider; > > or just put the object in the call below: > > tabs = DetermineStartupTabs(StartupTabProviderImpl(), ...); > > the latter is my preference since it keeps the object in the narrowest scope > possible > > > > or will this get more complicated with other provider impls? > > My understanding is that this needs to be a pointer if we are to access the > virtual methods in StartupTabProvider. (This is supported by the boatload of > linker errors I get when I try changing it to pass an object.) Were you passing by value or by constref? The latter is what I meant, and should work. If not, what were the errors? https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab.h (right): https://codereview.chromium.org/2164033002/diff/160001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab.h:16: StartupTab(GURL url_, bool is_pinned_); On 2016/09/10 00:19:41, tmartino wrote: > On 2016/09/09 at 11:40:30, grt (UTC plus 2) wrote: > > no '_' in parameter names. > > Changed to url_val and is_pinned_val, which feels icky. Is there a convention > for this situation? url and is_pinned will work, no? https://codereview.chromium.org/2164033002/diff/180001/chrome/browser/first_r... File chrome/browser/first_run/first_run.h (right): https://codereview.chromium.org/2164033002/diff/180001/chrome/browser/first_r... chrome/browser/first_run/first_run.h:14: #include "chrome/browser/first_run/first_run_internal.h" move back to the .cc file https://codereview.chromium.org/2164033002/diff/180001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2164033002/diff/180001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:584: std::unique_ptr<StartupTabProvider> provider(new StartupTabProviderImpl()); regardless of whether you pass this instance to callees by pointer or by constref, there should be no need to create it on the heap rather than on the stack. https://codereview.chromium.org/2164033002/diff/180001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:618: if (!is_incognito && !is_post_crash_launch) { the body of this is so much easier to read than the old code! https://codereview.chromium.org/2164033002/diff/180001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:626: provider->AddPinnedTabs(&tabs); how about adding a very brief comment for each of these calls to explain why they do or don't add any tabs? it saves the reader having to bounce around to the impls to understand what will happen. https://codereview.chromium.org/2164033002/diff/180001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:637: tabs.push_back(StartupTab(GURL(chrome::kChromeUINewTabURL), 0)); 0 -> false https://codereview.chromium.org/2164033002/diff/180001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:642: Browser* StartupBrowserCreatorImpl::RestoreOrCreateBrowser(StartupTabs tabs) { pass by constref https://codereview.chromium.org/2164033002/diff/180001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:646: return OpenTabsInBrowser(nullptr, true, tabs); why is |true| correct here rather than passing process_startup on down? https://codereview.chromium.org/2164033002/diff/180001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab_provider.cc (right): https://codereview.chromium.org/2164033002/diff/180001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:13: #if defined(OS_WIN) #include "build/build_config.h" to define OS_WIN https://codereview.chromium.org/2164033002/diff/180001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:48: StartupTabs insert_tabs; how about shortcutting the special case: if (!browser_creator) return false; StartupTabs insert_tabs = ... https://codereview.chromium.org/2164033002/diff/180001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:64: if (triggered_profile_resetter) { nit: omit braces https://codereview.chromium.org/2164033002/diff/180001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:86: StartupTabs StartupTabProviderImpl::CheckStandardOnboardingTabPolicy( there's room to get rid of additional copies by making these static methods append to a given StartupTabs instance. will that somehow make things much uglier?
I've been working off my chicken-scratch flowchart so I went back and turned it into a doc describing the flow of these functions in finer detail: https://docs.google.com/a/google.com/document/d/1CNaThe5bvTipaTzRV9FyCTDSOazr... Incidentally, this was a nice exercise to remind myself why this refactor was necessary :) https://codereview.chromium.org/2164033002/diff/180001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab_provider.cc (right): https://codereview.chromium.org/2164033002/diff/180001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:86: StartupTabs StartupTabProviderImpl::CheckStandardOnboardingTabPolicy( On 2016/09/12 at 07:28:46, grt (UTC plus 2) wrote: > there's room to get rid of additional copies by making these static methods append to a given StartupTabs instance. will that somehow make things much uglier? Initially I was hesitant to use the adder pattern anywhere--I like using a return because it structurally enforces that policy functions can't depending on prior functions to change their own behavior. Adding the StartupTabs* argument also means someone can come in and start reading from that value. That said, I think we've reached a structure that at least discourages this, and I do agree that the performance concerns are legitimate. I'll also rename the arguments from "tabs" to "output" to drive the point home :)
That doc is amazing and terrifying. :-) Could you transform it into the expected behavior for each scenario? For example: - first run w/ master prefs - regular new-process launch with nothing on the command line - session restore... - post-crash launch.. - etc From there, I hope it's possible to make unit tests for the new code to cover each of those scenarios. https://codereview.chromium.org/2164033002/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2164033002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:620: // tabs to be displayed on First Run, overriding any other tabs which would why use caps for First Run and Onboarding? https://codereview.chromium.org/2164033002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:631: // Polciies for Onboarding (e.g., First Run) may show promotional and Policies https://codereview.chromium.org/2164033002/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab_provider.cc (right): https://codereview.chromium.org/2164033002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:115: if (profile_has_trigger) { nit: reverse the logic to get rid of the braces: if (!...) return false; foo; return true; https://codereview.chromium.org/2164033002/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab_provider.h (right): https://codereview.chromium.org/2164033002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.h:26: // run policy, and appends them to |output|. Returns true iff any tabs were "output" doesn't say anything about what it is. i think |tabs| is better. it's our coding style that out params are non-const pointers at the end of parameter lists (https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering), so i think there's little risk of someone not getting that |tabs| is the thing to which tabs may or may not be added.
Makes sense; I'll try to update the doc tomorrow. Done to all comments. https://codereview.chromium.org/2164033002/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab_provider.h (right): https://codereview.chromium.org/2164033002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.h:26: // run policy, and appends them to |output|. Returns true iff any tabs were On 2016/09/14 at 11:21:22, grt (UTC plus 2) wrote: > "output" doesn't say anything about what it is. i think |tabs| is better. it's our coding style that out params are non-const pointers at the end of parameter lists (https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering), so i think there's little risk of someone not getting that |tabs| is the thing to which tabs may or may not be added. My concern was more that people editing these functions later would start examining the prior content of |tabs| and using it to dictate policy, which lead to some spaghetti in the past. However, I can see |output| didn't communicate that clearly anyway, so I've switched it back.
https://codereview.chromium.org/2164033002/diff/220001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab_provider.cc (right): https://codereview.chromium.org/2164033002/diff/220001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:115: if (!profile_has_trigger) { nit: omit braces
Here's the flattened behavior (the best that I could trace it): https://docs.google.com/a/google.com/spreadsheets/d/1GAimMY1DERzkQ-0IZ5qa-cu0... https://codereview.chromium.org/2164033002/diff/220001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab_provider.cc (right): https://codereview.chromium.org/2164033002/diff/220001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:115: if (!profile_has_trigger) { On 2016/09/16 at 07:05:31, grt (UTC plus 2) wrote: > nit: omit braces Sorry. Old habits die hard.
I think there are some changes in behavior; see below. Are they intentional? If so, could you make sure they're stated explicitly in the design doc? https://codereview.chromium.org/2164033002/diff/220001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2164033002/diff/220001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:578: // If command-line URLs were passed, they supercede all other policies and at least some(!) of the old codepaths put the settings reset tab in front of URLs passed on the command line. please check with robertshield to see if that was intentional and if it should be carried forward. off the top of my head, i think it should. https://codereview.chromium.org/2164033002/diff/220001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:622: if (provider.AddDistributionFirstRunTabs(browser_creator_, &tabs)) { nit: omit braces :-) https://codereview.chromium.org/2164033002/diff/220001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:638: provider.AddResetTriggerTabs(profile_, &tabs); i think the settings reset page is supposed to be the first tab the user sees, so it should be the first one added, no? https://codereview.chromium.org/2164033002/diff/220001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:647: tabs.push_back(StartupTab(GURL(chrome::kChromeUINewTabURL), false)); the old code would add the NTP if no URLs were specified on the command line or in first_run_tabs. this also suppresses it if the settings reset page is shown or if tabs are pinned. is this intentional?
On 2016/09/21 at 08:59:08, grt wrote: > I think there are some changes in behavior; see below. Are they intentional? If so, could you make sure they're stated explicitly in the design doc? > > https://codereview.chromium.org/2164033002/diff/220001/chrome/browser/ui/star... > File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): > > https://codereview.chromium.org/2164033002/diff/220001/chrome/browser/ui/star... > chrome/browser/ui/startup/startup_browser_creator_impl.cc:578: // If command-line URLs were passed, they supercede all other policies and > at least some(!) of the old codepaths put the settings reset tab in front of URLs passed on the command line. please check with robertshield to see if that was intentional and if it should be carried forward. off the top of my head, i think it should. > > https://codereview.chromium.org/2164033002/diff/220001/chrome/browser/ui/star... > chrome/browser/ui/startup/startup_browser_creator_impl.cc:622: if (provider.AddDistributionFirstRunTabs(browser_creator_, &tabs)) { > nit: omit braces :-) > > https://codereview.chromium.org/2164033002/diff/220001/chrome/browser/ui/star... > chrome/browser/ui/startup/startup_browser_creator_impl.cc:638: provider.AddResetTriggerTabs(profile_, &tabs); > i think the settings reset page is supposed to be the first tab the user sees, so it should be the first one added, no? > > https://codereview.chromium.org/2164033002/diff/220001/chrome/browser/ui/star... > chrome/browser/ui/startup/startup_browser_creator_impl.cc:647: tabs.push_back(StartupTab(GURL(chrome::kChromeUINewTabURL), false)); > the old code would add the NTP if no URLs were specified on the command line or in first_run_tabs. this also suppresses it if the settings reset page is shown or if tabs are pinned. is this intentional? OK, I've restructured DetermineStartupTabs to undo those behavior changes, and added tests for each of them. One of the changes to DetermineStartupTabs that enabled these changes is that the command line tabs are now passed in as an argument. I think this is a good change anyway, as it's more consistent with the meta-policy purpose of that function. One thing that may look like a behavior change in this patchset is that I no longer put pinned tabs at the beginning of the list; in fact, they will be hoisted to the left when opened anyway, and by placing them later we can ensure that the 0th tab (which could be the settings reset page, NTP, etc.) will appear as the active content even when pinned tabs are present.
This is looking pretty good, though it looks like some complexity is creeping into DetermineStartupTabs since patch set 10. An idea popped into my head while reading this one. I'm curious to know what you think: Imagine drawing an imaginary tabstrip with tabs from every conceivable set of sources represented (command line, master prefs, URLs pref, session restore, profile resetter, promos, NTP, etc). I *think* the intent is that the relative positions of these tab categories is consistent across every conceivable type of launch (if it isn't the case today, it probably should be). Now, what if we had a bitfield where each bit represents one category. Is it possible to have a bunch of policy that sets/clears these bits based on the various inputs, and then a separate function that iterates through the bitfield and adds the respective tabs with absolutely no fancy policy logic whatsoever? One advantage of this approach is that it becomes very hard to accidentally disturb the desired ordering. I think it also nicely breaks up what the next developer needs to think about: "Where should my new startup tab flavor go with respect to the other types of tabs, and under what conditions should my new flavor be shown (or suppressed)?" I haven't tried thinking about each twisty scenario, but I wonder if this would simplify things. What do you think? https://codereview.chromium.org/2164033002/diff/260001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2164033002/diff/260001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:617: return ntp_only; construct the vector with the item as you return it: return StartupTabs({StartupTab(GURL(chrome::kChromeUINewTabURL), false)}); or, failing that: return StartupTabs(1, StartupTab(GURL(chrome::kChromeUINewTabURL), false)); and if the first one works, you don't need braces for this if...else. :-) https://codereview.chromium.org/2164033002/diff/260001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:630: return tabs; nit: braces around this now since the conditional spans multiple lines. (gotcha!) https://codereview.chromium.org/2164033002/diff/260001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:639: if (cmd_line_tabs.empty()) { since the comment above refers to the handling of non-empty |cmd_line_tabs|, please flip the logic here and early-exit as above: if (!cmd_line_tabs.empty()) { tabs.insert(...); return tabs; } no need to move the comment on line 658 up here since the comment above makes it superfluous.
On 2016/09/26 at 09:27:02, grt wrote: > This is looking pretty good, though it looks like some complexity is creeping into DetermineStartupTabs since patch set 10. An idea popped into my head while reading this one. I'm curious to know what you think: > > Imagine drawing an imaginary tabstrip with tabs from every conceivable set of sources represented (command line, master prefs, URLs pref, session restore, profile resetter, promos, NTP, etc). I *think* the intent is that the relative positions of these tab categories is consistent across every conceivable type of launch (if it isn't the case today, it probably should be). Now, what if we had a bitfield where each bit represents one category. Is it possible to have a bunch of policy that sets/clears these bits based on the various inputs, and then a separate function that iterates through the bitfield and adds the respective tabs with absolutely no fancy policy logic whatsoever? One advantage of this approach is that it becomes very hard to accidentally disturb the desired ordering. I think it also nicely breaks up what the next developer needs to think about: "Where should my new startup tab flavor go with respect to the other types of tabs, and under what conditions should my new flavor be shown (or suppressed)?" > > I haven't tried thinking about each twisty scenario, but I wonder if this would simplify things. What do you think? > > https://codereview.chromium.org/2164033002/diff/260001/chrome/browser/ui/star... > File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): > > https://codereview.chromium.org/2164033002/diff/260001/chrome/browser/ui/star... > chrome/browser/ui/startup/startup_browser_creator_impl.cc:617: return ntp_only; > construct the vector with the item as you return it: > return StartupTabs({StartupTab(GURL(chrome::kChromeUINewTabURL), false)}); > or, failing that: > return StartupTabs(1, StartupTab(GURL(chrome::kChromeUINewTabURL), > false)); > > and if the first one works, you don't need braces for this if...else. :-) > > https://codereview.chromium.org/2164033002/diff/260001/chrome/browser/ui/star... > chrome/browser/ui/startup/startup_browser_creator_impl.cc:630: return tabs; > nit: braces around this now since the conditional spans multiple lines. (gotcha!) > > https://codereview.chromium.org/2164033002/diff/260001/chrome/browser/ui/star... > chrome/browser/ui/startup/startup_browser_creator_impl.cc:639: if (cmd_line_tabs.empty()) { > since the comment above refers to the handling of non-empty |cmd_line_tabs|, please flip the logic here and early-exit as above: > if (!cmd_line_tabs.empty()) { > tabs.insert(...); > return tabs; > } > no need to move the comment on line 658 up here since the comment above makes it superfluous. Hmm, I can definitely see the concern about complexity creep--but I'm increasingly convinced the problem is the policy itself, and not the expression of it. The edge cases that have caused the complexity creep are largely defined by desired relationships between categories (e.g., NTP should coexist with reset trigger and pinned tabs, but not with command-line, onboarding, or prefs tabs), and I think we're now those relationships quite directly: if (!cmd_line_tabs.empty()) { [...] } else { [...] if (!onboarding_added && !prefs_added) tabs.push_back(/* NTP */) } I'm also happy that the structure we've been working on has been flexible enough to accommodate those edge cases without any system state reads leaking into the same layer, and that we now have unit tests addressing these second-order rules directly. In my opinion, a bitmask would net the same level of complexity--we would basically be compiling a list of sentinels that nominally aren't tabs, and exchanging them later for the relevant tabs. The code determining which sentinels to include would have to be logically equivalent to what we have right now, and I couldn't see any way of permuting or reorganizing that logic that's simpler than what I have now. I'll agree that adding a second pass would give us more enforcement of the proper ordering of tabs. However, it would mean having to read some bits of system state in two separate places: once when determining whether to flip the bit, and once when determining what that flipped bit means. For instance, we have to read browser_creator->first_run_tabs_.empty() here to decide if Master Prefs content is included, and then we'd have to iterate over browser_creator->first_run_tabs_ in the adding phase to transform the URLs and keywords into StartupTab objects. Ultimately I think that divides up logic that we've put in a lot of effort to consolidate. I also don't think disrupting the ordering is a particularly big risk: as I've written it, DetermineStartupTabs is already working in strict order; there are some ifs/elses/early-exits, but line n+1 only ever appends to the list of tabs that existed on line n--never prepends, reads, or mutates.
makes sense. thanks for the detailed response. lgtm. https://codereview.chromium.org/2164033002/diff/260001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2164033002/diff/260001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:639: if (cmd_line_tabs.empty()) { On 2016/09/26 09:27:02, grt (UTC plus 2) wrote: > since the comment above refers to the handling of non-empty |cmd_line_tabs|, > please flip the logic here and early-exit as above: > if (!cmd_line_tabs.empty()) { > tabs.insert(...); > return tabs; > } > no need to move the comment on line 658 up here since the comment above makes it > superfluous. fwiw, i prefer with the early return since it's consistent with the returns on line 627 and above. https://codereview.chromium.org/2164033002/diff/280001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc (right): https://codereview.chromium.org/2164033002/diff/280001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:12: class StartupBrowserCreatorImplTest : public testing::Test {}; i believe this is unused since you're using TEST rather than TEST_F below; remove it. https://codereview.chromium.org/2164033002/diff/280001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:14: // Bits for FakeStartupTabProvider options. nit: wrap lines 14-77 in an unnamed namespace https://codereview.chromium.org/2164033002/diff/280001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab_provider_unittest.cc (right): https://codereview.chromium.org/2164033002/diff/280001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider_unittest.cc:12: class StartupTabProviderTest : public testing::Test {}; unused; remove
The CQ bit was checked by tmartino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by tmartino@chromium.org
The CQ bit was checked by tmartino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by tmartino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tmartino@chromium.org changed reviewers: + pkasting@chromium.org
+pkasting for OWNERS on /ui/* Hi Peter, This is the first part of the StartupBrowserCreatorImpl refactor we discussed a few months ago. It's been through quite a few iterations, but the goals are still, broadly, to trim code branches, improve unit test coverage, and consolidate decision making on which tabs need to be opened. Let me know if you have any questions about the new structure, my goals, or the review cycles I've had with grt. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Don't be put off by the number of comments below, most things are minor. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:111: // TODO(crbug/642442): Remove this when first_run_tabs gets refactored. Tiny nit: "crbug/" only works on corp. I tend to prefer e.g. "crbug.com/#" (or just "bug #", or a username) https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:506: if (base::FeatureList::IsEnabled(features::kUseConsolidatedStartupFlow)) { Nit: I think it would be slightly clearer to move this condition to the caller of this method instead of implementing one function in terms of the other. This way you can mark this method as fully deprecated like the others below (see comments there). https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:583: bool is_crash = HasPendingUncleanExit(profile_); Nit: |is_post_crash_launch| would be more descriptive and match the naming you use below. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:592: // TODO(tmartino): Function which determines what behavior of session Nit: Function which determines -> Determine? (Not really sure if there was some reason you were calling out the "function" bit explicitly.) https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:615: return StartupTabs({StartupTab(GURL(chrome::kChromeUINewTabURL), false)}); Nit: Is there any way to avoid having both ( and { here? I'm not opposed to constructing things this way, I just don't usually use this syntax, so I don't know if either one of them could be omitted. Another way involves moving the |startup_tabs| declaration up: StartupTabs startup_tabs; if (is_incognito || is_post_crash_launch) { if (cmd_line_tabs.empty()) startup_tabs.emplace_back(GURL(chrome::kChromeUINewTabURL), false); return cmd_line_tabs; } I kinda prefer this honestly but up to you. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:616: else Nit: No else after return https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:654: tabs.push_back(StartupTab(GURL(chrome::kChromeUINewTabURL), false)); Nit: Up to you, but I tend to prefer emplace_back() here just to omit the type name that is not really important. It would be nice to figure out a way to rearrange the code so we weren't doing this both here and at the beginning of this function, but I don't know whether doing so is very feasible. Possibly if we broke this function into more helpers? Don't worry too hard about this. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:656: // Reads and adds any tabs which the user has previously pinned. Nit: Reads and adds -> Add? (For parallel structure) https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:754: // enabled. Nit: Some of these comments you put inside the function, some above. Be consistent. But perhaps even better, don't comment here, comment in the header instead, placing all the deprecated functions together in one big block with one comment that says they're all deprecated. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.h (right): https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.h:123: // that code path entirely once kUseConsolidatedStartupFlow is on by default. Nit: For this last sentence, consider instead putting a TODO at the end of the ProcessLaunchURLs comment saying something like: "Under the kUseConsolidatedStartupFlow feature, this is replaced by ProcessLaunchUrlsUsingConsolidatedFlow() below. TODO(...): Enable that feature by default and remove this." https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.h:136: // Opens the given tabs in a new browser. Nit: It's not clear from this comment how this is intended to be different than OpenTabsInBrowser(). Maybe for now don't add it, until later you have more functionality to put in which makes the need for a new function clear? https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc (right): https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:25: explicit FakeStartupTabProvider(uint32_t options) : options_(options) {} Nit: The options bitfield feels a little clever; would separate bools, maybe with /* onboarding tabs */ type comments at the constructor callsites, be any simpler? https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:28: if (options_ & kOnboardingTabs) { Nit: All these methods can drop the {} if you reverse the conditionals (because the bodies will be one line). https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:29: tabs->push_back(StartupTab(GURL("https://onboarding"), false)); Nit: Comment in startup_browser_creator_impl.cc about emplace_back() also applicable a number of places in this file https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:74: cmd_line_tabs.push_back(StartupTab(GURL("https://cmd-line"), false)); 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? https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:89: StartupTabs no_tabs; 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) https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:111: // Incognito Nit: Trailing period (several places) https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:115: EXPECT_EQ(GURL(chrome::kChromeUINewTabURL).host(), output[0].url.host()); Nit: Maybe omit .host() calls? (3 different places) https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_features.cc (right): https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_features.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. Nit: No (c) https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_features.h (right): https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_features.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. Nit: No (c) https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab.h (right): https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab.h:15: StartupTab(const GURL& url, bool is_pinned); Nit: Random check, is there a way to use C++11 uniform init to avoid needing this constructor? Or does that only work when we don't define any user-defined constructors? I'm somehow hoping that {foo, bar} would magically init the two members of this class. Probably not huh. (You can see, from the comments on this CL, just how often I use uniform init.) https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab_provider.cc (right): https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:23: constexpr char kNewTabUrlHost[] = "new_tab_page"; Nit: Prefer to declare constants in the narrowest scope possible, in this case in the one function below that uses these rather than at file scope. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:24: constexpr char kWelcomePageUrlHost[] = "welcome_page"; Oh dear... these are valid hostnames for intranets. We really should not use this method of specifying magic constants unless these are only stored in a place we know intranet URLs could never go, e.g. the URL is "chrome://new_tab_page/" with the chrome:// scheme clearly indicating this is an internal thing (but then why not just use chrome://newtab/ directly?). If fixing this involves some kind of migration, do it separately -- but if at all possible please do fix this during this process. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:28: #if defined(OS_WIN) Nit: If it wouldn't add excessive indirection/boilerplate/etc., consider a startup_tab_provider_win.cc that defines Windows-specific functionality in addition to/in place of things in this file, done in such a way that we don't end up using ifdefs in this file (there are a couple ways to do this). https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:57: #if defined(OS_WIN) Why does this code only run for Windows? it seems like HasResetTrigger() will never return true on other platforms, but is properly implemented on all and should be safe to Just Call Everywhere. That would simplify this function a bit. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:58: TriggeredProfileResetter* triggered_profile_resetter = Nit: Can this be const? auto* might reduce redundancy here https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:61: has_reset_trigger = triggered_profile_resetter->HasResetTrigger(); Nit: Or this. Not really different in this implementation, but useful to avoid needing a temp at all if you eliminate the #ifdef OS_WIN. has_reset_trigger = triggered_profile_resetter && triggered_profile_resetter->HasResetTrigger(); https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:83: tabs->push_back(StartupTab(GetWelcomePageUrl(), false)); Nit: Same emplace_back() comment applies in this file too https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:86: return false; Nit: Shorter: if (is_first_run) tabs->... return is_first_run; https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:99: tabs->push_back(StartupTab(GURL(chrome::kChromeUINewTabURL), false)); Nit: Rather than duplicate the construction of StartupTab and pushing it back, seems like it might be nicer to do something like: for (GURL url : first_run_tabs) { if (url.host() == kNewTabUrlHost) url = GURL(chrome::kChromeUINewTabURL); else if (url.host() == kWelcomePageUrlHost) url = GetWelcomePageUrl(); tabs->emplace_back(url, false); } https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:117: return true; Nit: Shorter: if (profile_has_trigger) tabs->... return profile_has_trigger; https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab_provider.h (right): https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.h:19: // be shown for first run/onboarding?" Nit: May want to note why this class is pure virtual and not just implemented here directly. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.h:23: virtual ~StartupTabProvider() = default; 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. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.h:28: virtual bool AddOnboardingTabs(StartupTabs* tabs) const = 0; 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()) ... https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.h:32: // the value of first_run_urls_ in the provided BrowserCreator. Returns true 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. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.h:41: virtual bool AddResetTriggerTabs(Profile* profile, Nit: Can |profile| be const? https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.h:44: // Reads tabs from the user's pinned tabs, and appends them to |tabs|. 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 :) https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.h:48: // Reads tabs specified for new windows from the user's preferences, and Nit: What is a "tab specified for [a] new window"? As distinct, presumably, from tabs specified for other things? https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.h:56: bool AddOnboardingTabs(StartupTabs* tabs) const override; 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. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.h:63: static bool CheckStandardOnboardingTabPolicy(bool is_first_run, Nit: What do these functions do? Add comments. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.h:71: DISALLOW_COPY_AND_ASSIGN(StartupTabProviderImpl); 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. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.h:76: GURL GetWelcomePageUrl(); 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? https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.h:77: GURL GetTriggeredResetSettingsUrl(); Nit: Blank line + comment above this? I don't know what a "triggered reset settings URL" is. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab_provider_unittest.cc (right): https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider_unittest.cc:18: EXPECT_EQ(GetWelcomePageUrl().host(), output[0].url.host()); Nit: Eliminate host() calls? (2 different places) https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider_unittest.cc:36: input.push_back(GURL(base::ASCIIToUTF16("https://welcome_page"))); Nit: Use = {..., ..., ...} to init vector (2 places) https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider_unittest.cc:45: EXPECT_EQ("www.google.com", output[1].url.host()); Nit: EXPECT_EQ(input[1], output[1].url)? (2 places) https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider_unittest.cc:78: StartupTabProviderImpl::CheckResetTriggerTabPolicy(false, &output); Nit: You put a blank line below this in the above cases, be consistent
Thanks Peter! Assume everything is done unless otherwise specified. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:592: // TODO(tmartino): Function which determines what behavior of session On 2016/09/29 at 07:23:34, Peter Kasting (busy Sep 29) wrote: > Nit: Function which determines -> Determine? (Not really sure if there was some reason you were calling out the "function" bit explicitly.) The reason was simply to give a better idea of the intended future structure: the plan is to separate the "determine behavior" logic into its own function that shouldn't interact directly with any state (and should therefore be unit-testable). It will mirror what's been done with the tab logic. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:615: return StartupTabs({StartupTab(GURL(chrome::kChromeUINewTabURL), false)}); On 2016/09/29 at 07:23:33, Peter Kasting (busy Sep 29) wrote: > Nit: Is there any way to avoid having both ( and { here? I'm not opposed to constructing things this way, I just don't usually use this syntax, so I don't know if either one of them could be omitted. > > Another way involves moving the |startup_tabs| declaration up: > > StartupTabs startup_tabs; > if (is_incognito || is_post_crash_launch) { > if (cmd_line_tabs.empty()) > startup_tabs.emplace_back(GURL(chrome::kChromeUINewTabURL), false); > return cmd_line_tabs; > } > > I kinda prefer this honestly but up to you. I've tried this a few ways (this way was actually Greg's suggestion) and at the end of the day I found this to most clearly/directly express what's going on here. Every other method (e.g., vector<Foo> foolist = {foo1};) involves doing some local storage and manipulation, when what we're really trying to do is say "just bomb out and return this thing that's basically a constant". It's a little unaesthetic, but I think it makes the most sense to do it inline. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:654: tabs.push_back(StartupTab(GURL(chrome::kChromeUINewTabURL), false)); On 2016/09/29 at 07:23:34, Peter Kasting (busy Sep 29) wrote: > Nit: Up to you, but I tend to prefer emplace_back() here just to omit the type name that is not really important. > > It would be nice to figure out a way to rearrange the code so we weren't doing this both here and at the beginning of this function, but I don't know whether doing so is very feasible. Possibly if we broke this function into more helpers? Don't worry too hard about this. Changed to emplace_back here and throughout startup_tab_provider.cc as well. On the latter point, grt and I have put a lot of thought and iterations into this and this is really the best we've come up with--I discussed this extensively in message #34, if you're interested in the rationale. My goal with this method was to express, as directly as possible, the relationships between various policies, and in this case there are really two different policies at play: #1. In incognito/crash, exit early and only show the "constant" list of just the NTP. #2. The NTP should be added to the list of tabs depending on the results of certain other policies. I found it best to express those two separately, rather than trying to somehow cram them together in a more general way when they aren't really saying the same thing despite sharing a line of code. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc (right): https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:25: explicit FakeStartupTabProvider(uint32_t options) : options_(options) {} On 2016/09/29 at 07:23:34, Peter Kasting (busy Sep 29) wrote: > Nit: The options bitfield feels a little clever; would separate bools, maybe with /* onboarding tabs */ type comments at the constructor callsites, be any simpler? 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. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:28: if (options_ & kOnboardingTabs) { On 2016/09/29 at 07:23:34, Peter Kasting (busy Sep 29) wrote: > Nit: All these methods can drop the {} if you reverse the conditionals (because the bodies will be one line). 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. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:111: // Incognito On 2016/09/29 at 07:23:34, Peter Kasting (busy Sep 29) wrote: > Nit: Trailing period (several places) Fragment, so went with colon instead. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab.h (right): https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab.h:15: StartupTab(const GURL& url, bool is_pinned); On 2016/09/29 at 07:23:34, Peter Kasting (busy Sep 29) wrote: > Nit: Random check, is there a way to use C++11 uniform init to avoid needing this constructor? Or does that only work when we don't define any user-defined constructors? I'm somehow hoping that {foo, bar} would magically init the two members of this class. Probably not huh. > > (You can see, from the comments on this CL, just how often I use uniform init.) Believe me, I tried. :) But unfortunately, having any constructors breaks uniform init, and the default constructor (which, oddly, defaults to pinned = true) is used in a number of places outside the scope of this change. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab_provider.cc (right): https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:24: constexpr char kWelcomePageUrlHost[] = "welcome_page"; On 2016/09/29 at 07:23:35, Peter Kasting (busy Sep 29) wrote: > Oh dear... these are valid hostnames for intranets. We really should not use this method of specifying magic constants unless these are only stored in a place we know intranet URLs could never go, e.g. the URL is "chrome://new_tab_page/" with the chrome:// scheme clearly indicating this is an internal thing (but then why not just use chrome://newtab/ directly?). > > If fixing this involves some kind of migration, do it separately -- but if at all possible please do fix this during this process. I hate, hate, hate this too. I was horrified when I came across this logic when untangling the spaghetti. Unfortunately, I don't think there's anything that can be done about it. These don't seem to be defined anywhere else--it's entirely by convention--but they're used in Master Preferences files created by external partners and administrators, and bundled with Chrome in various distributions. https://support.google.com/chrome/a/answer/187948?hl=en https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:28: #if defined(OS_WIN) On 2016/09/29 at 07:23:35, Peter Kasting (busy Sep 29) wrote: > Nit: If it wouldn't add excessive indirection/boilerplate/etc., consider a startup_tab_provider_win.cc that defines Windows-specific functionality in addition to/in place of things in this file, done in such a way that we don't end up using ifdefs in this file (there are a couple ways to do this). We discussed this quite a few patchsets ago (first_run.cc in #2) and I wasn't a fan of that option. The fact that the remainder of this function is still needed on Windows builds (for versions < 10) means that we'd be adding a lot of indirection to get "back" here from the Windows version. As an alternative, I'd be OK with doing all the work described in the TODO in a _win.cc and having that call be the only if-def'd portion. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab_provider.h (right): https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.h:28: virtual bool AddOnboardingTabs(StartupTabs* tabs) const = 0; On 2016/09/29 at 07:23:35, Peter Kasting (busy Sep 29) wrote: > 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()) ... 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? https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.h:32: // the value of first_run_urls_ in the provided BrowserCreator. Returns true On 2016/09/29 at 07:23:35, Peter Kasting (busy Sep 29) wrote: > 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. 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. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.h:41: virtual bool AddResetTriggerTabs(Profile* profile, On 2016/09/29 at 07:23:35, Peter Kasting (busy Sep 29) wrote: > Nit: Can |profile| be const? Nope. TriggeredProfileResetterFactory::GetForBrowserContext takes a non-const. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.h:48: // Reads tabs specified for new windows from the user's preferences, and On 2016/09/29 at 07:23:35, Peter Kasting (busy Sep 29) wrote: > Nit: What is a "tab specified for [a] new window"? As distinct, presumably, from tabs specified for other things? Done. In this case I've retained the "reads" phrasing because it helps to break this up into multiple clauses. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab_provider_unittest.cc (right): https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider_unittest.cc:45: EXPECT_EQ("www.google.com", output[1].url.host()); On 2016/09/29 at 07:23:36, Peter Kasting (busy Sep 29) wrote: > Nit: EXPECT_EQ(input[1], output[1].url)? (2 places) Done, but I don't see another place this could apply. (Note that the other two values being checked here are NOT the same as their corresponding values in input--we are explicitly checking that they were transformed as desired.)
Thanks for the very thoughtful and detailed replies. Here's some responses. I have yet to re-review. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:615: return StartupTabs({StartupTab(GURL(chrome::kChromeUINewTabURL), false)}); On 2016/09/30 20:55:01, tmartino wrote: > On 2016/09/29 at 07:23:33, Peter Kasting (busy Sep 29) wrote: > > Nit: Is there any way to avoid having both ( and { here? I'm not opposed to > constructing things this way, I just don't usually use this syntax, so I don't > know if either one of them could be omitted. > > > > Another way involves moving the |startup_tabs| declaration up: > > > > StartupTabs startup_tabs; > > if (is_incognito || is_post_crash_launch) { > > if (cmd_line_tabs.empty()) > > startup_tabs.emplace_back(GURL(chrome::kChromeUINewTabURL), false); > > return cmd_line_tabs; > > } > > > > I kinda prefer this honestly but up to you. > > I've tried this a few ways (this way was actually Greg's suggestion) and at the > end of the day I found this to most clearly/directly express what's going on > here. Every other method (e.g., vector<Foo> foolist = {foo1};) involves doing > some local storage and manipulation, when what we're really trying to do is say > "just bomb out and return this thing that's basically a constant". It's a little > unaesthetic, but I think it makes the most sense to do it inline. Right, I'm mostly looking for different syntax for this. The reason I liked the "call emplace_back and then return" route, in addition to paralleling things below, was simply that this line is very noisy to read; it has the phrase StartupTab twice, there are tons of ({(())}). I figured breaking into two lines, especially when you had to return cmd_line_tabs anyway and thus you wouldn't cost any net LOC, was overall more readable. Semantically, I'm not sure "push this constant and return" is fundamentally different than "return this constant"; they read the same to me. You can override me on this. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc (right): https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:28: if (options_ & kOnboardingTabs) { On 2016/09/30 20:55:02, tmartino wrote: > On 2016/09/29 at 07:23:34, Peter Kasting (busy Sep 29) wrote: > > Nit: All these methods can drop the {} if you reverse the conditionals > (because the bodies will be one line). > > 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. 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 :) https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab_provider.cc (right): https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:24: constexpr char kWelcomePageUrlHost[] = "welcome_page"; On 2016/09/30 20:55:02, tmartino wrote: > On 2016/09/29 at 07:23:35, Peter Kasting (busy Sep 29) wrote: > > Oh dear... these are valid hostnames for intranets. We really should not use > this method of specifying magic constants unless these are only stored in a > place we know intranet URLs could never go, e.g. the URL is > "chrome://new_tab_page/" with the chrome:// scheme clearly indicating this is an > internal thing (but then why not just use chrome://newtab/ directly?). > > > > If fixing this involves some kind of migration, do it separately -- but if at > all possible please do fix this during this process. > > I hate, hate, hate this too. I was horrified when I came across this logic when > untangling the spaghetti. Unfortunately, I don't think there's anything that can > be done about it. These don't seem to be defined anywhere else--it's entirely by > convention--but they're used in Master Preferences files created by external > partners and administrators, and bundled with Chrome in various distributions. > > https://support.google.com/chrome/a/answer/187948?hl=en That's something we can change over time. It would require asking the master prefs folks how we would go about making a breaking change (e.g. ship support for both, announce deprecation somehow, set timelines, then remove the old thing). I think this is worth doing, but it will require some intentionality on your part if you want to make it happen. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:28: #if defined(OS_WIN) On 2016/09/30 20:55:02, tmartino wrote: > On 2016/09/29 at 07:23:35, Peter Kasting (busy Sep 29) wrote: > > Nit: If it wouldn't add excessive indirection/boilerplate/etc., consider a > startup_tab_provider_win.cc that defines Windows-specific functionality in > addition to/in place of things in this file, done in such a way that we don't > end up using ifdefs in this file (there are a couple ways to do this). > > We discussed this quite a few patchsets ago (first_run.cc in #2) and I wasn't a > fan of that option. The fact that the remainder of this function is still needed > on Windows builds (for versions < 10) means that we'd be adding a lot of > indirection to get "back" here from the Windows version. > > As an alternative, I'd be OK with doing all the work described in the TODO in a > _win.cc and having that call be the only if-def'd portion. I'm open to any working solution, and admittedly it's hard to design without the context of the full eventual implementation here. Possibilities: * Add a PlatformSpecificAddOnboardingTabs() function which Windows implements as non-empty and other platforms implement as empty; call that here. * Make people who want this impl class get it via a factory function; return a Windows-specific subclass from that factory function, implement that to do the Windowsy stuff and then call the base. These functions are already virtual, so this is less boilerplatey than it might sound. It's possible neither of these are wins. It's hard for me to tell. I probably would have been inclined to go with the second bullet. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab_provider.h (right): https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.h:28: virtual bool AddOnboardingTabs(StartupTabs* tabs) const = 0; On 2016/09/30 20:55:02, tmartino wrote: > On 2016/09/29 at 07:23:35, Peter Kasting (busy Sep 29) wrote: > > 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()) ... > > 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. 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. > 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); 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. > 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. 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. > 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. 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). https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.h:32: // the value of first_run_urls_ in the provided BrowserCreator. Returns true On 2016/09/30 20:55:02, tmartino wrote: > On 2016/09/29 at 07:23:35, Peter Kasting (busy Sep 29) wrote: > > 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. > > 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. 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.
Ok, I've addressed everything so PTAL. Also, I wanted to note that we're really hoping to land this before branch point on Thursday. In terms of the project plan, we're largely in good shape to ship in M55, and if this is landed I don't really see any of the followups as being particularly high-risk merges should it come to that. I don't say this to rush the review in any way--of course the most important thing is that we're all comfortable with the quality of the code, far more important than any branch dates. However, I wanted to suggest that if there are pieces that you're comfortable with me fixing in a followup, I am happy to do so (and will commit to actually doing them) to save some merge risk down the line. Thanks again for helping out with this! https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:615: return StartupTabs({StartupTab(GURL(chrome::kChromeUINewTabURL), false)}); On 2016/09/30 at 21:50:03, Peter Kasting wrote: > On 2016/09/30 20:55:01, tmartino wrote: > > On 2016/09/29 at 07:23:33, Peter Kasting (busy Sep 29) wrote: > > > Nit: Is there any way to avoid having both ( and { here? I'm not opposed to > > constructing things this way, I just don't usually use this syntax, so I don't > > know if either one of them could be omitted. > > > > > > Another way involves moving the |startup_tabs| declaration up: > > > > > > StartupTabs startup_tabs; > > > if (is_incognito || is_post_crash_launch) { > > > if (cmd_line_tabs.empty()) > > > startup_tabs.emplace_back(GURL(chrome::kChromeUINewTabURL), false); > > > return cmd_line_tabs; > > > } > > > > > > I kinda prefer this honestly but up to you. > > > > I've tried this a few ways (this way was actually Greg's suggestion) and at the > > end of the day I found this to most clearly/directly express what's going on > > here. Every other method (e.g., vector<Foo> foolist = {foo1};) involves doing > > some local storage and manipulation, when what we're really trying to do is say > > "just bomb out and return this thing that's basically a constant". It's a little > > unaesthetic, but I think it makes the most sense to do it inline. > > Right, I'm mostly looking for different syntax for this. The reason I liked the "call emplace_back and then return" route, in addition to paralleling things below, was simply that this line is very noisy to read; it has the phrase StartupTab twice, there are tons of ({(())}). I figured breaking into two lines, especially when you had to return cmd_line_tabs anyway and thus you wouldn't cost any net LOC, was overall more readable. Semantically, I'm not sure "push this constant and return" is fundamentally different than "return this constant"; they read the same to me. > > You can override me on this. OK, I went with moving the |tabs| declaration up. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc (right): https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:28: if (options_ & kOnboardingTabs) { On 2016/09/30 at 21:50:03, Peter Kasting wrote: > On 2016/09/30 20:55:02, tmartino wrote: > > On 2016/09/29 at 07:23:34, Peter Kasting (busy Sep 29) wrote: > > > Nit: All these methods can drop the {} if you reverse the conditionals > > (because the bodies will be one line). > > > > 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. > > 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 :) No longer applies anyway :) https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab_provider.cc (right): https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:24: constexpr char kWelcomePageUrlHost[] = "welcome_page"; On 2016/09/30 at 21:50:03, Peter Kasting wrote: > On 2016/09/30 20:55:02, tmartino wrote: > > On 2016/09/29 at 07:23:35, Peter Kasting (busy Sep 29) wrote: > > > Oh dear... these are valid hostnames for intranets. We really should not use > > this method of specifying magic constants unless these are only stored in a > > place we know intranet URLs could never go, e.g. the URL is > > "chrome://new_tab_page/" with the chrome:// scheme clearly indicating this is an > > internal thing (but then why not just use chrome://newtab/ directly?). > > > > > > If fixing this involves some kind of migration, do it separately -- but if at > > all possible please do fix this during this process. > > > > I hate, hate, hate this too. I was horrified when I came across this logic when > > untangling the spaghetti. Unfortunately, I don't think there's anything that can > > be done about it. These don't seem to be defined anywhere else--it's entirely by > > convention--but they're used in Master Preferences files created by external > > partners and administrators, and bundled with Chrome in various distributions. > > > > https://support.google.com/chrome/a/answer/187948?hl=en > > That's something we can change over time. It would require asking the master prefs folks how we would go about making a breaking change (e.g. ship support for both, announce deprecation somehow, set timelines, then remove the old thing). I think this is worth doing, but it will require some intentionality on your part if you want to make it happen. Added a line to the tech debt phase of my project plan. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:28: #if defined(OS_WIN) On 2016/09/30 at 21:50:03, Peter Kasting wrote: > On 2016/09/30 20:55:02, tmartino wrote: > > On 2016/09/29 at 07:23:35, Peter Kasting (busy Sep 29) wrote: > > > Nit: If it wouldn't add excessive indirection/boilerplate/etc., consider a > > startup_tab_provider_win.cc that defines Windows-specific functionality in > > addition to/in place of things in this file, done in such a way that we don't > > end up using ifdefs in this file (there are a couple ways to do this). > > > > We discussed this quite a few patchsets ago (first_run.cc in #2) and I wasn't a > > fan of that option. The fact that the remainder of this function is still needed > > on Windows builds (for versions < 10) means that we'd be adding a lot of > > indirection to get "back" here from the Windows version. > > > > As an alternative, I'd be OK with doing all the work described in the TODO in a > > _win.cc and having that call be the only if-def'd portion. > > I'm open to any working solution, and admittedly it's hard to design without the context of the full eventual implementation here. Possibilities: > > * Add a PlatformSpecificAddOnboardingTabs() function which Windows implements as non-empty and other platforms implement as empty; call that here. > > * Make people who want this impl class get it via a factory function; return a Windows-specific subclass from that factory function, implement that to do the Windowsy stuff and then call the base. These functions are already virtual, so this is less boilerplatey than it might sound. > > It's possible neither of these are wins. It's hard for me to tell. I probably would have been inclined to go with the second bullet. I think I'm OK with the second; for reasons of scope and sanity, I'll hold off and add that file in the followup CL that actually implements the relevant logic. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab_provider.h (right): https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.h:28: virtual bool AddOnboardingTabs(StartupTabs* tabs) const = 0; On 2016/09/30 at 21:50:03, Peter Kasting wrote: > On 2016/09/30 20:55:02, tmartino wrote: > > On 2016/09/29 at 07:23:35, Peter Kasting (busy Sep 29) wrote: > > > 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()) ... > > > > 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. > > 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. > > > 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); > > 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. > > > 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. > > 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. > > > 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. > > 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). OK, switched back to the Getter pattern. I think I'm happier with it now, even if it's slightly less concise. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.h:32: // the value of first_run_urls_ in the provided BrowserCreator. Returns true On 2016/09/30 at 21:50:03, Peter Kasting wrote: > On 2016/09/30 20:55:02, tmartino wrote: > > On 2016/09/29 at 07:23:35, Peter Kasting (busy Sep 29) wrote: > > > 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. > > > > 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. > > 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. 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.)
Small update: the new Welcome page has been landed for awhile now, so I knocked out an easy TODO and inserted it into GetWelcomePageUrl().
LGTM. Sorry for taking so long. I've been flooded. https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab_provider.h (right): https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.h:32: // the value of first_run_urls_ in the provided BrowserCreator. Returns true On 2016/10/03 23:26:06, tmartino wrote: > On 2016/09/30 at 21:50:03, Peter Kasting wrote: > > 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. > > 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. 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). > (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.) 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. https://codereview.chromium.org/2164033002/diff/380001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2164033002/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:357: if (base::FeatureList::IsEnabled(features::kUseConsolidatedStartupFlow)) { Nit: No {} https://codereview.chromium.org/2164033002/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:624: return cmd_line_tabs; Nit: Hmm, I made an error in the previous suggestion I made. The code doesn't actually work. Your version does, but at the cost of two return statements and some extra {} that end up losing the readability win over the old thing. One way to get back to very short code here would be to have DetermineStartupTabs() take |cmd_line_tabs| by value instead of const ref, then change this to: if (cmd_line_tabs.empty()) cmd_line_tabs.emplace_back(GURL(chrome::kChromeUINewTabURL), false); return cmd_line_tabs; This allows moving the |tabs| declaration back down. Otherwise, I would probably go back to the old route (except for eliminate the "else" after the first "return" as I previously suggested). This also allows moving |tabs| back down, so I guess I would move it back down regardless. :P See next comment also. Sorry about this. https://codereview.chromium.org/2164033002/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:626: Nit: Some reordering of these next few blocks produces equivalent behavior but avoids testing for cmd_line_tabs.empty() repeatedly, which feels a bit confusing. Overall, I find this logic clearer. (It is also slightly shorter.) I rewrote the comments a little to better clarify the new order. // A trigger on a profile may indicate that we should show a tab which // offers to reset the user's settings. When this appears, it is first, and // may be shown alongside command-line tabs. StartupTabs tabs = provider.GetResetTriggerTabs(profile_); // URLs passed on the command line supersede all others. AppendTabs(cmd_line_tabs, &tabs); if (!cmd_line_tabs.empty()) return tabs; // A Master Preferences file provided with this distribution may specify // tabs to be displayed on first run, overriding all non-command-line tabs, // including the profile reset tab. StartupTabs distribution_tabs = provider.GetDistributionFirstRunTabs(browser_creator_); if (!distribution_tabs.empty()) return distribution_tabs; ... continues with "Policies for onboarding" https://codereview.chromium.org/2164033002/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:631: StartupTabs distribution_tabs = Nit: Could just use |tabs| here (but not if you take my suggestion above) https://codereview.chromium.org/2164033002/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:633: if (!distribution_tabs.empty()) { Nit: No {} https://codereview.chromium.org/2164033002/diff/380001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.h (right): https://codereview.chromium.org/2164033002/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.h:113: void ProcessLaunchUrlsUsingConsolidatedFlow( Nit: Make sure before you land that .cc order matches .h order. The two are out of sync right now. https://codereview.chromium.org/2164033002/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.h:143: // Record Rappor metrics on startup URLs. Nit: Records https://codereview.chromium.org/2164033002/diff/380001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc (right): https://codereview.chromium.org/2164033002/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:31: return tabs; Nit: See comments in startup_tab_provider.cc for a possible way all of these methods could be simplified even further now that the signatures have changed. https://codereview.chromium.org/2164033002/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:137: StartupTabs({StartupTab(GURL("https://cmd-line"), false)}); Nit: "StartupTabs()" wrapper around initializer is unnecessary https://codereview.chromium.org/2164033002/diff/380001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab_provider.cc (right): https://codereview.chromium.org/2164033002/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:20: StartupTabs StartupTabProviderImpl::GetOnboardingTabs() const { Nit: Make sure the definition order of the functions in this .cc file matches the declaration order in your class header. https://codereview.chromium.org/2164033002/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:48: auto* triggered_profile_resetter = Nit: Still wondering if this can be const auto* https://codereview.chromium.org/2164033002/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:73: return tabs; Nit: Now that these functions return StartupTabs directly, you could make this and CheckResetTriggerTabPolicy even shorter with something like: const StartupTabs first_run_tabs = {StartupTab(GetWelcomePageUrl(), false)}; return is_first_run ? first_run_tabs : StartupTabs(); Up to you. https://codereview.chromium.org/2164033002/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:89: if (url.host() == kNewTabUrlHost) { Nit: {} not necessary (all arms are one line) https://codereview.chromium.org/2164033002/diff/380001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab_provider.h (right): https://codereview.chromium.org/2164033002/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.h:43: // new window. Nit: Line wrapping
Thanks again Roger, Greg, and Peter for all the attention you've given this. The difference in quality between patchset 1 and 21 is pretty humongous. Will followup with patches addressing the various bugs and TODOs in the very near future. :) https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab_provider.h (right): https://codereview.chromium.org/2164033002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.h:32: // the value of first_run_urls_ in the provided BrowserCreator. Returns true On 2016/10/06 at 05:49:08, Peter Kasting (OOO Oct. 6-9) wrote: > On 2016/10/03 23:26:06, tmartino wrote: > > On 2016/09/30 at 21:50:03, Peter Kasting wrote: > > > 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. > > > > 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. > > 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). > > > (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.) > > 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. crbug.com/653590 https://codereview.chromium.org/2164033002/diff/380001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2164033002/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:624: return cmd_line_tabs; On 2016/10/06 at 05:49:08, Peter Kasting (OOO Oct. 6-9) wrote: > Nit: Hmm, I made an error in the previous suggestion I made. The code doesn't actually work. Your version does, but at the cost of two return statements and some extra {} that end up losing the readability win over the old thing. > > One way to get back to very short code here would be to have DetermineStartupTabs() take |cmd_line_tabs| by value instead of const ref, then change this to: > > if (cmd_line_tabs.empty()) > cmd_line_tabs.emplace_back(GURL(chrome::kChromeUINewTabURL), false); > return cmd_line_tabs; > > This allows moving the |tabs| declaration back down. > > Otherwise, I would probably go back to the old route (except for eliminate the "else" after the first "return" as I previously suggested). This also allows moving |tabs| back down, so I guess I would move it back down regardless. :P See next comment also. > > Sorry about this. Went back to the old way, mostly out of preference for const ref. https://codereview.chromium.org/2164033002/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:626: On 2016/10/06 at 05:49:08, Peter Kasting (OOO Oct. 6-9) wrote: > Nit: Some reordering of these next few blocks produces equivalent behavior but avoids testing for cmd_line_tabs.empty() repeatedly, which feels a bit confusing. Overall, I find this logic clearer. (It is also slightly shorter.) I rewrote the comments a little to better clarify the new order. > > // A trigger on a profile may indicate that we should show a tab which > // offers to reset the user's settings. When this appears, it is first, and > // may be shown alongside command-line tabs. > StartupTabs tabs = provider.GetResetTriggerTabs(profile_); > > // URLs passed on the command line supersede all others. > AppendTabs(cmd_line_tabs, &tabs); > if (!cmd_line_tabs.empty()) > return tabs; > > // A Master Preferences file provided with this distribution may specify > // tabs to be displayed on first run, overriding all non-command-line tabs, > // including the profile reset tab. > StartupTabs distribution_tabs = > provider.GetDistributionFirstRunTabs(browser_creator_); > if (!distribution_tabs.empty()) > return distribution_tabs; > > ... continues with "Policies for onboarding" This is great! Thanks. https://codereview.chromium.org/2164033002/diff/380001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab_provider.cc (right): https://codereview.chromium.org/2164033002/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:48: auto* triggered_profile_resetter = On 2016/10/06 at 05:49:09, Peter Kasting (OOO Oct. 6-9) wrote: > Nit: Still wondering if this can be const auto* Weirdly, HasResetTrigger is non-const. https://codereview.chromium.org/2164033002/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:73: return tabs; On 2016/10/06 at 05:49:09, Peter Kasting (OOO Oct. 6-9) wrote: > Nit: Now that these functions return StartupTabs directly, you could make this and CheckResetTriggerTabPolicy even shorter with something like: > > const StartupTabs first_run_tabs = {StartupTab(GetWelcomePageUrl(), false)}; > return is_first_run ? first_run_tabs : StartupTabs(); > > Up to you. Ack. IMO the current way is a bit more expressive, and it saves a few calls/allocations.
The CQ bit was checked by tmartino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tmartino@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2164033002/#ps400001 (title: "Attempting submission")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #21 (id:400001)
Message was sent while issue was closed.
Description was changed from ========== Refactoring startup logic for upcoming FRE changes (non-Win 10). Refactor goals (in descending order): 1. Consolidate the various places throughout startup where onboarding and other special-case tabs are added. 2. Reduce the massive number of branches, spaghetti calls, and seemingly-redundant checks into a manageable, linear flow. 3. Offload all policy logic (e.g., "Should we show the Welcome page?") for FRE to the more-appropriate first_run directory. 4. Where possible, consolidate code into testable units. BUG=618454, 248883, 517248 ========== to ========== Refactoring startup logic for upcoming FRE changes (non-Win 10). Refactor goals (in descending order): 1. Consolidate the various places throughout startup where onboarding and other special-case tabs are added. 2. Reduce the massive number of branches, spaghetti calls, and seemingly-redundant checks into a manageable, linear flow. 3. Offload all policy logic (e.g., "Should we show the Welcome page?") for FRE to the more-appropriate first_run directory. 4. Where possible, consolidate code into testable units. BUG=618454, 248883, 517248 Committed: https://crrev.com/b930f6e90137299cc399cd35bcf7e7f89c19587e Cr-Commit-Position: refs/heads/master@{#423636} ==========
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/b930f6e90137299cc399cd35bcf7e7f89c19587e Cr-Commit-Position: refs/heads/master@{#423636}
Message was sent while issue was closed.
i like how this came together. i just made a quick pass and have two suggestions for the next round. cheers. https://codereview.chromium.org/2164033002/diff/400001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2164033002/diff/400001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:682: for (size_t i = 0; i < urls.size(); ++i) { nit: this can be condensed a bit: foreach (const GURL& url : urls) { size_t j = 0; while (j < num_existing_tabs && url != (*tabs)[j].url) ++j; if (j == num_existing_tabs) tabs->emplace_back(url, false); } https://codereview.chromium.org/2164033002/diff/400001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_tab_provider.cc (right): https://codereview.chromium.org/2164033002/diff/400001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_tab_provider.cc:82: constexpr char kNewTabUrlHost[] = "new_tab_page"; nit: static constexpr for these
Message was sent while issue was closed.
https://codereview.chromium.org/2164033002/diff/400001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2164033002/diff/400001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:682: for (size_t i = 0; i < urls.size(); ++i) { On 2016/10/07 07:21:45, grt (UTC plus 2) wrote: > nit: this can be condensed a bit: > foreach (const GURL& url : urls) { > size_t j = 0; > while (j < num_existing_tabs && url != (*tabs)[j].url) > ++j; > if (j == num_existing_tabs) > tabs->emplace_back(url, false); > } Yep. And without spending much time thinking about the details, I'd say: The first lines of that are trying to find if a matching element occurs in a container or not, so look for a way to rewrite them with find_if() and a lambda. Even if this does not save much code, it more clearly expresses the underlying intent. |
