[Mac] AppControllerMac updates the window when a Profile is deleted.

AppControllerMac updates the window when a Profile is deleted.

BUG=477392
TEST=(Mac only) Have at least two profiles. Open one of the profiles. navigate to generate browser history, verify it in the History Menu. Delete the profile; note that history should no longer show the deleted profile's history.

Committed: https://crrev.com/ef6d33eabfb3d553153217a3b874dd6a0cfdf961
Cr-Commit-Position: refs/heads/master@{#326281}

[Mike Lerman, 2015-04-21 01:24:39]

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

[Mike Lerman, 2015-04-21 01:24:40]

Hi Robert,

I got a test working that correctly validates the change that I made. I had run
into an interesting issue - I shouldn't have been alloc'ing and creating a new
AppController, BrowserTests already create one. Anywho, this works and validates
what I wrote. Please take a look.

Thanks,
Mike

[Robert Sesek, 2015-04-21 18:02:42]

https://codereview.chromium.org/1097553002/diff/60001/chrome/browser/app_cont...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/1097553002/diff/60001/chrome/browser/app_cont...
chrome/browser/app_controller_mac.mm:897: // Force windowChangedToProfile to set
the lastProfile_ and also update the
nit: : after windowChangedToProfile (colons are part of the method name).

https://codereview.chromium.org/1097553002/diff/60001/chrome/browser/app_cont...
chrome/browser/app_controller_mac.mm:899: lastProfile_ = NULL;
nullptr

https://codereview.chromium.org/1097553002/diff/60001/chrome/browser/app_cont...
File chrome/browser/app_controller_mac_browsertest.mm (right):

https://codereview.chromium.org/1097553002/diff/60001/chrome/browser/app_cont...
chrome/browser/app_controller_mac_browsertest.mm:110: void
WaitForHistoryBackendToRun(Profile* profile) {
Can you use

base::RunLoop run_loop;
HistoryService::FlushForTest(run_loop.QuitClosure());
run_loop.Run();

instead?

(I'm not sure what FlushForTest is for).

https://codereview.chromium.org/1097553002/diff/60001/chrome/browser/app_cont...
chrome/browser/app_controller_mac_browsertest.mm:474: // // Create profile 2.
nit: extra //

https://codereview.chromium.org/1097553002/diff/60001/chrome/browser/app_cont...
chrome/browser/app_controller_mac_browsertest.mm:478:
profile_manager->CreateProfileAsync(
Can you create the profile synchronously, like on line 545?

[Mike Lerman, 2015-04-21 19:33:17]

Thanks Robert.

https://codereview.chromium.org/1097553002/diff/60001/chrome/browser/app_cont...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/1097553002/diff/60001/chrome/browser/app_cont...
chrome/browser/app_controller_mac.mm:897: // Force windowChangedToProfile to set
the lastProfile_ and also update the
On 2015/04/21 18:02:42, Robert Sesek wrote:
> nit: : after windowChangedToProfile (colons are part of the method name).

Oh Cocoa.
Done.

https://codereview.chromium.org/1097553002/diff/60001/chrome/browser/app_cont...
chrome/browser/app_controller_mac.mm:899: lastProfile_ = NULL;
On 2015/04/21 18:02:42, Robert Sesek wrote:
> nullptr

Done.

https://codereview.chromium.org/1097553002/diff/60001/chrome/browser/app_cont...
File chrome/browser/app_controller_mac_browsertest.mm (right):

https://codereview.chromium.org/1097553002/diff/60001/chrome/browser/app_cont...
chrome/browser/app_controller_mac_browsertest.mm:110: void
WaitForHistoryBackendToRun(Profile* profile) {
On 2015/04/21 18:02:42, Robert Sesek wrote:
> Can you use
> 
> base::RunLoop run_loop;
> HistoryService::FlushForTest(run_loop.QuitClosure());
> run_loop.Run();
> 
> instead?
> 
> (I'm not sure what FlushForTest is for).

We actually don't need to load the Backend at all in this test - we are just
testing the Service pointers change.

https://codereview.chromium.org/1097553002/diff/60001/chrome/browser/app_cont...
chrome/browser/app_controller_mac_browsertest.mm:474: // // Create profile 2.
On 2015/04/21 18:02:42, Robert Sesek wrote:
> nit: extra //

Done.

https://codereview.chromium.org/1097553002/diff/60001/chrome/browser/app_cont...
chrome/browser/app_controller_mac_browsertest.mm:478:
profile_manager->CreateProfileAsync(
On 2015/04/21 18:02:42, Robert Sesek wrote:
> Can you create the profile synchronously, like on line 545?

The command on line 545 doesn't do all the same things CreateProfileAsync does.
CreateProfile creates a Profile*. CreateProfileAsync calls CreateProfile, but
also adds the Profile* to the ProfileManager and the ProfileInfoCache - the
latter is key since the AppController learns the profile is deleted by Observing
the ProfileInfoCache.

[Robert Sesek, 2015-04-21 20:12:30]

Sorry, should have noticed this first time around. Don't DCHECK in tests, use
the expect macros.

https://codereview.chromium.org/1097553002/diff/60001/chrome/browser/app_cont...
File chrome/browser/app_controller_mac_browsertest.mm (right):

https://codereview.chromium.org/1097553002/diff/60001/chrome/browser/app_cont...
chrome/browser/app_controller_mac_browsertest.mm:478:
profile_manager->CreateProfileAsync(
On 2015/04/21 19:33:16, Mike Lerman wrote:
> On 2015/04/21 18:02:42, Robert Sesek wrote:
> > Can you create the profile synchronously, like on line 545?
> 
> The command on line 545 doesn't do all the same things CreateProfileAsync
does.
> CreateProfile creates a Profile*. CreateProfileAsync calls CreateProfile, but
> also adds the Profile* to the ProfileManager and the ProfileInfoCache - the
> latter is key since the AppController learns the profile is deleted by
Observing
> the ProfileInfoCache.

Got it. Thanks for the explanation.

https://codereview.chromium.org/1097553002/diff/80001/chrome/browser/app_cont...
File chrome/browser/app_controller_mac_browsertest.mm (right):

https://codereview.chromium.org/1097553002/diff/80001/chrome/browser/app_cont...
chrome/browser/app_controller_mac_browsertest.mm:459: DCHECK(profile2);
ASSERT_TRUE

https://codereview.chromium.org/1097553002/diff/80001/chrome/browser/app_cont...
chrome/browser/app_controller_mac_browsertest.mm:466: DCHECK_EQ([ac
historyMenuBridge]->service(),
EXPECT_EQ

https://codereview.chromium.org/1097553002/diff/80001/chrome/browser/app_cont...
chrome/browser/app_controller_mac_browsertest.mm:475: DCHECK_EQ([ac
historyMenuBridge]->service(),
EXPECT_EQ

https://codereview.chromium.org/1097553002/diff/80001/chrome/browser/app_cont...
chrome/browser/app_controller_mac_browsertest.mm:478: DCHECK_NE(
EXPECT_NE

https://codereview.chromium.org/1097553002/diff/80001/chrome/browser/app_cont...
chrome/browser/app_controller_mac_browsertest.mm:490: DCHECK_EQ([ac
historyMenuBridge]->service(),
EXPECT_EQ

[Mike Lerman, 2015-04-21 20:17:54]

Patchset #6 (id:100001) has been deleted

[Mike Lerman, 2015-04-21 20:18:48]

Of course, you're right. Thanks for seeing that.
s/DCHECK/EXPECT or ASSERT/g

https://codereview.chromium.org/1097553002/diff/80001/chrome/browser/app_cont...
File chrome/browser/app_controller_mac_browsertest.mm (right):

https://codereview.chromium.org/1097553002/diff/80001/chrome/browser/app_cont...
chrome/browser/app_controller_mac_browsertest.mm:459: DCHECK(profile2);
On 2015/04/21 20:12:30, Robert Sesek wrote:
> ASSERT_TRUE

Done.

https://codereview.chromium.org/1097553002/diff/80001/chrome/browser/app_cont...
chrome/browser/app_controller_mac_browsertest.mm:466: DCHECK_EQ([ac
historyMenuBridge]->service(),
On 2015/04/21 20:12:30, Robert Sesek wrote:
> EXPECT_EQ

Done.

https://codereview.chromium.org/1097553002/diff/80001/chrome/browser/app_cont...
chrome/browser/app_controller_mac_browsertest.mm:475: DCHECK_EQ([ac
historyMenuBridge]->service(),
On 2015/04/21 20:12:30, Robert Sesek wrote:
> EXPECT_EQ

Done.

https://codereview.chromium.org/1097553002/diff/80001/chrome/browser/app_cont...
chrome/browser/app_controller_mac_browsertest.mm:478: DCHECK_NE(
On 2015/04/21 20:12:30, Robert Sesek wrote:
> EXPECT_NE

Done.

https://codereview.chromium.org/1097553002/diff/80001/chrome/browser/app_cont...
chrome/browser/app_controller_mac_browsertest.mm:490: DCHECK_EQ([ac
historyMenuBridge]->service(),
On 2015/04/21 20:12:30, Robert Sesek wrote:
> EXPECT_EQ

Done.

[Robert Sesek, 2015-04-21 20:23:22]

LGTM

[Mike Lerman, 2015-04-21 20:50:45]

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

[commit-bot: I haz the power, 2015-04-21 20:52:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1097553002/120001

[commit-bot: I haz the power, 2015-04-21 21:54:54]

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

[commit-bot: I haz the power, 2015-04-21 21:54:55]

Dry run: This issue passed the CQ dry run.

[Mike Lerman, 2015-04-22 13:07:41]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2015-04-22 13:08:29]

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

[commit-bot: I haz the power, 2015-04-22 13:09:42]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-04-22 13:10:38]

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

[horo, 2015-04-23 01:46:11]

A revert of this CL (patchset #6 id:120001) has been created in
https://codereview.chromium.org/1072193012/ by horo@chromium.org.

The reason for reverting is: browser_tests failure

http://build.chromium.org/p/chromium.mac/builders/Mac10.8%20Tests/builds/1394...


.