Refactor shutdown code to allow win/linux to run after last browser closes.

Moved code that generates APP_TERMINATING notification into BrowserList so it can be used on all platforms. Updated observers (ExtensionProcessManager,
BackgroundContents) to listen for APP_TERMINATING instead of listening for
BROWSER_CLOSED on some platforms.

APP_TERMINATING is now sent just before the main message loop exits rather than  just after, but no code depends on this timing.

Updated Mac code to always call BrowserList::CloseAllBrowsers() even if there
are no open browsers, to ensure that APP_TERMINATING is always fired.

Changed BackgroundContentsService to keep the browser process alive when there
are BackgroundContents running and updated the unit tests.

Renamed BrowserList::IsInPersistentMode() => WillShutdownWhenLastBrowserCloses() and AllBrowsersClosed() => AllBrowsersClosedAndAppExiting() to more precisely indicate their true functions.

Exposed BrowserProcess::ModuleRefCount() so BrowserList can determine when the application is going to exit so the right notifications/callbacks can be generated.

Updated background-auto-update-restart code to use new BrowserList APIs to determine whether the application is running "in the background".

Added code to cancel shutdown on all plaforms if the user selects cancel in an onbeforeunload dialog. 
BUG=45275
TEST=RunInBackgroundTest (ui_test)

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

[tfarina, 2010-06-30 02:38:13]

http://codereview.chromium.org/2866034/diff/1/3
File chrome/browser/browser_list.cc (right):

