Moving new First Run to use per-profile value, rather than simply checking IsChromeFirstRun.

Moving new First Run to use per-profile value, rather than simply checking IsChromeFirstRun.

This solves several edge cases described in the bug.

BUG=660159
Committed: https://crrev.com/6254f4715b9a55b9dac805d432ce9f33376d65d6
Cr-Commit-Position: refs/heads/master@{#433466}

[tmartino, 2016-11-08 23:12:28]

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

[tmartino, 2016-11-08 23:12:29]

Adding gab@ for OWNERS on chrome/browser/prefs
Adding pkasting@ for OWNERS on remaining files

[gab, 2016-11-09 16:35:12]

prefs lgtm

[Peter Kasting, 2016-11-10 05:26:43]

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

https://codereview.chromium.org/2487553002/diff/20001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_tab_provider.cc:26: constexpr char
kVersionWelcomeIntroduced[] = "55.0.0.0";
I'm not a big fan of a version-number-based check for this.

You can use PrefService::HasPrefPath(prefs::kHasSeenWelcomeUI) to distinguish
between a pref that exists in a profile and one that doesn't.  Then just modify
the profile creation function in this CL to write "false" to that pref, and
HasPrefPath() will tell you if the profile is new enough or not.

That said, I think we can go even simpler: still have the profile creation code
write "false" for this pref, but register the pref with a default value of
"true".  Then you don't even need to check HasPrefPath(); just read the pref
value, and only show the UI if the pref is false.  (A clearer way of doing this
might be to reverse the sense of the pref to "should show welcome UI", default
to false, and have the profile creation function set it to true.)

https://codereview.chromium.org/2487553002/diff/20001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_tab_provider.cc:174:
url.append("?variant=everywhere");
Nit: Safer in case someone changes the welcome URL to have a query param:

  GURL url(chrome::kChromeUIWelcomeURL);
  return use_later_run_variant
             ? net::AppendQueryParameter(url, "variant", "everywhere")
             : url;

(You can use AppendOrReplaceQueryParameter() instead if you think someone might
add a default value of "variant" to the base URL and you want to make sure this
forces it to the value you want.)

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

https://codereview.chromium.org/2487553002/diff/20001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_tab_provider.h:62: // run policy.
Nit: Improve documentation of parameters; in particular,
|created_after_welcome_ui| isn't obvious.

https://codereview.chromium.org/2487553002/diff/20001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_tab_provider.h:94: // Gets the URL for the
"Welcome to Chrome" page.
Nit: Update to document the meaning of the parameter.

[tmartino, 2016-11-15 00:16:30]

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

https://codereview.chromium.org/2487553002/diff/20001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_tab_provider.cc:26: constexpr char
kVersionWelcomeIntroduced[] = "55.0.0.0";
On 2016/11/10 at 05:26:43, Peter Kasting wrote:
> I'm not a big fan of a version-number-based check for this.
> 
> You can use PrefService::HasPrefPath(prefs::kHasSeenWelcomeUI) to distinguish
between a pref that exists in a profile and one that doesn't.  Then just modify
the profile creation function in this CL to write "false" to that pref, and
HasPrefPath() will tell you if the profile is new enough or not.
> 
> That said, I think we can go even simpler: still have the profile creation
code write "false" for this pref, but register the pref with a default value of
"true".  Then you don't even need to check HasPrefPath(); just read the pref
value, and only show the UI if the pref is false.  (A clearer way of doing this
might be to reverse the sense of the pref to "should show welcome UI", default
to false, and have the profile creation function set it to true.)

Agreed on the structural changes, but I've looked at this a fair amount and I
still can't place which function "the profile creation function in this CL" is
supposed to be. Could you elaborate on that?

https://codereview.chromium.org/2487553002/diff/20001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_tab_provider.cc:174:
url.append("?variant=everywhere");
On 2016/11/10 at 05:26:43, Peter Kasting wrote:
> Nit: Safer in case someone changes the welcome URL to have a query param:
> 
>   GURL url(chrome::kChromeUIWelcomeURL);
>   return use_later_run_variant
>              ? net::AppendQueryParameter(url, "variant", "everywhere")
>              : url;
> 
> (You can use AppendOrReplaceQueryParameter() instead if you think someone
might add a default value of "variant" to the base URL and you want to make sure
this forces it to the value you want.)

Done

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

https://codereview.chromium.org/2487553002/diff/20001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_tab_provider.h:62: // run policy.
On 2016/11/10 at 05:26:43, Peter Kasting wrote:
> Nit: Improve documentation of parameters; in particular,
|created_after_welcome_ui| isn't obvious.

This will be obsolete pending changes from the other comment; see the question
there.

https://codereview.chromium.org/2487553002/diff/20001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_tab_provider.h:94: // Gets the URL for the
"Welcome to Chrome" page.
On 2016/11/10 at 05:26:43, Peter Kasting wrote:
> Nit: Update to document the meaning of the parameter.

Done.

[Peter Kasting, 2016-11-15 00:32:06]

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

https://codereview.chromium.org/2487553002/diff/20001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_tab_provider.cc:26: constexpr char
kVersionWelcomeIntroduced[] = "55.0.0.0";
On 2016/11/15 00:16:30, tmartino wrote:
> On 2016/11/10 at 05:26:43, Peter Kasting wrote:
> > I'm not a big fan of a version-number-based check for this.
> > 
> > You can use PrefService::HasPrefPath(prefs::kHasSeenWelcomeUI) to
distinguish
> between a pref that exists in a profile and one that doesn't.  Then just
modify
> the profile creation function in this CL to write "false" to that pref, and
> HasPrefPath() will tell you if the profile is new enough or not.
> > 
> > That said, I think we can go even simpler: still have the profile creation
> code write "false" for this pref, but register the pref with a default value
of
> "true".  Then you don't even need to check HasPrefPath(); just read the pref
> value, and only show the UI if the pref is false.  (A clearer way of doing
this
> might be to reverse the sense of the pref to "should show welcome UI", default
> to false, and have the profile creation function set it to true.)
> 
> Agreed on the structural changes, but I've looked at this a fair amount and I
> still can't place which function "the profile creation function in this CL" is
> supposed to be. Could you elaborate on that?

Sorry, I think you parsed the phrase differently than I meant it.  Remove the
phrase "in this CL" entirely; I just meant that within this CL, you could do the
changes I'm describing to whatever the profile creation function is.  I didn't
mean "this CL already adds a function that creates profiles, you should modify
it" or the like.

[tmartino, 2016-11-16 00:55:25]

On 2016/11/15 at 00:32:06, pkasting wrote:
>
https://codereview.chromium.org/2487553002/diff/20001/chrome/browser/ui/start...
> File chrome/browser/ui/startup/startup_tab_provider.cc (right):
> 
>
https://codereview.chromium.org/2487553002/diff/20001/chrome/browser/ui/start...
> chrome/browser/ui/startup/startup_tab_provider.cc:26: constexpr char
kVersionWelcomeIntroduced[] = "55.0.0.0";
> On 2016/11/15 00:16:30, tmartino wrote:
> > On 2016/11/10 at 05:26:43, Peter Kasting wrote:
> > > I'm not a big fan of a version-number-based check for this.
> > > 
> > > You can use PrefService::HasPrefPath(prefs::kHasSeenWelcomeUI) to
distinguish
> > between a pref that exists in a profile and one that doesn't.  Then just
modify
> > the profile creation function in this CL to write "false" to that pref, and
> > HasPrefPath() will tell you if the profile is new enough or not.
> > > 
> > > That said, I think we can go even simpler: still have the profile creation
> > code write "false" for this pref, but register the pref with a default value
of
> > "true".  Then you don't even need to check HasPrefPath(); just read the pref
> > value, and only show the UI if the pref is false.  (A clearer way of doing
this
> > might be to reverse the sense of the pref to "should show welcome UI",
default
> > to false, and have the profile creation function set it to true.)
> > 
> > Agreed on the structural changes, but I've looked at this a fair amount and
I
> > still can't place which function "the profile creation function in this CL"
is
> > supposed to be. Could you elaborate on that?
> 
> Sorry, I think you parsed the phrase differently than I meant it.  Remove the
phrase "in this CL" entirely; I just meant that within this CL, you could do the
changes I'm describing to whatever the profile creation function is.  I didn't
mean "this CL already adds a function that creates profiles, you should modify
it" or the like.

OK. I've tried doing it this way quite a bit. Specifically the most promising
attempt involved adding the "set this for new profiles" step to profile_impl's
DoFinalInit method, which seems to be the proper place for this sort of thing.
(As an example,
https://cs.chromium.org/chromium/src/chrome/browser/signin/signin_ui_util.cc?...
is doing a similar thing.)  Unfortunately, the callchain leading to DoFinalInit
can be invoked asynchronously, and in fact I was able to trigger a race
condition where this was not set in time to be reflected in StartupTabProvider.
So as far as I can tell, doing something at profile-create-time is probably not
going to work.

[Peter Kasting, 2016-11-16 01:22:31]

On 2016/11/16 00:55:25, tmartino wrote:
> On 2016/11/15 at 00:32:06, pkasting wrote:
> >
>
https://codereview.chromium.org/2487553002/diff/20001/chrome/browser/ui/start...
> > File chrome/browser/ui/startup/startup_tab_provider.cc (right):
> > 
> >
>
https://codereview.chromium.org/2487553002/diff/20001/chrome/browser/ui/start...
> > chrome/browser/ui/startup/startup_tab_provider.cc:26: constexpr char
> kVersionWelcomeIntroduced[] = "55.0.0.0";
> > On 2016/11/15 00:16:30, tmartino wrote:
> > > On 2016/11/10 at 05:26:43, Peter Kasting wrote:
> > > > I'm not a big fan of a version-number-based check for this.
> > > > 
> > > > You can use PrefService::HasPrefPath(prefs::kHasSeenWelcomeUI) to
> distinguish
> > > between a pref that exists in a profile and one that doesn't.  Then just
> modify
> > > the profile creation function in this CL to write "false" to that pref,
and
> > > HasPrefPath() will tell you if the profile is new enough or not.
> > > > 
> > > > That said, I think we can go even simpler: still have the profile
creation
> > > code write "false" for this pref, but register the pref with a default
value
> of
> > > "true".  Then you don't even need to check HasPrefPath(); just read the
pref
> > > value, and only show the UI if the pref is false.  (A clearer way of doing
> this
> > > might be to reverse the sense of the pref to "should show welcome UI",
> default
> > > to false, and have the profile creation function set it to true.)
> > > 
> > > Agreed on the structural changes, but I've looked at this a fair amount
and
> I
> > > still can't place which function "the profile creation function in this
CL"
> is
> > > supposed to be. Could you elaborate on that?
> > 
> > Sorry, I think you parsed the phrase differently than I meant it.  Remove
the
> phrase "in this CL" entirely; I just meant that within this CL, you could do
the
> changes I'm describing to whatever the profile creation function is.  I didn't
> mean "this CL already adds a function that creates profiles, you should modify
> it" or the like.
> 
> OK. I've tried doing it this way quite a bit. Specifically the most promising
> attempt involved adding the "set this for new profiles" step to profile_impl's
> DoFinalInit method, which seems to be the proper place for this sort of thing.
> (As an example,
>
https://cs.chromium.org/chromium/src/chrome/browser/signin/signin_ui_util.cc?...
> is doing a similar thing.)  Unfortunately, the callchain leading to
DoFinalInit
> can be invoked asynchronously, and in fact I was able to trigger a race
> condition where this was not set in time to be reflected in
StartupTabProvider.
> So as far as I can tell, doing something at profile-create-time is probably
not
> going to work.

Can't we do this earlier than at that stage?  I was thinking of writing this out
ASAP (i.e. immediately after the pref store exists at all).

Separately, it sounds like a potential source of bugs that a profile has not
been finalized before we're already trying to launch a browser with it.

[tmartino, 2016-11-17 19:28:01]

I spent some time talking to anthonyvd, who is an OWNER of the profiles
directory, and we couldn't come up with an appropriate solution this way. The
constructor of ProfileImpl calls RegisterUserProfilePrefs (which registers the
pref via our method in StartupBrowserCreator, but which doesn't have a handle on
the Profile), loads pref values either synchronously or asynchronously, and then
calls OnPrefsLoaded (which, via a few more calls, seems to be the appropriate
place for this sort of thing, and which is where I initially tried to insert
this). There just isn't a lot of room between registering the prefs and loading
their values, plus, none of it seems like a structurally appropriate place, and
it's unclear to me what the effect of setting this before loading the prefs
would or should be.

I agree that async pref loading is...troubling. That tradeoff between launch
speed and stability doesn't really make sense to me.

+mlerman: Per anthonyvd's suggestion, I'm adding mlerman who may know an
approach to this problem from his prior work. Mike, for context, what we're
trying to do is set a default value for a pref, but override it when a profile
is created for the first time--essentially, we are tracking whether we've shown
a new page to a particular profile, but we want to assume we've shown it to
profiles which predate this page. We're looking for an appropriate place to
handle this in the profile creation flow, but need to be careful because this
value is used at launch time, and doing it after async pref loading completes
creates a meaningful race condition.

[Peter Kasting, 2016-11-17 19:37:31]

On 2016/11/17 19:28:01, tmartino wrote:
> I spent some time talking to anthonyvd, who is an OWNER of the profiles
> directory, and we couldn't come up with an appropriate solution this way. The
> constructor of ProfileImpl calls RegisterUserProfilePrefs (which registers the
> pref via our method in StartupBrowserCreator, but which doesn't have a handle
on
> the Profile), loads pref values either synchronously or asynchronously, and
then
> calls OnPrefsLoaded (which, via a few more calls, seems to be the appropriate
> place for this sort of thing, and which is where I initially tried to insert
> this). There just isn't a lot of room between registering the prefs and
loading
> their values, plus, none of it seems like a structurally appropriate place,
and
> it's unclear to me what the effect of setting this before loading the prefs
> would or should be.

If pref loading is async, can arbitrary amounts of Chrome code run before it
completes?  Because lots of code expects to read/write prefs, and I'm wondering
if this could introduce an effectively-arbitrary number of (hopefully rare)
cases where the Wrong Thing happens.  If we deal with writes before this point
by buffering them, you'd be safe to write the pref "too early".

Or is there a point during startup where we kick off an async prefs load, and
then a point where we block if it hasn't completed yet?  If so, is it possible
to block the decision about whether this is first run to happen after this
point?

[Mike Lerman, 2016-11-17 20:17:16]

On 2016/11/17 19:37:31, Peter Kasting wrote:
> On 2016/11/17 19:28:01, tmartino wrote:
> > I spent some time talking to anthonyvd, who is an OWNER of the profiles
> > directory, and we couldn't come up with an appropriate solution this way.
The
> > constructor of ProfileImpl calls RegisterUserProfilePrefs (which registers
the
> > pref via our method in StartupBrowserCreator, but which doesn't have a
handle
> on
> > the Profile), loads pref values either synchronously or asynchronously, and
> then
> > calls OnPrefsLoaded (which, via a few more calls, seems to be the
appropriate
> > place for this sort of thing, and which is where I initially tried to insert
> > this). There just isn't a lot of room between registering the prefs and
> loading
> > their values, plus, none of it seems like a structurally appropriate place,
> and
> > it's unclear to me what the effect of setting this before loading the prefs
> > would or should be.
> 
> If pref loading is async, can arbitrary amounts of Chrome code run before it
> completes?  Because lots of code expects to read/write prefs, and I'm
wondering
> if this could introduce an effectively-arbitrary number of (hopefully rare)
> cases where the Wrong Thing happens.  If we deal with writes before this point
> by buffering them, you'd be safe to write the pref "too early".
> 
> Or is there a point during startup where we kick off an async prefs load, and
> then a point where we block if it hasn't completed yet?  If so, is it possible
> to block the decision about whether this is first run to happen after this
> point?

Side notes:
StartupBrowserCreator::ProcessCmdLineImpl has dependencies on the prefs service,
for determining if Incognito can/should be used/forced. I wonder if they're
running into the same issue.
Bonus points to someone who removes line 840 from profile_impl.cc, which should
have been removed in M28 according to sky@.

About Synchronicity:
When the Profile is created synchronously (as it is for the launch of
Chrome.exe), then the prefs are also loaded synchronously. See the async_prefs
variable in profile_impl.cc.
I'm assuming you're thus concerned about asynchronous profile creation...
although the Browser won't get started until the Profile is created.
So... doesn't that do the trick?

Ideas - although I don't think they're necessary.
You could perhaps force loading of this particular pref in
ProfileManager::DoFinalInitForServices, which is called before Synchronous
profile creation completes.
InitProfileUserPrefs, although not well named for your purpose, could also be a
reasonable spot, although this really just contains things that Profiles UI
needs to display profiles.
Basically I'd focus in on ProfileManager::AddProfile(), and the methods called
there. Nothing uses a Profile before it's been added to the ProfileManager.

[tmartino, 2016-11-17 20:56:01]

On 2016/11/17 at 20:17:16, mlerman wrote:
> On 2016/11/17 19:37:31, Peter Kasting wrote:
> > On 2016/11/17 19:28:01, tmartino wrote:
> > > I spent some time talking to anthonyvd, who is an OWNER of the profiles
> > > directory, and we couldn't come up with an appropriate solution this way.
The
> > > constructor of ProfileImpl calls RegisterUserProfilePrefs (which registers
the
> > > pref via our method in StartupBrowserCreator, but which doesn't have a
handle
> > on
> > > the Profile), loads pref values either synchronously or asynchronously,
and
> > then
> > > calls OnPrefsLoaded (which, via a few more calls, seems to be the
appropriate
> > > place for this sort of thing, and which is where I initially tried to
insert
> > > this). There just isn't a lot of room between registering the prefs and
> > loading
> > > their values, plus, none of it seems like a structurally appropriate
place,
> > and
> > > it's unclear to me what the effect of setting this before loading the
prefs
> > > would or should be.
> > 
> > If pref loading is async, can arbitrary amounts of Chrome code run before it
> > completes?  Because lots of code expects to read/write prefs, and I'm
wondering
> > if this could introduce an effectively-arbitrary number of (hopefully rare)
> > cases where the Wrong Thing happens.  If we deal with writes before this
point
> > by buffering them, you'd be safe to write the pref "too early".
> > 
> > Or is there a point during startup where we kick off an async prefs load,
and
> > then a point where we block if it hasn't completed yet?  If so, is it
possible
> > to block the decision about whether this is first run to happen after this
> > point?
> 
> Side notes:
> StartupBrowserCreator::ProcessCmdLineImpl has dependencies on the prefs
service, for determining if Incognito can/should be used/forced. I wonder if
they're running into the same issue.
> Bonus points to someone who removes line 840 from profile_impl.cc, which
should have been removed in M28 according to sky@.
> 
> About Synchronicity:
> When the Profile is created synchronously (as it is for the launch of
Chrome.exe), then the prefs are also loaded synchronously. See the async_prefs
variable in profile_impl.cc.
> I'm assuming you're thus concerned about asynchronous profile creation...
although the Browser won't get started until the Profile is created.
> So... doesn't that do the trick?

The trick is that something that acts like a launch will happen (and load a
profile asynchronously) when switching profiles/creating a new profile while
Chrome is already running, and that's what caused the race condition I found.
> 
> Ideas - although I don't think they're necessary.
> You could perhaps force loading of this particular pref in
ProfileManager::DoFinalInitForServices, which is called before Synchronous
profile creation completes.
> InitProfileUserPrefs, although not well named for your purpose, could also be
a reasonable spot, although this really just contains things that Profiles UI
needs to display profiles.
> Basically I'd focus in on ProfileManager::AddProfile(), and the methods called
there. Nothing uses a Profile before it's been added to the ProfileManager.

I'll test doing this in the ProfileManager and report back. Thanks!

[tmartino, 2016-11-18 00:04:54]

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

[tmartino, 2016-11-18 00:06:36]

On 2016/11/17 at 20:56:01, tmartino wrote:
> On 2016/11/17 at 20:17:16, mlerman wrote:
> > On 2016/11/17 19:37:31, Peter Kasting wrote:
> > > On 2016/11/17 19:28:01, tmartino wrote:
> > > > I spent some time talking to anthonyvd, who is an OWNER of the profiles
> > > > directory, and we couldn't come up with an appropriate solution this
way. The
> > > > constructor of ProfileImpl calls RegisterUserProfilePrefs (which
registers the
> > > > pref via our method in StartupBrowserCreator, but which doesn't have a
handle
> > > on
> > > > the Profile), loads pref values either synchronously or asynchronously,
and
> > > then
> > > > calls OnPrefsLoaded (which, via a few more calls, seems to be the
appropriate
> > > > place for this sort of thing, and which is where I initially tried to
insert
> > > > this). There just isn't a lot of room between registering the prefs and
> > > loading
> > > > their values, plus, none of it seems like a structurally appropriate
place,
> > > and
> > > > it's unclear to me what the effect of setting this before loading the
prefs
> > > > would or should be.
> > > 
> > > If pref loading is async, can arbitrary amounts of Chrome code run before
it
> > > completes?  Because lots of code expects to read/write prefs, and I'm
wondering
> > > if this could introduce an effectively-arbitrary number of (hopefully
rare)
> > > cases where the Wrong Thing happens.  If we deal with writes before this
point
> > > by buffering them, you'd be safe to write the pref "too early".
> > > 
> > > Or is there a point during startup where we kick off an async prefs load,
and
> > > then a point where we block if it hasn't completed yet?  If so, is it
possible
> > > to block the decision about whether this is first run to happen after this
> > > point?
> > 
> > Side notes:
> > StartupBrowserCreator::ProcessCmdLineImpl has dependencies on the prefs
service, for determining if Incognito can/should be used/forced. I wonder if
they're running into the same issue.
> > Bonus points to someone who removes line 840 from profile_impl.cc, which
should have been removed in M28 according to sky@.
> > 
> > About Synchronicity:
> > When the Profile is created synchronously (as it is for the launch of
Chrome.exe), then the prefs are also loaded synchronously. See the async_prefs
variable in profile_impl.cc.
> > I'm assuming you're thus concerned about asynchronous profile creation...
although the Browser won't get started until the Profile is created.
> > So... doesn't that do the trick?
> 
> The trick is that something that acts like a launch will happen (and load a
profile asynchronously) when switching profiles/creating a new profile while
Chrome is already running, and that's what caused the race condition I found.
> > 
> > Ideas - although I don't think they're necessary.
> > You could perhaps force loading of this particular pref in
ProfileManager::DoFinalInitForServices, which is called before Synchronous
profile creation completes.
> > InitProfileUserPrefs, although not well named for your purpose, could also
be a reasonable spot, although this really just contains things that Profiles UI
needs to display profiles.
> > Basically I'd focus in on ProfileManager::AddProfile(), and the methods
called there. Nothing uses a Profile before it's been added to the
ProfileManager.
> 
> I'll test doing this in the ProfileManager and report back. Thanks!

OK, the ProfileManager::AddProfile approach seems to have resolved the issues I
was seeing. Thanks for the help.

pkasting: PTAL

[commit-bot: I haz the power, 2016-11-18 00:07:46]

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

[commit-bot: I haz the power, 2016-11-18 00:14:54]

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

[commit-bot: I haz the power, 2016-11-18 00:14:55]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[tmartino, 2016-11-18 14:29:12]

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

[commit-bot: I haz the power, 2016-11-18 14:29:29]

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

[commit-bot: I haz the power, 2016-11-18 14:54:32]

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

[commit-bot: I haz the power, 2016-11-18 14:54:33]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[tmartino, 2016-11-18 19:24:29]

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

[commit-bot: I haz the power, 2016-11-18 19:25:21]

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

[commit-bot: I haz the power, 2016-11-18 19:51:03]

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

[commit-bot: I haz the power, 2016-11-18 19:51:04]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[tmartino, 2016-11-18 20:06:14]

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

[commit-bot: I haz the power, 2016-11-18 20:07:17]

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

[tmartino, 2016-11-18 21:51:57]

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

[tmartino, 2016-11-18 21:51:58]

OK, looks like the try bot issues are fixed now, so please take a look.

anthonyvd from CC to OWNERS for chrome/browser/profiles/

[anthonyvd, 2016-11-18 21:56:22]

c/b/profiles/profile_manager.cc lgtm

[commit-bot: I haz the power, 2016-11-18 23:30:51]

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

[commit-bot: I haz the power, 2016-11-18 23:30:52]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2016-11-20 07:32:09]

