Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1226643002:
Welcome page changes for Windows 10 and over. (Closed)
|
Created:
5 years, 5 months ago by grt (UTC plus 2) Modified:
5 years, 5 months ago Reviewers:
hodie, jochen (gone - plz use gerrit), Mattias Nissler (ping if slow), gab, Andrew T Wilson (Slow), msw, Alexei Svitkine (slow) CC:
asvitkine+watch_chromium.org, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWelcome page changes for Windows 10.
* Show the welcome page on the first launch following OS upgrades to or
past Windows 10 when Chrome is not the default browser unless the
WelcomePageOnOSUpgradeEnabled policy setting is set to false or the
welcome_page_on_os_upgrade_enabled distribution master_preferences
value is set to false.
* Suppress showing the signin promo during first-run on Windows 10 and
instead show the welcome page before the NTP.
BUG=505029
TEST=On Windows 8.1 and older, nothing changes. On Windows 10, the
signin promo is no longer shown on first run; rather, the welcome page
and the NTP are shown. On Windows 10, the welcome page is shown on the
first ordinary launch (for all "On startup" settings except when an URL
is provided on the command line) following an upgrade from an earlier
version of Windows when Chrome is not the default browser.
Committed: https://crrev.com/f14398c95a77206066cbab24b45070a85aba469a
Cr-Commit-Position: refs/heads/master@{#338688}
Patch Set 1 : #Patch Set 2 : support master_preferences and better policy tweaking #Patch Set 3 : support launching from bg mode and passing browser_tests #Patch Set 4 : allow ScopedAllowIO in startup_browser_creator_impl.cc #Patch Set 5 : fix RestoreOnStartupURLsPolicySpecified #
Total comments: 88
Patch Set 6 : sync to position 337983 #Patch Set 7 : partial comments #
Total comments: 4
Patch Set 8 : more comments #Patch Set 9 : removed need for ScopedAllowIO #Patch Set 10 : there is no StartupBrowserCreator on Android #Patch Set 11 : simplified recording of welcome run complete #
Total comments: 33
Patch Set 12 : more msw comments #Patch Set 13 : simplified WelcomeRunType #Patch Set 14 : fix chromeos build #
Total comments: 2
Patch Set 15 : comment fix #
Total comments: 2
Patch Set 16 : rewording in policy_templates.json and sync to position 338549 #Messages
Total messages: 83 (35 generated)
Patchset #1 (id:1) has been deleted
grt@chromium.org changed reviewers: + asvitkine@chromium.org
Patchset #1 (id:20001) has been deleted
grt@chromium.org changed reviewers: + atwilson@chromium.org
grt@chromium.org changed reviewers: + mnissler@chromium.org
grt@chromium.org changed reviewers: + msw@chromium.org
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1226643002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) (exceeded global retry quota)
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
grt@chromium.org changed reviewers: + gab@chromium.org
msw: please review chrome/browser/ui/startup gab: please review the first_run and installer/ stuff atwilson: please review chrome/browser/signin, components/policy/resources, and the other policy related bits asvitkine: please review histograms.xml Thanks, all.
grt@chromium.org changed reviewers: + jochen@chromium.org
+jochen: please review chrome/browser/chrome_browser_main.cc and PRESUBMIT.py. As noted in startup_browser_creator_impl.cc, StartupBrowserCreatorImpl::InitializeWelcomeRunType needs to check whether or not Chrome is the user's chosen default browser. This will touch the filesystem for one and only launch on Win10+ for per-user installs, which are now the minority. For all other platforms, for per-machine installs, and for per-user installs after the first post-Win10 launch, it will not touch the FS. Changing the code to make StartupBrowserCreator::LaunchBrowser asynchronous would be a massive undertaking. Thanks for taking a look.
The CQ bit was checked by grt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1226643002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) (exceeded global retry quota)
FYI to reviewers: the win trybot failure is http://crbug.com/508531 and is unrelated to this change.
histograms.xml lgtm, did not look at anything else
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:104: #if defined(OS_WIN) Why not define this on all platforms and have an os-specific impl? https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:105: static void RegisterLocalStatePrefs(PrefRegistrySimple* registry); Why not add this to the impl class, where the prefs are used? https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right): https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:97: // Presently, this is exactly the same set of platforms that support But the condition is simple, and changes to ShowWelcomePageAfterFirstRun might unwittingly alter this behavior too... Why not just keep the logic separate, or nix both for something like IsWindowsVersion10OrGreater() instead (not that I think such a helper is needed)? https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:249: g_browser_process->AddRefModule(); Why isn't this needed for StartupURLsOnNewWindow which also closes the window? https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:772: ASSERT_LT(0, tab_strip->count()); nit: not necessary with the checks in each case below. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:562: InitializeWelcomeRunType(urls_to_open); I feel like there's a lot of unnecessary complexity here... Why doesn't this member function simply add the welcome page to |urls_to_open| and record it as shown, as needed? I don't see the value in having a separate state to initialize, having the codepaths below actually add the page, and waiting until the page is actually shown to record the prefs. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:933: // Put the welcome page before the new tab page on Windows 10. nit: remove the comment about win10. This code here isn't win10 specific, even if the code setting |welcome_run_type_| is os/version specific. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:1047: // Remember that the welcome page was added for this OS version. nit: s/added/shown/ https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.h (right): https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.h:38: bool ShowWelcomePageAfterFirstRun(); Inline this in StartupBrowserCreatorImpl::InitializeWelcomeRunType, and make the tests similarly check the os/version inline. I don't see a good reason to abstract this decision in this manner, nor explain this logic in the header. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.h:94: FIRST_RUN_FIRST, // Inject the welcome page as the first first-run tab. It doesn't seem like the enum needs to convey the reason for showing the welcome page as the first tab... Even though the *FIRST value might be set for different reasons, nothing actually triggers distinct behavior for FIRST_RUN_FIRST vs. ANY_RUN_FIRST, afaict... So why not just have 3 enum values: WELCOME_TAB_[NONE|FIRST|LAST]. Better yet, why don't we just always show the welcome tab first? Is there any reason to have distinct welcome tab ordering for different cases? https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.h:161: // Initializes |welcome_run_type_| for this launch. For first-run, the type Don't explain the nuanced impl details here, move it to the code file.
As owner:
c/b/first_run lgtm
c/b/prefs lgtm w/ comment nit
c/installer lgtm w/ comment nit
In close relation with prefs ownership:
c/b/chrome_browser_main.cc (master_prefs handling) lgtm w/ comment
c/b/ui/startup_browser_creator.(h|cc) (RegisterPrefs() addition) lgtm
c/common/pref_names(h|cc) (new constants) lgtm
Drive-by:
c/b/ui/startup_browser_creator_impl.(h|cc)
=> some comments, but I overall can't make any sense of the control flow in
this StartupBrowserCreator, hopefully msw can..
c/b/ui/startup/startup_browser_creator_browsertest.cc
=> some comments, at least these are giving me some confidence in the above
logic being correct..
c/b/signin => lgtm w/ suggestion
policy stuff => looked but no idea how this code works
Cheers!
Gab
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:994:
local_state_->SetBoolean(prefs::kWelcomePageOnOSUpgradeEnabled, false);
Should be outside the |variations_seed| if (i.e. up one scope level from here).
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/first_r...
File chrome/browser/first_run/first_run.cc (right):
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/first_r...
chrome/browser/first_run/first_run.cc:557: &value) &&
|value| is unchanged if this returns false (i.e. the pref is not there), so
having it default to true on 596 and calling the getter without interpreting the
return value is sufficient -- though I realize now that the way you wrote it is
also the way it's written for other bool prefs above so I guess this is fine..
tl;dr; I'm okay with this, but there's room for clean up..
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/signin/...
File chrome/browser/signin/signin_promo.cc (right):
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/signin/...
chrome/browser/signin/signin_promo.cc:124: if (is_new_profile &&
base::win::GetVersion() >= base::win::VERSION_WIN10)
Should we add a
DCHECK_EQ(base::win::VERSION_WIN10, base::win::GetVersion())
<< "Need to revisit first run sequence on new Windows version".
here?
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/signin/...
chrome/browser/signin/signin_promo.cc:124: if (is_new_profile &&
base::win::GetVersion() >= base::win::VERSION_WIN10)
optional: Feels this check should go first (before checking any of the prefs,
etc.).
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star...
File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right):
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:97: //
Presently, this is exactly the same set of platforms that support
s/that support/that don't support/ ?
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:294:
g_browser_process->ReleaseModule();
(orthogonal: Wouldn't it be nice to have scoped objects for these refcounts,
eh...)
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:316:
new_browser = FindTabbedBrowser(profile, true, host_desktop_type);
The initial assignment above uses FindOneOtherBrowser(), I don't care which is
used, but we should be consistent (or comment on why it differs if it truly
does).
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:772:
ASSERT_LT(0, tab_strip->count());
Constant on RHS for inequalities (also >= 1 makes more sense here IMO).
But actually I think you should get rid of this line as it's already covered
more strictly be the ASSERT_EQ's below.
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:774: if
(internals::ShowWelcomePageAfterFirstRun()) {
Add a comment here like:
// The first profile should get the welcome page on supported platforms, but the
second profile should not get a second instance of it.
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:777:
web_contents = tab_strip->GetWebContentsAt(1);
When reading this I first got confused thinking this was the Welcome Page's
index and wondering why it wasn't first...
I think this would be more readable if each WebContents* had its own explicitly
named variable then re-using a single one declared outside this scope
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:1001: //
launched, even if it will open just the home page (and the welcome page on
s/home page/new tab page/
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:1007:
ASSERT_LT(0, tab_strip->count());
Redundant.
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:1012:
web_contents = tab_strip->GetWebContentsAt(1);
Same comment as above, suggest using explicit variable names.
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:1130:
ASSERT_LT(0, tab_strip->count());
Redundant.
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:1135:
web_contents = tab_strip->GetWebContentsAt(1);
Explicit names.
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:304:
OSInfo::VersionNumber v(OSInfo::GetInstance()->version_number());
const
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:305: std::string
os_version(base::StringPrintf("%d.%d", v.major, v.minor));
const
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:593: std::vector<GURL>
adjust_urls(urls_to_open);
s/adjust_urls/adjusted_urls/
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:595:
adjust_urls.push_back(internals::GetWelcomePageURL());
Doesn't this unconditionally put it last?
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:679: std::vector<GURL>
adjust_urls(urls_to_open);
s/adjust_urls/adjusted_urls/
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:681:
adjust_urls.push_back(internals::GetWelcomePageURL());
Again, why is unconditionally pushing to the back okay here?
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:742: if
(welcome_run_type_ == WelcomeRunType::ANY_RUN_FIRST)
Why not also FIRST_RUN_FIRST? I guess I don't understand why you need the 4th
Type (why is NONE, FIRST, LAST not sufficient?)
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:743:
UrlsToTabs(std::vector<GURL>(1, internals::GetWelcomePageURL()), &tabs);
UrlsToTabs()'s name is misleading :-( -- it's really AppendTabsFromUrls()
(I'm not suggesting you change it here, just stating my confusion)
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:982:
base::win::OSInfo* os_info = base::win::OSInfo::GetInstance();
const
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:1004: PrefService*
local_state = g_browser_process->local_state();
const
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:1012:
base::win::OSInfo::VersionNumber v(os_info->version_number());
const
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:1013: std::string
this_os_version(base::StringPrintf("%d.%d", v.major, v.minor));
const
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:1048:
internals::RecordWelcomeRunComplete();
Should we also reset |welcome_run_type_ = WelcomeRunType::NONE| here? i.e. I'm
not 100% convinced by reading the above code that some other piece of code won't
potentially end up handling something a second time based on this stale value
(SO MUCH code potentially having access to this member..
startup/rendez-vous/other types of restores?/remote sync session
restores?/bookmark folder restore?).
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star...
File chrome/browser/ui/startup/startup_browser_creator_impl.h (right):
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_impl.h:36: // upgrade, even
when not in first-run. When this is true, the welcome page
"even when not" suggests it *may also* happen during First Run; while the method
name "AfterFirstRun" suggests it's *only after* First Run (I still haven't read
further to figure out which is the truth, but this comment/method name could be
clearer).
(addition) Ah, now that I'm reading the impl I see what this is checking, how
about "ShowWelcomePageAfterFirstRunAllowed()"? Also I think this is perhaps not
the best place to describe the behavior (i.e. this method only checks whether
it's allowed but doesn't enforce the behavior it documents)
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_impl.h:163: // FIRST_TAB. For
non-first run, the type will be NONE for all systems
FIRST_TAB and LAST_TAB don't match the values of the WelcomeRunType enum
(assuming you changed them after writing the comment and didn't update here?)
https://codereview.chromium.org/1226643002/diff/200001/chrome/common/pref_nam...
File chrome/common/pref_names.cc (right):
https://codereview.chromium.org/1226643002/diff/200001/chrome/common/pref_nam...
chrome/common/pref_names.cc:1674: // WelcomePageOnOSUpgradeEnabled policy
setting or by master_preferences.
Technically it's not "overridden" by master_preferences, it's "set from"
master_preferences, WDYT of:
// May be set from master_preferences or overridden by the
WelcomePageOnOSUpgradeEnabled policy setting.
?
https://codereview.chromium.org/1226643002/diff/200001/chrome/installer/util/...
File chrome/installer/util/master_preferences.h (right):
https://codereview.chromium.org/1226643002/diff/200001/chrome/installer/util/...
chrome/installer/util/master_preferences.h:56: //
"welcome_page_on_os_upgrade_enabled": false
If anyone re-uses this "prototype" without giving it much thought, we should
suggest "true" as a default value here I think.
https://codereview.chromium.org/1226643002/diff/200001/components/policy/reso...
File components/policy/resources/policy_templates.json (right):
https://codereview.chromium.org/1226643002/diff/200001/components/policy/reso...
components/policy/resources/policy_templates.json:7411: If this policy is
enalbed and set to false, the browser will not re-show the welcome page on the
first launch following an OS upgrade.''',
s/enalbed/enabled
Thanks for the reviews. I've addressed some of the comments. Please take a look at my questions below. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator.h (right): https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:104: #if defined(OS_WIN) On 2015/07/09 17:51:30, msw wrote: > Why not define this on all platforms and have an os-specific impl? No compelling reason. Done. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator.h:105: static void RegisterLocalStatePrefs(PrefRegistrySimple* registry); On 2015/07/09 17:51:30, msw wrote: > Why not add this to the impl class, where the prefs are used? Because I consider this .h to be the public facing portion of browser/ui/startup. It seemed icky to me for code outside of here to care about an implementation detail such as the impl class. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right): https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:249: g_browser_process->AddRefModule(); On 2015/07/09 17:51:31, msw wrote: > Why isn't this needed for StartupURLsOnNewWindow which also closes the window? StartupURLsOnNewWindow closes the second browser (new_browser), but not the initial one created by the test fixture (browser()), so that initial window is open throughout the test and keeping the browser process alive. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:772: ASSERT_LT(0, tab_strip->count()); On 2015/07/09 18:05:11, gab wrote: > Constant on RHS for inequalities (also >= 1 makes more sense here IMO). Done. > But actually I think you should get rid of this line as it's already covered > more strictly be the ASSERT_EQ's below. Done. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:772: ASSERT_LT(0, tab_strip->count()); On 2015/07/09 17:51:30, msw wrote: > nit: not necessary with the checks in each case below. tab_strip->GetWebContentsAt(0) on the next line will cause the test to crash if the tab strip is empty. I thought it would be better for the test to fail with the ASSERT here rather than crash outright. I've rejiggered things now so that it's no longer necessary. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:774: if (internals::ShowWelcomePageAfterFirstRun()) { On 2015/07/09 18:05:12, gab wrote: > Add a comment here like: > > // The first profile should get the welcome page on supported platforms, but the > second profile should not get a second instance of it. Done. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:777: web_contents = tab_strip->GetWebContentsAt(1); On 2015/07/09 18:05:11, gab wrote: > When reading this I first got confused thinking this was the Welcome Page's > index and wondering why it wasn't first... > > I think this would be more readable if each WebContents* had its own explicitly > named variable then re-using a single one declared outside this scope Done. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:982: base::win::OSInfo* os_info = base::win::OSInfo::GetInstance(); On 2015/07/09 18:05:12, gab wrote: > const which thing, the pointer or the thing being pointed to? consting the thing being pointed to would work, although it would mean this code couldn't use one of the getters if it wanted to. consting the pointer would look odd. i find both displeasing for different reasons. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:1012: base::win::OSInfo::VersionNumber v(os_info->version_number()); On 2015/07/09 18:05:12, gab wrote: > const i find that consting all variables that don't happen to be modified after initialization clutters up the function. is there a compelling reason to do this? https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:1047: // Remember that the welcome page was added for this OS version. On 2015/07/09 17:51:31, msw wrote: > nit: s/added/shown/ nice catch. done. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:1048: internals::RecordWelcomeRunComplete(); On 2015/07/09 18:05:12, gab wrote: > Should we also reset |welcome_run_type_ = WelcomeRunType::NONE| here? i.e. I'm > not 100% convinced by reading the above code that some other piece of code won't > potentially end up handling something a second time based on this stale value > (SO MUCH code potentially having access to this member.. > startup/rendez-vous/other types of restores?/remote sync session > restores?/bookmark folder restore?). Good point. Done. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.h (right): https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.h:94: FIRST_RUN_FIRST, // Inject the welcome page as the first first-run tab. On 2015/07/09 17:51:31, msw wrote: > It doesn't seem like the enum needs to convey the reason for showing the welcome > page as the first tab... Even though the *FIRST value might be set for different > reasons, nothing actually triggers distinct behavior for FIRST_RUN_FIRST vs. > ANY_RUN_FIRST, afaict... So why not just have 3 enum values: > WELCOME_TAB_[NONE|FIRST|LAST]. Better yet, why don't we just always show the > welcome tab first? Is there any reason to have distinct welcome tab ordering for > different cases? I didn't differentiate these initially, but it turned out to be significant. The handling of SessionStartupPref::URLS in StartupBrowserCreatorImpl::ProcessSpecifiedURLs shouldn't add the welcome page during first run since this would add it when an enterprise uses the kRestoreOnStartup policy to specify the tabs to be used for startup. An alternative is for InitializeWelcomeRunType to set the type to NONE if the startup URLs are specified by policy. I think that would get rid of the need for the extra value here and would potentially be safer for enterprises. What do you think? https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.h:161: // Initializes |welcome_run_type_| for this launch. For first-run, the type On 2015/07/09 17:51:31, msw wrote: > Don't explain the nuanced impl details here, move it to the code file. Done. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.h:163: // FIRST_TAB. For non-first run, the type will be NONE for all systems On 2015/07/09 18:05:13, gab wrote: > FIRST_TAB and LAST_TAB don't match the values of the WelcomeRunType enum > (assuming you changed them after writing the comment and didn't update here?) Done.
policy and prefs bits LGTM
https://codereview.chromium.org/1226643002/diff/240001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/1226643002/diff/240001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:1024: base::ThreadRestrictions::ScopedAllowIO allow_io; is it possible to move this check to before the io restrictions are turned on?
Thanks for the comment. See response below. https://codereview.chromium.org/1226643002/diff/240001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/1226643002/diff/240001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:1024: base::ThreadRestrictions::ScopedAllowIO allow_io; On 2015/07/10 09:03:31, jochen wrote: > is it possible to move this check to before the io restrictions are turned on? For an ordinary Chrome launch, this does indeed take place before restrictions are turned on. When the browser is already running (either in background mode or foreground), a new chrome.exe will rendezvous with the running process, hand it the command line, and that running process will hit this. In this case, IO restrictions are already on.
https://codereview.chromium.org/1226643002/diff/240001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/1226643002/diff/240001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:1024: base::ThreadRestrictions::ScopedAllowIO allow_io; On 2015/07/10 at 12:05:31, grt wrote: > On 2015/07/10 09:03:31, jochen wrote: > > is it possible to move this check to before the io restrictions are turned on? > > For an ordinary Chrome launch, this does indeed take place before restrictions are turned on. When the browser is already running (either in background mode or foreground), a new chrome.exe will rendezvous with the running process, hand it the command line, and that running process will hit this. In this case, IO restrictions are already on. that means that we potentially show the page twice, right? what about setting some flag after this check was done on the actual first run, and just bail out on subsequent calls in scenarios like the one you described?
https://codereview.chromium.org/1226643002/diff/240001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/1226643002/diff/240001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:1024: base::ThreadRestrictions::ScopedAllowIO allow_io; On 2015/07/10 12:12:51, jochen wrote: > On 2015/07/10 at 12:05:31, grt wrote: > > On 2015/07/10 09:03:31, jochen wrote: > > > is it possible to move this check to before the io restrictions are turned > on? > > > > For an ordinary Chrome launch, this does indeed take place before restrictions > are turned on. When the browser is already running (either in background mode or > foreground), a new chrome.exe will rendezvous with the running process, hand it > the command line, and that running process will hit this. In this case, IO > restrictions are already on. > > that means that we potentially show the page twice, right? No. In fact, we'll never his this line if the page has already been shown due to the check on line 1011. The most likely case for this being hit will be if Chrome was launched in bg mode after an OS upgrade and then the user clicked the Chrome icon to launch. > what about setting some flag after this check was done on the actual first run, > and just bail out on subsequent calls in scenarios like the one you described?
On 2015/07/10 at 12:17:31, grt wrote: > https://codereview.chromium.org/1226643002/diff/240001/chrome/browser/ui/star... > File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): > > https://codereview.chromium.org/1226643002/diff/240001/chrome/browser/ui/star... > chrome/browser/ui/startup/startup_browser_creator_impl.cc:1024: base::ThreadRestrictions::ScopedAllowIO allow_io; > On 2015/07/10 12:12:51, jochen wrote: > > On 2015/07/10 at 12:05:31, grt wrote: > > > On 2015/07/10 09:03:31, jochen wrote: > > > > is it possible to move this check to before the io restrictions are turned > > on? > > > > > > For an ordinary Chrome launch, this does indeed take place before restrictions > > are turned on. When the browser is already running (either in background mode or > > foreground), a new chrome.exe will rendezvous with the running process, hand it > > the command line, and that running process will hit this. In this case, IO > > restrictions are already on. > > > > that means that we potentially show the page twice, right? > > No. In fact, we'll never his this line if the page has already been shown due to the check on line 1011. The most likely case for this being hit will be if Chrome was launched in bg mode after an OS upgrade and then the user clicked the Chrome icon to launch. > in that case, however, we could post a task to another thread, right? > > what about setting some flag after this check was done on the actual first run, > > and just bail out on subsequent calls in scenarios like the one you described?
On 2015/07/10 12:19:39, jochen wrote: > On 2015/07/10 at 12:17:31, grt wrote: > > > https://codereview.chromium.org/1226643002/diff/240001/chrome/browser/ui/star... > > File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): > > > > > https://codereview.chromium.org/1226643002/diff/240001/chrome/browser/ui/star... > > chrome/browser/ui/startup/startup_browser_creator_impl.cc:1024: > base::ThreadRestrictions::ScopedAllowIO allow_io; > > On 2015/07/10 12:12:51, jochen wrote: > > > On 2015/07/10 at 12:05:31, grt wrote: > > > > On 2015/07/10 09:03:31, jochen wrote: > > > > > is it possible to move this check to before the io restrictions are > turned > > > on? > > > > > > > > For an ordinary Chrome launch, this does indeed take place before > restrictions > > > are turned on. When the browser is already running (either in background > mode or > > > foreground), a new chrome.exe will rendezvous with the running process, hand > it > > > the command line, and that running process will hit this. In this case, IO > > > restrictions are already on. > > > > > > that means that we potentially show the page twice, right? > > > > No. In fact, we'll never his this line if the page has already been shown due > to the check on line 1011. The most likely case for this being hit will be if > Chrome was launched in bg mode after an OS upgrade and then the user clicked the > Chrome icon to launch. > > > > > in that case, however, we could post a task to another thread, right? Ideally, yes, but that would require a refactor of the entire StartupBrowserCreator (that's what I meant by "Changing the code to make StartupBrowserCreator::LaunchBrowser asynchronous would be a massive undertaking.") I think that the risk of introducing visible jank with this change is pretty much zero, since there will likely be no visible UI when the code is hit.
On 2015/07/10 at 12:34:14, grt wrote: > On 2015/07/10 12:19:39, jochen wrote: > > On 2015/07/10 at 12:17:31, grt wrote: > > > > > https://codereview.chromium.org/1226643002/diff/240001/chrome/browser/ui/star... > > > File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): > > > > > > > > https://codereview.chromium.org/1226643002/diff/240001/chrome/browser/ui/star... > > > chrome/browser/ui/startup/startup_browser_creator_impl.cc:1024: > > base::ThreadRestrictions::ScopedAllowIO allow_io; > > > On 2015/07/10 12:12:51, jochen wrote: > > > > On 2015/07/10 at 12:05:31, grt wrote: > > > > > On 2015/07/10 09:03:31, jochen wrote: > > > > > > is it possible to move this check to before the io restrictions are > > turned > > > > on? > > > > > > > > > > For an ordinary Chrome launch, this does indeed take place before > > restrictions > > > > are turned on. When the browser is already running (either in background > > mode or > > > > foreground), a new chrome.exe will rendezvous with the running process, hand > > it > > > > the command line, and that running process will hit this. In this case, IO > > > > restrictions are already on. > > > > > > > > that means that we potentially show the page twice, right? > > > > > > No. In fact, we'll never his this line if the page has already been shown due > > to the check on line 1011. The most likely case for this being hit will be if > > Chrome was launched in bg mode after an OS upgrade and then the user clicked the > > Chrome icon to launch. > > > > > > > > > in that case, however, we could post a task to another thread, right? > > Ideally, yes, but that would require a refactor of the entire StartupBrowserCreator (that's what I meant by "Changing > the code to make StartupBrowserCreator::LaunchBrowser asynchronous would be a massive undertaking.") I think that the risk of introducing visible jank with this change is pretty much zero, since there will likely be no visible UI when the code is hit. hum, let me add jam@ to get a second opinion
jochen@chromium.org changed reviewers: + jam@chromium.org
John, wdyt about the ScopedAllowIO here?
Thinking about it a bit more -- it will be hit more than once for per-user installs where Chrome is the default browser. I think it would suffice to cache Chrome's defaultness at startup in the BrowserProcess (when IO is allowed) and access it here. It won't be 100% accurate (Chrome could become default after startup), but I think that's good enough for this feature to do the right thing the vast majority of the time. What do you think about adding BrowserProcess::CachedIsDefaultBrowser() or somesuch that holds the last default state?
sgtm
More comments addressed. msw and gab: Please see my question regarding SessionStartupPref::URLS and the FIRST_RUN_FIRST/ANY_FIRST_RUN distinction. jochen: I will notify you when I upload a patch set containing the cached default browser state we discussed. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:994: local_state_->SetBoolean(prefs::kWelcomePageOnOSUpgradeEnabled, false); On 2015/07/09 18:05:11, gab wrote: > Should be outside the |variations_seed| if (i.e. up one scope level from here). Absolutely. Nice catch. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/first_r... File chrome/browser/first_run/first_run.cc (right): https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/first_r... chrome/browser/first_run/first_run.cc:557: &value) && On 2015/07/09 18:05:11, gab wrote: > |value| is unchanged if this returns false (i.e. the pref is not there), so > having it default to true on 596 and calling the getter without interpreting the > return value is sufficient -- though I realize now that the way you wrote it is > also the way it's written for other bool prefs above so I guess this is fine.. > > tl;dr; I'm okay with this, but there's room for clean up.. I don't understand. The default value is used if the pref is not present by design. What do you think could be cleaned up? https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/signin/... File chrome/browser/signin/signin_promo.cc (right): https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/signin/... chrome/browser/signin/signin_promo.cc:124: if (is_new_profile && base::win::GetVersion() >= base::win::VERSION_WIN10) On 2015/07/09 18:05:11, gab wrote: > Should we add a > > DCHECK_EQ(base::win::VERSION_WIN10, base::win::GetVersion()) > << "Need to revisit first run sequence on new Windows version". > > here? I don't think so. It will annoy a random developer using a debug build rather than one of the deciders who will likely be using release builds to evaluate the experience. I will make it clear to the deciders that the new behavior they've asked for will continue on for future Windows versions until they file a bug to change it. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/signin/... chrome/browser/signin/signin_promo.cc:124: if (is_new_profile && base::win::GetVersion() >= base::win::VERSION_WIN10) On 2015/07/09 18:05:11, gab wrote: > optional: Feels this check should go first (before checking any of the prefs, > etc.). I debated that, but decided not to since they seemed to go in order of most general to most specific. The "on first run" part of this made it feel more specific to me than the general "is OTR," for example. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right): https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:97: // Presently, this is exactly the same set of platforms that support On 2015/07/09 18:05:11, gab wrote: > s/that support/that don't support/ ? Done. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:97: // Presently, this is exactly the same set of platforms that support On 2015/07/09 17:51:31, msw wrote: > But the condition is simple, and changes to ShowWelcomePageAfterFirstRun might > unwittingly alter this behavior too... Good point. I agree that it's not good to couple them together like this. > Why not just keep the logic separate, or > nix both for something like IsWindowsVersion10OrGreater() instead (not that I > think such a helper is needed)? I've made the platform and version check explicit here. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:294: g_browser_process->ReleaseModule(); On 2015/07/09 18:05:11, gab wrote: > (orthogonal: Wouldn't it be nice to have scoped objects for these refcounts, > eh...) Acknowledged. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:316: new_browser = FindTabbedBrowser(profile, true, host_desktop_type); On 2015/07/09 18:05:11, gab wrote: > The initial assignment above uses FindOneOtherBrowser(), I don't care which is > used, but we should be consistent (or comment on why it differs if it truly > does). Comment added. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:777: web_contents = tab_strip->GetWebContentsAt(1); On 2015/07/09 18:05:11, gab wrote: > When reading this I first got confused thinking this was the Welcome Page's > index and wondering why it wasn't first... > > I think this would be more readable if each WebContents* had its own explicitly > named variable then re-using a single one declared outside this scope On 2015/07/09 18:05:12, gab wrote: > Explicit names. i went ahead and removed these web_contents variables altogether. i think it's even more clear (and consistent) to have GetWebContentsAt() in the test expectation. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:1001: // launched, even if it will open just the home page (and the welcome page on On 2015/07/09 18:05:11, gab wrote: > s/home page/new tab page/ Done. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:1007: ASSERT_LT(0, tab_strip->count()); On 2015/07/09 18:05:12, gab wrote: > Redundant. removed https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:1012: web_contents = tab_strip->GetWebContentsAt(1); On 2015/07/09 18:05:12, gab wrote: > Same comment as above, suggest using explicit variable names. Done. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:1130: ASSERT_LT(0, tab_strip->count()); On 2015/07/09 18:05:11, gab wrote: > Redundant. removed https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:1135: web_contents = tab_strip->GetWebContentsAt(1); On 2015/07/09 18:05:12, gab wrote: > Explicit names. Done. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:562: InitializeWelcomeRunType(urls_to_open); On 2015/07/09 17:51:31, msw wrote: > I feel like there's a lot of unnecessary complexity here... Why doesn't this > member function simply add the welcome page to |urls_to_open| and record it as > shown, as needed? I don't see the value in having a separate state to > initialize, having the codepaths below actually add the page, and waiting until > the page is actually shown to record the prefs. I originally had it as you describe, but found that it was inadequate for the session restore case. Specifically, SessionService::RestoreIfNecessary is called in cases where there may or may not be a session restore. Either the pref update needs to be backed out if RestoreIfNecessary returns false, or the state in the pref needs to be committed after RestoreIfNecessary returns true. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:593: std::vector<GURL> adjust_urls(urls_to_open); On 2015/07/09 18:05:12, gab wrote: > s/adjust_urls/adjusted_urls/ Done. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:595: adjust_urls.push_back(internals::GetWelcomePageURL()); On 2015/07/09 18:05:12, gab wrote: > Doesn't this unconditionally put it last? adjust_urls will be empty, so it'll do the right thing. since that seems a bit subtle to me now that you point it out, i'll make it explicitly add it to the front (which will be the same, in practice). https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:679: std::vector<GURL> adjust_urls(urls_to_open); On 2015/07/09 18:05:13, gab wrote: > s/adjust_urls/adjusted_urls/ Done. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:681: adjust_urls.push_back(internals::GetWelcomePageURL()); On 2015/07/09 18:05:12, gab wrote: > Again, why is unconditionally pushing to the back okay here? Done. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:742: if (welcome_run_type_ == WelcomeRunType::ANY_RUN_FIRST) On 2015/07/09 18:05:12, gab wrote: > Why not also FIRST_RUN_FIRST? I guess I don't understand why you need the 4th > Type (why is NONE, FIRST, LAST not sufficient?) Here's the explanation I gave to msw: I didn't differentiate these initially, but it turned out to be significant. The handling of SessionStartupPref::URLS in StartupBrowserCreatorImpl::ProcessSpecifiedURLs shouldn't add the welcome page during first run since this would add it when an enterprise uses the kRestoreOnStartup policy to specify the tabs to be used for startup. An alternative is for InitializeWelcomeRunType to set the type to NONE if the startup URLs are specified by policy. I think that would get rid of the need for the extra value here and would potentially be safer for enterprises. What do you think? https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:743: UrlsToTabs(std::vector<GURL>(1, internals::GetWelcomePageURL()), &tabs); On 2015/07/09 18:05:12, gab wrote: > UrlsToTabs()'s name is misleading :-( -- it's really AppendTabsFromUrls() > > (I'm not suggesting you change it here, just stating my confusion) Acknowledged. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:933: // Put the welcome page before the new tab page on Windows 10. On 2015/07/09 17:51:31, msw wrote: > nit: remove the comment about win10. This code here isn't win10 specific, even > if the code setting |welcome_run_type_| is os/version specific. Done. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.h (right): https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.h:36: // upgrade, even when not in first-run. When this is true, the welcome page On 2015/07/09 18:05:13, gab wrote: > "even when not" suggests it *may also* happen during First Run; while the method > name "AfterFirstRun" suggests it's *only after* First Run (I still haven't read > further to figure out which is the truth, but this comment/method name could be > clearer). > > (addition) Ah, now that I'm reading the impl I see what this is checking, how > about "ShowWelcomePageAfterFirstRunAllowed()"? Also I think this is perhaps not > the best place to describe the behavior (i.e. this method only checks whether > it's allowed but doesn't enforce the behavior it documents) comment removed https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.h:38: bool ShowWelcomePageAfterFirstRun(); On 2015/07/09 17:51:31, msw wrote: > Inline this in StartupBrowserCreatorImpl::InitializeWelcomeRunType, and make the > tests similarly check the os/version inline. I don't see a good reason to > abstract this decision in this manner, nor explain this logic in the header. Done. https://codereview.chromium.org/1226643002/diff/200001/chrome/common/pref_nam... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/1226643002/diff/200001/chrome/common/pref_nam... chrome/common/pref_names.cc:1674: // WelcomePageOnOSUpgradeEnabled policy setting or by master_preferences. On 2015/07/09 18:05:13, gab wrote: > Technically it's not "overridden" by master_preferences, it's "set from" > master_preferences, WDYT of: > > // May be set from master_preferences or overridden by the > WelcomePageOnOSUpgradeEnabled policy setting. > > ? SGTM, thanks. https://codereview.chromium.org/1226643002/diff/200001/chrome/installer/util/... File chrome/installer/util/master_preferences.h (right): https://codereview.chromium.org/1226643002/diff/200001/chrome/installer/util/... chrome/installer/util/master_preferences.h:56: // "welcome_page_on_os_upgrade_enabled": false On 2015/07/09 18:05:13, gab wrote: > If anyone re-uses this "prototype" without giving it much thought, we should > suggest "true" as a default value here I think. I chose to show the setting that would change Chrome's behavior. Suggesting "true" is the same as not including the preference at all, which isn't useful. https://codereview.chromium.org/1226643002/diff/200001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/1226643002/diff/200001/components/policy/reso... components/policy/resources/policy_templates.json:7411: If this policy is enalbed and set to false, the browser will not re-show the welcome page on the first launch following an OS upgrade.''', On 2015/07/09 18:05:13, gab wrote: > s/enalbed/enabled Done.
The CQ bit was checked by grt@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, asvitkine@chromium.org, mnissler@chromium.org Link to the patchset: https://codereview.chromium.org/1226643002/#ps280001 (title: "removed need for ScopedAllowIO")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1226643002/280001
grt@chromium.org changed reviewers: - jam@chromium.org
-jam since the ScopedIOAllow is now gone. jochen, msw, gab: PTAL, taking into consideration that hodie says this must land asap to reach M44. I'm happy to make cleanups in a followon commit. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:562: InitializeWelcomeRunType(urls_to_open); On 2015/07/10 15:43:24, grt wrote: > On 2015/07/09 17:51:31, msw wrote: > > I feel like there's a lot of unnecessary complexity here... Why doesn't this > > member function simply add the welcome page to |urls_to_open| and record it as > > shown, as needed? I don't see the value in having a separate state to > > initialize, having the codepaths below actually add the page, and waiting > until > > the page is actually shown to record the prefs. > > I originally had it as you describe, but found that it was inadequate for the > session restore case. Specifically, SessionService::RestoreIfNecessary is called > in cases where there may or may not be a session restore. Either the pref update > needs to be backed out if RestoreIfNecessary returns false, or the state in the > pref needs to be committed after RestoreIfNecessary returns true. But if RestoreIfNecessary returns false, the code falls through to open the urls in another manner, so the welcome page would still be shown so long as it's included in |urls_to_open|, right? There must be some way to reasonably avoid the duplicated code across ProcessLaunchURLs, ProcessStartupURLs, ProcessSpecifiedURLs, and AddStartupURLs. At some point, the costs of added complexity aren't warranted by the corner cases it's covering (showing absolutely first/last in every scenario)... I wholeheartedly second atwilson's TODO ("Simplify the logic that decides which tabs to open on start-up and make it more consistent. http://crbug.com/248883") and wonder if you ought to find a reviewer more familiar with this code. https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.h (right): https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.h:94: FIRST_RUN_FIRST, // Inject the welcome page as the first first-run tab. On 2015/07/09 18:59:38, grt wrote: > On 2015/07/09 17:51:31, msw wrote: > > It doesn't seem like the enum needs to convey the reason for showing the > welcome > > page as the first tab... Even though the *FIRST value might be set for > different > > reasons, nothing actually triggers distinct behavior for FIRST_RUN_FIRST vs. > > ANY_RUN_FIRST, afaict... So why not just have 3 enum values: > > WELCOME_TAB_[NONE|FIRST|LAST]. Better yet, why don't we just always show the > > welcome tab first? Is there any reason to have distinct welcome tab ordering > for > > different cases? > > I didn't differentiate these initially, but it turned out to be significant. The > handling of SessionStartupPref::URLS in > StartupBrowserCreatorImpl::ProcessSpecifiedURLs shouldn't add the welcome page > during first run since this would add it when an enterprise uses the > kRestoreOnStartup policy to specify the tabs to be used for startup. > > An alternative is for InitializeWelcomeRunType to set the type to NONE if the > startup URLs are specified by policy. I think that would get rid of the need for > the extra value here and would potentially be safer for enterprises. What do you > think? That sounds plausible, but I might have to see it to understand it fully...
On 2015/07/10 19:40:45, msw wrote: > https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... > File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): > > https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... > chrome/browser/ui/startup/startup_browser_creator_impl.cc:562: > InitializeWelcomeRunType(urls_to_open); > On 2015/07/10 15:43:24, grt wrote: > > On 2015/07/09 17:51:31, msw wrote: > > > I feel like there's a lot of unnecessary complexity here... Why doesn't this > > > member function simply add the welcome page to |urls_to_open| and record it > as > > > shown, as needed? I don't see the value in having a separate state to > > > initialize, having the codepaths below actually add the page, and waiting > > until > > > the page is actually shown to record the prefs. > > > > I originally had it as you describe, but found that it was inadequate for the > > session restore case. Specifically, SessionService::RestoreIfNecessary is > called > > in cases where there may or may not be a session restore. Either the pref > update > > needs to be backed out if RestoreIfNecessary returns false, or the state in > the > > pref needs to be committed after RestoreIfNecessary returns true. > > But if RestoreIfNecessary returns false, the code falls through to open the urls > in another manner, so the welcome page would still be shown so long as it's > included in |urls_to_open|, right? There must be some way to reasonably avoid > the duplicated code across ProcessLaunchURLs, ProcessStartupURLs, > ProcessSpecifiedURLs, and AddStartupURLs. At some point, the costs of added > complexity aren't warranted by the corner cases it's covering (showing > absolutely first/last in every scenario)... I wholeheartedly second atwilson's > TODO ("Simplify the logic that decides which tabs to open on start-up and make > it more consistent. http://crbug.com/248883%22) and wonder if you ought to find a > reviewer more familiar with this code. > > https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... > File chrome/browser/ui/startup/startup_browser_creator_impl.h (right): > > https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... > chrome/browser/ui/startup/startup_browser_creator_impl.h:94: FIRST_RUN_FIRST, > // Inject the welcome page as the first first-run tab. > On 2015/07/09 18:59:38, grt wrote: > > On 2015/07/09 17:51:31, msw wrote: > > > It doesn't seem like the enum needs to convey the reason for showing the > > welcome > > > page as the first tab... Even though the *FIRST value might be set for > > different > > > reasons, nothing actually triggers distinct behavior for FIRST_RUN_FIRST vs. > > > ANY_RUN_FIRST, afaict... So why not just have 3 enum values: > > > WELCOME_TAB_[NONE|FIRST|LAST]. Better yet, why don't we just always show the > > > welcome tab first? Is there any reason to have distinct welcome tab ordering > > for > > > different cases? > > > > I didn't differentiate these initially, but it turned out to be significant. > The > > handling of SessionStartupPref::URLS in > > StartupBrowserCreatorImpl::ProcessSpecifiedURLs shouldn't add the welcome page > > during first run since this would add it when an enterprise uses the > > kRestoreOnStartup policy to specify the tabs to be used for startup. > > > > An alternative is for InitializeWelcomeRunType to set the type to NONE if the > > startup URLs are specified by policy. I think that would get rid of the need > for > > the extra value here and would potentially be safer for enterprises. What do > you > > think? > > That sounds plausible, but I might have to see it to understand it fully... What I have in mind is to return NONE from InitializeWelcomeRunType if kURLsToRestoreOnStartup is in-force by mandatory policy. I'd like to hold off for a followon commit, however.
PTAL https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/1226643002/diff/200001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:562: InitializeWelcomeRunType(urls_to_open); On 2015/07/10 19:40:45, msw wrote: > On 2015/07/10 15:43:24, grt wrote: > > On 2015/07/09 17:51:31, msw wrote: > > > I feel like there's a lot of unnecessary complexity here... Why doesn't this > > > member function simply add the welcome page to |urls_to_open| and record it > as > > > shown, as needed? I don't see the value in having a separate state to > > > initialize, having the codepaths below actually add the page, and waiting > > until > > > the page is actually shown to record the prefs. > > > > I originally had it as you describe, but found that it was inadequate for the > > session restore case. Specifically, SessionService::RestoreIfNecessary is > called > > in cases where there may or may not be a session restore. Either the pref > update > > needs to be backed out if RestoreIfNecessary returns false, or the state in > the > > pref needs to be committed after RestoreIfNecessary returns true. > > But if RestoreIfNecessary returns false, the code falls through to open the urls > in another manner, so the welcome page would still be shown so long as it's > included in |urls_to_open|, right? Ah, yes, I think I can get rid of RecordWelcomeRunComplete now that state is kept in the instance. I like this. Done. > There must be some way to reasonably avoid > the duplicated code across ProcessLaunchURLs, ProcessStartupURLs, > ProcessSpecifiedURLs, and AddStartupURLs. At some point, the costs of added > complexity aren't warranted by the corner cases it's covering (showing > absolutely first/last in every scenario)... Unfortunately, the corner cases are effectively all of the main use cases: startup with the default tabs, startup with a specific set of urls, and session restore. The existing tests are fairly comprehensive (and caught a regression I'd inadvertently introduced), so I don't think that this change is a risk of introducing a catastrophic break. > I wholeheartedly second atwilson's > TODO ("Simplify the logic that decides which tabs to open on start-up and make > it more consistent. http://crbug.com/248883%22) Me, too! It's a wreck. > and wonder if you ought to find a > reviewer more familiar with this code. I think one reason this code is so complicated now is that there is no true owner. PK was unable to suggest anyone, so I'm not sure who to turn to, other than the last people who have made edits. From what I've seen looking at the logs, most changes have been spot additions related to other things, so these people likely do not feel great ownership over it.
The CQ bit was checked by grt@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from mnissler@chromium.org, gab@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1226643002/#ps320001 (title: "simplified recording of welcome run complete")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1226643002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right): https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:275: if (IsWindows10OrNewer()) { Just add the welcome page url to the front of |urls| (or an |expected_urls| copy) in this case, and then just have one code path to check the tabs. https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:281: for (size_t i = 0; i < urls.size(); i++) { nit: curly braces aren't needed https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:288: for (size_t i = 0; i < urls.size(); i++) { nit: curly braces aren't needed https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:294: EXPECT_NE(tab_strip->GetWebContentsAt(0)->GetSiteInstance(), This isn't checked in the windows 10 case, but probably should be (or not at all); just check the two latter indices (urls.size()-1 and urls.size()-2) https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:298: // The test is complete if the welcome page wasn't included above. Run the remaining checks regardless? the expectations will be the same... Otherwise, maybe invert this and have a block for the remaining win10 checks? https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:320: // Find the new browser and ensure that it has only the expected URLs this nit: s/expected/specified/ https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:372: // The test is complete if the welcome page wasn't included above. Ditto, run the rest regardless... https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:642: tab_strip->GetWebContentsAt(GetNTPIndex())->GetURL()); If ShouldShowPromoAtStartup was true, are we guaranteed that this will be at index 0? (ditto below) Avoid the conditional index helper functions and be straightforward wherever possible. https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:577: adjusted_urls.insert(adjusted_urls.begin(), Could this be inserting the page at the front for a last case? I'd suggest adding a helper like void AddWelcomePageIfNeeded(std::vector<GURL>* urls_to_open) that would address the code duplication, inconsistency of supported enum values, and potential re-addition of the welcome page (by not setting |welcome_run_type_| to NONE after adding...), but I guess you need to potentially add the welcome page to the url vector multiple times (because restore might not take hold), and really, I'd like the existing helper to just add the page, as I requested before, but I guess that's not possible... It's hard to make sense of this mess. https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:663: adjusted_urls.insert(adjusted_urls.begin(), Why is this always inserting first, why doesn't this need to handle *_LAST? https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:724: if (welcome_run_type_ == WelcomeRunType::ANY_RUN_FIRST) Ditto: Why doesn't this need to handle FIRST_RUN_*? https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:972: ? WelcomeRunType::FIRST_RUN_FIRST It would be better to reduce the enum to NONE/FIRST/LAST now, rather than in a followup. Forgive me for being a bit skeptical that anyone would want to come back and resolve some technical debt in this file. https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:973: : WelcomeRunType::FIRST_RUN_LAST; I don't think you ever addressed my question: Better yet, why don't we just always show the welcome tab first? Is there any reason to have distinct welcome tab ordering for different cases? https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:1012: local_state->SetString(prefs::kLastWelcomedOSVersion, this_version); By inlining RecordWelcomeRunComplete here, that prevents addressing gab's point: Should we also reset |welcome_run_type_ = WelcomeRunType::NONE| here? i.e. I'm not 100% convinced by reading the above code that some other piece of code won't potentially end up handling something a second time based on this stale value (SO MUCH code potentially having access to this member.. startup/rendez-vous/other types of restores?/remote sync session restores?/bookmark folder restore?). Could the welcome page now be added twice in the spaghetti above? https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.h (right): https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.h:152: // welcome page is to be injected for future launches. nit: "is to be" or "is not to be"? no joke, won't this prevent future welcomes?
The CQ bit was checked by grt@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from mnissler@chromium.org, gab@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1226643002/#ps340001 (title: "more msw comments")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1226643002/340001
https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right): https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:275: if (IsWindows10OrNewer()) { On 2015/07/10 22:07:31, msw wrote: > Just add the welcome page url to the front of |urls| (or an |expected_urls| > copy) in this case, and then just have one code path to check the tabs. Done. https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:281: for (size_t i = 0; i < urls.size(); i++) { On 2015/07/10 22:07:31, msw wrote: > nit: curly braces aren't needed Done. https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:288: for (size_t i = 0; i < urls.size(); i++) { On 2015/07/10 22:07:31, msw wrote: > nit: curly braces aren't needed Done. https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:294: EXPECT_NE(tab_strip->GetWebContentsAt(0)->GetSiteInstance(), On 2015/07/10 22:07:31, msw wrote: > This isn't checked in the windows 10 case, but probably should be (or not at > all); just check the two latter indices (urls.size()-1 and urls.size()-2) Done. https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:298: // The test is complete if the welcome page wasn't included above. On 2015/07/10 22:07:32, msw wrote: > Run the remaining checks regardless? the expectations will be the same... > Otherwise, maybe invert this and have a block for the remaining win10 checks? Done. https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:320: // Find the new browser and ensure that it has only the expected URLs this On 2015/07/10 22:07:31, msw wrote: > nit: s/expected/specified/ Done. https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:372: // The test is complete if the welcome page wasn't included above. On 2015/07/10 22:07:31, msw wrote: > Ditto, run the rest regardless... Done. https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:642: tab_strip->GetWebContentsAt(GetNTPIndex())->GetURL()); On 2015/07/10 22:07:31, msw wrote: > If ShouldShowPromoAtStartup was true, are we guaranteed that this will be at > index 0? (ditto below) Avoid the conditional index helper functions and be > straightforward wherever possible. Done https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:577: adjusted_urls.insert(adjusted_urls.begin(), On 2015/07/10 22:07:32, msw wrote: > Could this be inserting the page at the front for a last case? I'd suggest > adding a helper like void AddWelcomePageIfNeeded(std::vector<GURL>* > urls_to_open)... This change started with a simple helper like that, but blossomed into the beautiful flower it has become because it had to handle all of the odd cases spread about this file. :-( > that would address the code duplication, inconsistency of > supported enum values, and potential re-addition of the welcome page (by not > setting |welcome_run_type_| to NONE after adding...), but I guess you need to > potentially add the welcome page to the url vector multiple times (because > restore might not take hold), and really, I'd like the existing helper to just > add the page, as I requested before, but I guess that's not possible... It's > hard to make sense of this mess. Agreed. https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:663: adjusted_urls.insert(adjusted_urls.begin(), On 2015/07/10 22:07:32, msw wrote: > Why is this always inserting first, why doesn't this need to handle *_LAST? This is handling session restore rather than first run. https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:724: if (welcome_run_type_ == WelcomeRunType::ANY_RUN_FIRST) On 2015/07/10 22:07:32, msw wrote: > Ditto: Why doesn't this need to handle FIRST_RUN_*? Because this isn't a first-run codepath. https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:972: ? WelcomeRunType::FIRST_RUN_FIRST On 2015/07/10 22:07:32, msw wrote: > It would be better to reduce the enum to NONE/FIRST/LAST now, rather than in a > followup. Forgive me for being a bit skeptical that anyone would want to come > back and resolve some technical debt in this file. I'll take a stab at it in the next patch set to see if/how it simplifies things. I will add my thoughts on how this class could be simplified and made unit-testable to the existing "the creator is too damn complex" bug. https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:973: : WelcomeRunType::FIRST_RUN_LAST; On 2015/07/10 22:07:32, msw wrote: > I don't think you ever addressed my question: Better yet, why don't we just > always show the welcome tab first? Is there any reason to have distinct welcome > tab ordering for different cases? Because this is what the product deciders have decided: prior to this change, the signin promo is king and takes the first slot in place of the NTP, while the welcome page takes the second slot. following this change, Win10 is special in that there is no signin promo and the welcome page takes the first slot while the NTP takes the second. https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:1012: local_state->SetString(prefs::kLastWelcomedOSVersion, this_version); On 2015/07/10 22:07:32, msw wrote: > By inlining RecordWelcomeRunComplete here, that prevents addressing gab's point: > > Should we also reset |welcome_run_type_ = WelcomeRunType::NONE| here? i.e. I'm > not 100% convinced by reading the above code that some other piece of code won't > potentially end up handling something a second time based on this stale value > (SO MUCH code potentially having access to this member.. > startup/rendez-vous/other types of restores?/remote sync session > restores?/bookmark folder restore?). > > Could the welcome page now be added twice in the spaghetti above? A potentially stale value is only alive for the lifetime of the StartupBrowserCreatorImpl, which is a stack object used to handle a single launch. It does not live for the lifetime of the browser process. There is no risk of the value being used for a later rendez-vous, other restore, etc. That said, if recording the welcome page is done in this function, then either: 1) welcome_run_type_ must retain its initial value so that if it isn't handled during session restore it can be handled in the fallback cases, or 2) extra handling is needed to revert the pref value and reset welcome_run_type_ when session restore fails so that it can be handled in the fallback case. Which takes us back to why I'd pulled it out into its own function in the first place. I'm happy to go back to using RecordWelcomeRunComplete if you find that cleaner (I did, since it was an attempt to record what had actually happened rather than what was expected to happen). https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.h (right): https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.h:152: // welcome page is to be injected for future launches. On 2015/07/10 22:07:32, msw wrote: > nit: "is to be" or "is not to be"? no joke, won't this prevent future welcomes? English... I wrote that thinking "...is to be injected on this run for the sake of decision making on future launches," but that is completely insane. Reworded.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) (exceeded global retry quota)
lgtm
https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:972: ? WelcomeRunType::FIRST_RUN_FIRST On 2015/07/11 12:16:08, grt wrote: > On 2015/07/10 22:07:32, msw wrote: > > It would be better to reduce the enum to NONE/FIRST/LAST now, rather than in a > > followup. Forgive me for being a bit skeptical that anyone would want to come > > back and resolve some technical debt in this file. > > I'll take a stab at it in the next patch set to see if/how it simplifies things. It turned out to be quite simple. PTAL.
The CQ bit was checked by grt@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, mnissler@chromium.org, gab@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1226643002/#ps380001 (title: "fix chromeos build")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1226643002/380001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) (exceeded global retry quota)
lgtm with a nit, thanks for bearing with all my comments and questions. I really think you should double check that the Win10-specific welcome tab ordering is important enough to justify its additional complexity. https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:973: : WelcomeRunType::FIRST_RUN_LAST; On 2015/07/11 12:16:08, grt wrote: > On 2015/07/10 22:07:32, msw wrote: > > I don't think you ever addressed my question: Better yet, why don't we just > > always show the welcome tab first? Is there any reason to have distinct > welcome > > tab ordering for different cases? > > Because this is what the product deciders have decided: prior to this change, > the signin promo is king and takes the first slot in place of the NTP, while the > welcome page takes the second slot. following this change, Win10 is special in > that there is no signin promo and the welcome page takes the first slot while > the NTP takes the second. Then let's double check with the deciders [sic]. When design preferences substantially increase code complexity and maintainability costs, it's worth circling back. At best, you might find out that the preference wasn't strong enough to merit such hassle, and simplicity wins out. At worst, we get this beautiful flower of a CL. Either way, the costs are revealed and there's more recognition of the technical debt here. https://codereview.chromium.org/1226643002/diff/320001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_impl.cc:1012: local_state->SetString(prefs::kLastWelcomedOSVersion, this_version); On 2015/07/11 12:16:08, grt wrote: > On 2015/07/10 22:07:32, msw wrote: > > By inlining RecordWelcomeRunComplete here, that prevents addressing gab's > point: > > > > Should we also reset |welcome_run_type_ = WelcomeRunType::NONE| here? i.e. I'm > > not 100% convinced by reading the above code that some other piece of code > won't > > potentially end up handling something a second time based on this stale value > > (SO MUCH code potentially having access to this member.. > > startup/rendez-vous/other types of restores?/remote sync session > > restores?/bookmark folder restore?). > > > > Could the welcome page now be added twice in the spaghetti above? > > A potentially stale value is only alive for the lifetime of the > StartupBrowserCreatorImpl, which is a stack object used to handle a single > launch. It does not live for the lifetime of the browser process. There is no > risk of the value being used for a later rendez-vous, other restore, etc. > > That said, if recording the welcome page is done in this function, then either: > 1) welcome_run_type_ must retain its initial value so that if it isn't handled > during session restore it can be handled in the fallback cases, or > 2) extra handling is needed to revert the pref value and reset welcome_run_type_ > when session restore fails so that it can be handled in the fallback case. > Which takes us back to why I'd pulled it out into its own function in the first > place. I'm happy to go back to using RecordWelcomeRunComplete if you find that > cleaner (I did, since it was an attempt to record what had actually happened > rather than what was expected to happen). I think it's okay as-is, but I don't feel too strongly either way. https://codereview.chromium.org/1226643002/diff/380001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right): https://codereview.chromium.org/1226643002/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:265: // The two non-welcome tabs, despite having the same site, should be in nit: s/non-welcome/test_server/
Thanks for the thorough review. I'll keep this class in mind when I have an itch to make the world a better place. This could be a case study as to why mixing policy and implementation is bad, and why designing to make code unit-testable from the get-go is a good thing. In theory, the SBCI has a simple job that could be exhaustively unit-tested. In practice... https://codereview.chromium.org/1226643002/diff/380001/chrome/browser/ui/star... File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right): https://codereview.chromium.org/1226643002/diff/380001/chrome/browser/ui/star... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:265: // The two non-welcome tabs, despite having the same site, should be in On 2015/07/13 19:16:06, msw wrote: > nit: s/non-welcome/test_server/ Done.
The CQ bit was checked by hodie@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, mnissler@chromium.org, gab@chromium.org, msw@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1226643002/#ps400001 (title: "comment fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1226643002/400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
policy_templates.json LGTM https://codereview.chromium.org/1226643002/diff/400001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/1226643002/diff/400001/components/policy/reso... components/policy/resources/policy_templates.json:7411: If this policy is enabled and set to false, the browser will not re-show the welcome page on the first launch following an OS upgrade.''', I think "If this policy is set to false" is better. Not clear what "enabled and set to false" means.
Thanks for taking a look. https://codereview.chromium.org/1226643002/diff/400001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/1226643002/diff/400001/components/policy/reso... components/policy/resources/policy_templates.json:7411: If this policy is enabled and set to false, the browser will not re-show the welcome page on the first launch following an OS upgrade.''', On 2015/07/14 13:52:44, Andrew T Wilson wrote: > I think "If this policy is set to false" is better. Not clear what "enabled and > set to false" means. Done.
The CQ bit was checked by grt@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, mnissler@chromium.org, gab@chromium.org, atwilson@chromium.org, msw@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1226643002/#ps420001 (title: "rewording in policy_templates.json and sync to position 338549")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1226643002/420001
Message was sent while issue was closed.
Committed patchset #16 (id:420001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/f14398c95a77206066cbab24b45070a85aba469a Cr-Commit-Position: refs/heads/master@{#338688} |