http://codereview.chromium.org/2866034/diff/1/3#newcode234
chrome/browser/browser_list.cc:234: if (!BrowserList::size()) {
Instead of this, could you add a:

static bool empty() { return browsers_.empty(); }

and use it here? Or even, just use browsers_.empty()?

[Andrew T Wilson (Slow), 2010-07-13 15:38:59]

This is ready for review, so please take a look - if there's someone else who
would be a better fit for reviewing shutdown changes, feel free to add them as a
reviewer instead.

[sky, 2010-07-13 16:10:13]

http://codereview.chromium.org/2866034/diff/15001/16008
File chrome/browser/browser_list.cc (right):

http://codereview.chromium.org/2866034/diff/15001/16008#newcode339
chrome/browser/browser_list.cc:339: return g_browser_process->ModuleRefCount()
== ref_count;
I don't think it's right to use the ref count as a way to know if the app is
going to shutdown when the last browser is closed. If this evaluates to false,
the app may still end up shutting down almost immediately after the last browser
closes. For example, if a menu is shown the ref count is incremented, but as
soon the menu is hidden the ref count is decremented
(chrome/browser/views/chrome_views_delegate). If the browser is closed before
the menu (which can happen) this would incorrectly return false.

[Paweł Hajdan Jr., 2010-07-13 16:24:06]

Drive-by with some automation comments.

http://codereview.chromium.org/2866034/diff/15001/16002
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/2866034/diff/15001/16002#newcode171
chrome/browser/automation/automation_provider.cc:171: DLOG(WARNING) << "SHUTTING
DOWN AUTOMATION PROVIDER";
Is this log message still needed and useful after developing this patch?

http://codereview.chromium.org/2866034/diff/15001/16019
File chrome/browser/browser_uitest.cc (right):

http://codereview.chromium.org/2866034/diff/15001/16019#newcode344
chrome/browser/browser_uitest.cc:344: scoped_refptr<BrowserProxy>
browser(automation()->GetBrowserWindow(0));
Please ASSERT_TRUE that browser is non-NULL.

[Andrew T Wilson (Slow), 2010-07-14 01:39:49]

http://codereview.chromium.org/2866034/diff/15001/16008
File chrome/browser/browser_list.cc (right):

http://codereview.chromium.org/2866034/diff/15001/16008#newcode339
chrome/browser/browser_list.cc:339: return g_browser_process->ModuleRefCount()
== ref_count;
On 2010/07/13 16:10:14, sky wrote:
> I don't think it's right to use the ref count as a way to know if the app is
> going to shutdown when the last browser is closed. If this evaluates to false,
> the app may still end up shutting down almost immediately after the last
browser
> closes. For example, if a menu is shown the ref count is incremented, but as
> soon the menu is hidden the ref count is decremented
Good catch. It's bad, because this means that if you close a browser window
while a menu is shown, the APP_TERMINATING notification won't be sent out before
shutdown.

The other way to do this is to introduce a separate "run-in-background" count
rather than relying on the browser ref count. I avoided doing this originally
because I thought that would be too fragile (having two counts in parallel) but
I think it's probably the cleanest solution. I'll make this change tomorrow.

[Andrew T Wilson (Slow), 2010-07-17 17:02:16]

Please take another look. There is a trybot failure on linux, because this CL is
dependent on a fix to the linux shutdown code
(http://codereview.chromium.org/3040006/show) which hasn't landed yet.

http://codereview.chromium.org/2866034/diff/15001/16002
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/2866034/diff/15001/16002#newcode171
chrome/browser/automation/automation_provider.cc:171: DLOG(WARNING) << "SHUTTING
DOWN AUTOMATION PROVIDER";
On 2010/07/13 16:24:08, Paweł Hajdan Jr. wrote:
> Is this log message still needed and useful after developing this patch?
Removed, thanks for catching this.

http://codereview.chromium.org/2866034/diff/15001/16019
File chrome/browser/browser_uitest.cc (right):

http://codereview.chromium.org/2866034/diff/15001/16019#newcode344
chrome/browser/browser_uitest.cc:344: scoped_refptr<BrowserProxy>
browser(automation()->GetBrowserWindow(0));
On 2010/07/13 16:24:08, Paweł Hajdan Jr. wrote:
> Please ASSERT_TRUE that browser is non-NULL.

Done.

[sky, 2010-07-19 16:10:08]

Mostly just nits. But two additional comments/questions:

. Did you make sure everything works ok if shutdown is the result of the user
logging out?
. Does session restore need to be updated?

  -Scott

http://codereview.chromium.org/2866034/diff/25001/26003
File chrome/browser/background_contents_service.cc (right):

http://codereview.chromium.org/2866034/diff/25001/26003#newcode43
chrome/browser/background_contents_service.cc:43: ,
always_keep_alive_(command_line->HasSwitch(switches::kKeepAliveForTest)) {
, should be on previous line, eg:
: prefs_(NULL),
  always_keep_alive_(...

http://codereview.chromium.org/2866034/diff/25001/26008
File chrome/browser/browser_list.cc (right):

http://codereview.chromium.org/2866034/diff/25001/26008#newcode335
chrome/browser/browser_list.cc:335: unsigned int BrowserList::keep_alive_count_
= 0;
Put this with the other declarations.

http://codereview.chromium.org/2866034/diff/25001/26008#newcode355
chrome/browser/browser_list.cc:355: // (MessageLoop::current() == null)
nit: add . to end of sentence.

http://codereview.chromium.org/2866034/diff/25001/26009
File chrome/browser/browser_list.h (right):

http://codereview.chromium.org/2866034/diff/25001/26009#newcode171
chrome/browser/browser_list.h:171: static unsigned int keep_alive_count_;
Style guide says not to use unsigned like this. Use int.

http://codereview.chromium.org/2866034/diff/25001/26019
File chrome/browser/js_modal_dialog.cc (right):

http://codereview.chromium.org/2866034/diff/25001/26019#newcode10
chrome/browser/js_modal_dialog.cc:10: #include
"chrome/browser/browser_shutdown.h"
This should be before line 8.

http://codereview.chromium.org/2866034/diff/25001/26024
File chrome/common/notification_type.h (right):

http://codereview.chromium.org/2866034/diff/25001/26024#newcode201
chrome/common/notification_type.h:201: // that if true indicates that the last
browser window has closed - this
Should the boolean be nuked?

[Paweł Hajdan Jr., 2010-07-19 19:14:51]

Code I commented in the drive-by LGTM.

[Andrew T Wilson (Slow), 2010-07-19 20:59:51]

OK, addressed your comments (see below for some clarifications). Please take
another look.

Session restore acts like it previously did in the case that the browser process
is not in keep-alive mode. When running in keep-alive mode, if the user closes
the last browser window, then later tries to open a browser window (for
instance, by re-running chrome.exe) session restore is triggered and the
previously opened tabs are reopened. I believe this is the desired behavior.

Logout causes chrome to exit when in keep-alive and non-keep-alive modes (all of
the shutdown paths run through BrowserList::CloseAllBrowsers() now, which is
what WindowsSessionEnding() uses).

http://codereview.chromium.org/2866034/diff/25001/26003
File chrome/browser/background_contents_service.cc (right):

http://codereview.chromium.org/2866034/diff/25001/26003#newcode43
chrome/browser/background_contents_service.cc:43: ,
always_keep_alive_(command_line->HasSwitch(switches::kKeepAliveForTest)) {
On 2010/07/19 16:10:08, sky wrote:
> , should be on previous line, eg:
> : prefs_(NULL),
>   always_keep_alive_(...
> 

Done.

http://codereview.chromium.org/2866034/diff/25001/26008
File chrome/browser/browser_list.cc (right):

http://codereview.chromium.org/2866034/diff/25001/26008#newcode335
chrome/browser/browser_list.cc:335: unsigned int BrowserList::keep_alive_count_
= 0;
On 2010/07/19 16:10:08, sky wrote:
> Put this with the other declarations.

There doesn't seem to be a single place to declare things. For example,
activity_observer is defined on line 80, browsers_ and observers_ are defined at
line 141, last_active_browsers_ is at line 368. So the pattern in the file seems
to be "declare static next to the code that uses it".

If you want, I can move all the declarations in the file into one place, but
otherwise, I think this matches the existing style in the file.

http://codereview.chromium.org/2866034/diff/25001/26008#newcode355
chrome/browser/browser_list.cc:355: // (MessageLoop::current() == null)
On 2010/07/19 16:10:08, sky wrote:
> nit: add . to end of sentence.

Done.

http://codereview.chromium.org/2866034/diff/25001/26009
File chrome/browser/browser_list.h (right):

http://codereview.chromium.org/2866034/diff/25001/26009#newcode171
chrome/browser/browser_list.h:171: static unsigned int keep_alive_count_;
On 2010/07/19 16:10:08, sky wrote:
> Style guide says not to use unsigned like this. Use int.

Done.

http://codereview.chromium.org/2866034/diff/25001/26019
File chrome/browser/js_modal_dialog.cc (right):

http://codereview.chromium.org/2866034/diff/25001/26019#newcode10
chrome/browser/js_modal_dialog.cc:10: #include
"chrome/browser/browser_shutdown.h"
On 2010/07/19 16:10:08, sky wrote:
> This should be before line 8.

Done.

http://codereview.chromium.org/2866034/diff/25001/26024
File chrome/common/notification_type.h (right):

http://codereview.chromium.org/2866034/diff/25001/26024#newcode201
chrome/common/notification_type.h:201: // that if true indicates that the last
browser window has closed - this
On 2010/07/19 16:10:08, sky wrote:
> Should the boolean be nuked?
Per our email conversation from May 27, you wrote:
"I think both are still interesting. Lets assume closing the last
window doesn't exit chrome. On the windows/linux side I suspect folks
would expect some action that triggers launching a new browser to
restore pinned tabs/trigger session restore. In this case the boolean
in BROWSER_CLOSING should still be true, even though we're not
exiting.
...
See earlier comment, I think BROWSER_CLOSING still needs the boolean."

I left it in based on that - looking at the source, it's still used by the
automation code to provide consistent cross-platform behavior (shutdown when
last browser closes on all platforms) and chromeos notification code. I could
probably change the chromeos code to use APP_TERMINATING, but I still think we
want the boolean for the automation code. So I think we should leave it in.

[sky, 2010-07-19 21:17:44]

LGTM