LGTM.  Thanks for spending so much time trying to get the pref-based system to
work.  It's not a huge difference in the amount of code, but I think it will be
more robust, especially for people who rebrand the browser or testers/developers
who try using the same profile with different Chrome versions.  And it's
definitely easier for me to understand what it's doing.

My only meta-comment is that your terminology in this CL for the welcome UI --
other than the word "welcome" -- is very inconsistent.  I would try to be as
consistent as possible in how you refer to this in comments and code to make it
clearer that these all refer to exactly the same thing (no subset/superset
issues), and to make findind references to this easier later.  I saw all the
following:

"chrome://welcome First Run page"
"WelcomeUI"
"Welcome screen"
"welcome page"
"WelcomePageUrl"
"welcome"
"'Welcome to Chrome' page"

https://codereview.chromium.org/2487553002/diff/130001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/2487553002/diff/130001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:931: if (profile->IsNewProfile()) {
Nit: No {}

https://codereview.chromium.org/2487553002/diff/130001/chrome/browser/ui/star...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/2487553002/diff/130001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator.cc:488:
registry->RegisterBooleanPref(prefs::kHasSeenWelcomeUI, true);
Nit: I would add a comment about why we default this to true.

https://codereview.chromium.org/2487553002/diff/130001/chrome/browser/ui/star...
File chrome/browser/ui/startup/startup_tab_provider.cc (right):

