Remove details from BROWSER_CLOSING and BROWSER_CLOSED notifications.

Remove details from BROWSER_CLOSING and BROWSER_CLOSED notifications.

The details was a boolean indicating whether this is the last browser window closing. As part of this change the logic for saving pinned tabs has been changed to not use the boolean in the details, and has been updated to handle the case where a user exit or closing of the last window is cancelled by an onbeforeunload event.

Also as part of this change, both the pinned tab and multi profile features have been updated to handle cases of the browser process not being closed due to background mode or (in the future) packaged apps, and then more browser windows being opened.

BUG=None
TEST=Test shutdown 


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148281

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148741

[benwells, 2012-07-19 11:16:28]

There are some problems with PyAuto I haven't looked at yet, but assuming they
are not too major to fix this is ready for a look by reviewers.

http://codereview.chromium.org/10800031/diff/8001/chrome/browser/ui/tabs/pinn...
File chrome/browser/ui/tabs/pinned_tab_service.cc (right):

http://codereview.chromium.org/10800031/diff/8001/chrome/browser/ui/tabs/pinn...
chrome/browser/ui/tabs/pinned_tab_service.cc:51: // continues their session, or
if the process is kept alive by the background
Is this logic too tricky? I'm not sure if it's the right way to handle
onbeforeunload cancelling a shutdown.

[sky, 2012-07-19 16:31:27]

http://codereview.chromium.org/10800031/diff/8001/chrome/browser/ui/tabs/pinn...
File chrome/browser/ui/tabs/pinned_tab_service.cc (right):

http://codereview.chromium.org/10800031/diff/8001/chrome/browser/ui/tabs/pinn...
chrome/browser/ui/tabs/pinned_tab_service.cc:51: // continues their session, or
if the process is kept alive by the background
On 2012/07/19 11:16:28, benwells wrote:
> Is this logic too tricky? I'm not sure if it's the right way to handle
> onbeforeunload cancelling a shutdown.

Can you instead listen for NOTIFICATION_BROWSER_CLOSE_CANCELLED ?

[sail, 2012-07-19 16:41:29]

profile/* LGTM!

[benwells, 2012-07-20 01:41:56]

I can't reproduce the problems with pyauto tests locally, I think maybe it's
noise or sick bots. I'm running the tests again.

http://codereview.chromium.org/10800031/diff/8001/chrome/browser/ui/tabs/pinn...
File chrome/browser/ui/tabs/pinned_tab_service.cc (right):

http://codereview.chromium.org/10800031/diff/8001/chrome/browser/ui/tabs/pinn...
chrome/browser/ui/tabs/pinned_tab_service.cc:51: // continues their session, or
if the process is kept alive by the background
On 2012/07/19 16:31:27, sky wrote:
> On 2012/07/19 11:16:28, benwells wrote:
> > Is this logic too tricky? I'm not sure if it's the right way to handle
> > onbeforeunload cancelling a shutdown.
> 
> Can you instead listen for NOTIFICATION_BROWSER_CLOSE_CANCELLED ?

I could, I'm not sure if it's right or not. I've updated the logic a bit and
expanded the comment. Let me know if this makes sense the way it is.

[sky, 2012-07-20 16:06:40]

http://codereview.chromium.org/10800031/diff/5003/chrome/browser/ui/tabs/pinn...
File chrome/browser/ui/tabs/pinned_tab_service.cc (right):

http://codereview.chromium.org/10800031/diff/5003/chrome/browser/ui/tabs/pinn...
chrome/browser/ui/tabs/pinned_tab_service.cc:61: //     over-writing the correct
state.
The problem with this, and why I suggested NOTIFICATION_BROWSER_CLOSE_CANCELLED,
is that you're assuming the user exits immediately. We don't necessarily know
that is going to happen. A new browser window is a good indication, but I tend
to think a new tab created is better.

[benwells, 2012-07-23 01:22:12]

https://chromiumcodereview.appspot.com/10800031/diff/5003/chrome/browser/ui/t...
File chrome/browser/ui/tabs/pinned_tab_service.cc (right):

https://chromiumcodereview.appspot.com/10800031/diff/5003/chrome/browser/ui/t...
chrome/browser/ui/tabs/pinned_tab_service.cc:61: //     over-writing the correct
state.
On 2012/07/20 16:06:40, sky wrote:
> The problem with this, and why I suggested
NOTIFICATION_BROWSER_CLOSE_CANCELLED,
> is that you're assuming the user exits immediately. We don't necessarily know
> that is going to happen. A new browser window is a good indication, but I tend
> to think a new tab created is better.

Yep, they won't always exit immediately. I've put in your suggestion and I
reckon it's better, especially in the case where there is only one window open.

[sky, 2012-07-23 15:56:27]

LGTM

[commit-bot: I haz the power, 2012-07-23 23:58:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/10800031/7003

[commit-bot: I haz the power, 2012-07-23 23:58:46]

Presubmit check for 10800031-7003 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/automation

Presubmit checks took 1.1s to calculate.

[benwells, 2012-07-24 00:23:11]

+ben for automation owners review

background: i want to simplify shutdown so my poor, limited consciousness can
understand it.

[Ben Goodger (Google), 2012-07-24 15:58:33]

lgtm

[commit-bot: I haz the power, 2012-07-25 00:39:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/10800031/7003

[commit-bot: I haz the power, 2012-07-25 02:32:23]

Change committed as 148281

[benwells, 2012-07-27 00:04:09]

This change originally broke the performance ui tests so I reverted it. It's now
fixed.

The problem was that the change to the automation ipc messages seemed to change
all the autmation ipc ids, breaking comms with the reference build. While
investigating I found one or two places where the closing app field is used by
automation, so I have put it back, but in a way that it is generated by the
automation system.

ben - ptal

[Ben Goodger (Google), 2012-07-27 00:05:01]

lgtm

[commit-bot: I haz the power, 2012-07-27 01:56:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/10800031/23001

[commit-bot: I haz the power, 2012-07-27 04:09:47]

Try job failure for 10800031-23001 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-07-27 05:51:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/10800031/23001

[commit-bot: I haz the power, 2012-07-27 10:48:20]

Change committed as 148741