Reland r318288: Wait until a new profile has been created before deleting the active profile.

Reland r318288: Wait until a new profile has been created before deleting the active profile.

Previous review URL:
https://codereview.chromium.org/953453002

BUG=460859
TBR=rsesek@chromium.org

Committed: https://crrev.com/447f6430eebb4d233ca2aa16db89e30d9a1f8705
Cr-Commit-Position: refs/heads/master@{#318775}

[Bernhard Bauer, 2015-02-27 14:01:48]

bauerb@chromium.org changed reviewers:
+ noms@chromium.org

[Bernhard Bauer, 2015-02-27 14:01:49]

Please review!

The version that was reverted is in patch set 1, for comparison. I changed the
ProfileManagerBrowserTest to use individual run loops instead of RunUntilIdle()
and quitting the current message loop.

[Bernhard Bauer, 2015-03-02 10:06:42]

bauerb@chromium.org changed reviewers:
+ mlerman@chromium.org
- noms@chromium.org

[Bernhard Bauer, 2015-03-02 10:06:43]

Mike, could you TAL while Monica is OOO? Thanks!

[Mike Lerman, 2015-03-02 15:06:11]

On 2015/03/02 10:06:43, Bernhard Bauer wrote:
> Mike, could you TAL while Monica is OOO? Thanks!

Hi, rather than creating a RunLoop and Wait()ing on it to complete, why not use
a WindowedNotificationObserver? The ProfileManager sends out a
NOTIFICATION_PROFILE_ADDED message whenever the profile's completed being added,
so you should be able to listen to that and have the same effect, with a bit
nicer code.

[Bernhard Bauer, 2015-03-02 16:25:52]

On 2015/03/02 15:06:11, Mike Lerman wrote:
> On 2015/03/02 10:06:43, Bernhard Bauer wrote:
> > Mike, could you TAL while Monica is OOO? Thanks!
> 
> Hi, rather than creating a RunLoop and Wait()ing on it to complete, why not
use
> a WindowedNotificationObserver? The ProfileManager sends out a
> NOTIFICATION_PROFILE_ADDED message whenever the profile's completed being
added,
> so you should be able to listen to that and have the same effect, with a bit
> nicer code.

Notifications are deprecated (see https://crbug.com/268984), and I assume that
NOTIFICATION_PROFILE_ADDED will eventually be replaced with a
ProfileInfoCacheObserver method. I could preempt that and implement a
ProfileInfoCacheObserver here, but we never actually wait for PROFILE_ADDED; we
either wait for a PasswordStoreConsumer callback, or for the callback we
directly pass to profile creation or profile deletion methods. I find that a bit
nicer, as it uses the publicly available API.

[noms (inactive), 2015-03-02 16:44:25]

On 2015/03/02 16:25:52, Bernhard Bauer wrote:
> On 2015/03/02 15:06:11, Mike Lerman wrote:
> > On 2015/03/02 10:06:43, Bernhard Bauer wrote:
> > > Mike, could you TAL while Monica is OOO? Thanks!
> > 
> > Hi, rather than creating a RunLoop and Wait()ing on it to complete, why not
> use
> > a WindowedNotificationObserver? The ProfileManager sends out a
> > NOTIFICATION_PROFILE_ADDED message whenever the profile's completed being
> added,
> > so you should be able to listen to that and have the same effect, with a bit
> > nicer code.
> 
> Notifications are deprecated (see https://crbug.com/268984), and I assume that
> NOTIFICATION_PROFILE_ADDED will eventually be replaced with a
> ProfileInfoCacheObserver method. I could preempt that and implement a
> ProfileInfoCacheObserver here, but we never actually wait for PROFILE_ADDED;
we
> either wait for a PasswordStoreConsumer callback, or for the callback we
> directly pass to profile creation or profile deletion methods. I find that a
bit
> nicer, as it uses the publicly available API.

Just butting in over here: PROFILE_ADDED will probably not be a
ProfileInfoCacheObserver method, but (maybe) a ProfileManagerObserver one, which
doesn't exist yet, and I'm not sure is a yak you want to adopt and shave :)
But if you do want to take that yak over: oh man, I'll send you so many cat gifs
as thanks.

[Mike Lerman, 2015-03-02 16:45:19]

On 2015/03/02 16:44:25, noms (OOO-slow to respond) wrote:
> On 2015/03/02 16:25:52, Bernhard Bauer wrote:
> > On 2015/03/02 15:06:11, Mike Lerman wrote:
> > > On 2015/03/02 10:06:43, Bernhard Bauer wrote:
> > > > Mike, could you TAL while Monica is OOO? Thanks!
> > > 
> > > Hi, rather than creating a RunLoop and Wait()ing on it to complete, why
not
> > use
> > > a WindowedNotificationObserver? The ProfileManager sends out a
> > > NOTIFICATION_PROFILE_ADDED message whenever the profile's completed being
> > added,
> > > so you should be able to listen to that and have the same effect, with a
bit
> > > nicer code.
> > 
> > Notifications are deprecated (see https://crbug.com/268984), and I assume
that
> > NOTIFICATION_PROFILE_ADDED will eventually be replaced with a
> > ProfileInfoCacheObserver method. I could preempt that and implement a
> > ProfileInfoCacheObserver here, but we never actually wait for PROFILE_ADDED;
> we
> > either wait for a PasswordStoreConsumer callback, or for the callback we
> > directly pass to profile creation or profile deletion methods. I find that a
> bit
> > nicer, as it uses the publicly available API.
> 
> Just butting in over here: PROFILE_ADDED will probably not be a
> ProfileInfoCacheObserver method, but (maybe) a ProfileManagerObserver one,
which
> doesn't exist yet, and I'm not sure is a yak you want to adopt and shave :)
> But if you do want to take that yak over: oh man, I'll send you so many cat
gifs
> as thanks.

Go back on vacation ;)

[Mike Lerman, 2015-03-02 18:37:39]

Okay, LGTM. Can you update the comments to reflect that we're using a "Wait"
behavior, rather than Spinning as the comments suggest?

Thanks!

https://codereview.chromium.org/962143002/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager_browsertest.cc (right):

https://codereview.chromium.org/962143002/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager_browsertest.cc:169: // Spin things till
profile is actually deleted.
Replace Spin with Wait

https://codereview.chromium.org/962143002/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager_browsertest.cc:317: // Spin to allow
profile creation to take place, loop is terminated
Replace "Spin" with "Wait", or somehow note that we're waiting for the callback
at the end of Profile Creation.

Both here and above, line 202.

[Bernhard Bauer, 2015-03-02 21:06:44]

bauerb@chromium.org changed reviewers:
+ rsesek@chromium.org

[Bernhard Bauer, 2015-03-02 21:06:45]

Thanks!

Robert: TBR for Mac code (no changes compared to the old version).

https://codereview.chromium.org/962143002/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager_browsertest.cc (right):

https://codereview.chromium.org/962143002/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager_browsertest.cc:169: // Spin things till
profile is actually deleted.
On 2015/03/02 18:37:38, Mike Lerman wrote:
> Replace Spin with Wait

Done.

https://codereview.chromium.org/962143002/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager_browsertest.cc:317: // Spin to allow
profile creation to take place, loop is terminated
On 2015/03/02 18:37:38, Mike Lerman wrote:
> Replace "Spin" with "Wait", or somehow note that we're waiting for the
callback
> at the end of Profile Creation.
> 
> Both here and above, line 202.

Done. (I used "run the message loop", as "spin" is sometimes used to mean
spinning the message loop, and sometimes means busy-waiting ("spinlock"), and
likewise "wait" sometimes means blocking the thread, and sometimes means running
a message loop.)

[Bernhard Bauer, 2015-03-02 21:07:41]

The CQ bit was checked by bauerb@chromium.org

[Bernhard Bauer, 2015-03-02 21:07:41]

The patchset sent to the CQ was uploaded after l-g-t-m from mlerman@chromium.org
Link to the patchset: https://codereview.chromium.org/962143002/#ps80001 (title:
"comments")

[commit-bot: I haz the power, 2015-03-02 21:09:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/962143002/80001

[Robert Sesek, 2015-03-02 21:51:05]

lgtm

[commit-bot: I haz the power, 2015-03-02 22:34:59]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-02 22:36:20]

Patchset 5 (id:??) landed as
https://crrev.com/447f6430eebb4d233ca2aa16db89e30d9a1f8705
Cr-Commit-Position: refs/heads/master@{#318775}