https://codereview.chromium.org/2487553002/diff/130001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_tab_provider.cc:47: 
Nit: I'd probably remove this blank line since the variables above are defined
solely to be used in this next call

[tmartino, 2016-11-21 00:23:45]

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

[commit-bot: I haz the power, 2016-11-21 00:23:53]

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

[tmartino, 2016-11-21 00:27:35]

Agreed on the terminology--it's one of those things that you lose track of when
you've been working on a feature for so long, but that looks pretty bad for an
external reader. I did a pass on my changes here and tried to standardize on
"Welcome page."

https://codereview.chromium.org/2487553002/diff/130001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/2487553002/diff/130001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:931: if (profile->IsNewProfile()) {
On 2016/11/20 at 07:32:09, Peter Kasting wrote:
> Nit: No {}

Done

https://codereview.chromium.org/2487553002/diff/130001/chrome/browser/ui/star...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/2487553002/diff/130001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator.cc:488:
registry->RegisterBooleanPref(prefs::kHasSeenWelcomeUI, true);
On 2016/11/20 at 07:32:09, Peter Kasting wrote:
> Nit: I would add a comment about why we default this to true.

Good call, done

https://codereview.chromium.org/2487553002/diff/130001/chrome/browser/ui/star...
File chrome/browser/ui/startup/startup_tab_provider.cc (right):

https://codereview.chromium.org/2487553002/diff/130001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_tab_provider.cc:47: 
On 2016/11/20 at 07:32:09, Peter Kasting wrote:
> Nit: I'd probably remove this blank line since the variables above are defined
solely to be used in this next call

Done

[commit-bot: I haz the power, 2016-11-21 01:05:26]

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

[commit-bot: I haz the power, 2016-11-21 01:05:27]

Dry run: This issue passed the CQ dry run.

[tmartino, 2016-11-21 01:19:24]

The CQ bit was checked by tmartino@chromium.org

[tmartino, 2016-11-21 01:19:24]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
pkasting@chromium.org, anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/2487553002/#ps150001
(title: "Addressing pkasting feedback")

[commit-bot: I haz the power, 2016-11-21 01:19:37]

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

[commit-bot: I haz the power, 2016-11-21 01:22:59]

CQ is committing da patch.

Bot data: {"patchset_id": 150001, "attempt_start_ts": 1479691164184740,
"parent_rev": ["a4f943e3691a26862f73406d5df9b37b44272b55", null], "commit_rev":
["35bc97262d77e1bcce75fa7ddb1f8b44c9e010d7", null]}

[commit-bot: I haz the power, 2016-11-21 01:23:26]

Committed patchset #9 (id:150001)

[commit-bot: I haz the power, 2016-11-21 01:25:22]

Description was changed from

==========
Moving new First Run to use per-profile value, rather than simply checking
IsChromeFirstRun.

This solves several edge cases described in the bug.

BUG=660159
==========

to

==========
Moving new First Run to use per-profile value, rather than simply checking
IsChromeFirstRun.

This solves several edge cases described in the bug.

BUG=660159

Committed: https://crrev.com/6254f4715b9a55b9dac805d432ce9f33376d65d6
Cr-Commit-Position: refs/heads/master@{#433466}
==========

[commit-bot: I haz the power, 2016-11-21 01:25:23]

Patchset 9 (id:??) landed as
https://crrev.com/6254f4715b9a55b9dac805d432ce9f33376d65d6
Cr-Commit-Position: refs/heads/master@{#433466}