Leave apps running on Windows and Linux when quitting Chrome from the wrench menu.

Leave apps running on Windows and Linux when quitting Chrome.

Previously, when Chrome was quit from the wrench menu or the background mode
tray icon, all browser and app windows were closed. Now, all browser windows
are closed and background mode is suspended, but apps are left running. Other
quits, such as restarts, are not affected by this change.

BUG=130457
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=231284

[benwells, 2013-10-20 23:46:21]

What happens if the process is started by an app? Does background mode get
started in that case? Now we have fancy background mode suspension, it should
start up suspended when the process is brought up by an app.

https://codereview.chromium.org/25603004/diff/194001/chrome/browser/backgroun...
File chrome/browser/background/background_mode_manager.cc (right):

https://codereview.chromium.org/25603004/diff/194001/chrome/browser/backgroun...
chrome/browser/background/background_mode_manager.cc:575:
chrome::EndKeepAlive();
Move the keepalive / status tray stuff into functions and reuse from here and
Start / End Background mode. Or factor it out somehow. The simpler / more
tractable the keep alive stuff the better; if it is wrong it introduces
nastiness.

If you could find a nice way of using a scoped variable to manage the keep alive
it would be great, this would prevent adding / releasing keepalives incorrectly.

https://codereview.chromium.org/25603004/diff/194001/chrome/browser/backgroun...
File chrome/browser/background/background_mode_manager.h (right):

https://codereview.chromium.org/25603004/diff/194001/chrome/browser/backgroun...
chrome/browser/background/background_mode_manager.h:70: // until for the current
browser session.
Nit: until for what?

https://codereview.chromium.org/25603004/diff/194001/chrome/browser/backgroun...
chrome/browser/background/background_mode_manager.h:77: virtual void
OnBrowserAdded(Browser* browser) OVERRIDE;
Nit: Make private, and add comment for what interface / class this is overriding
from, like:
  // chrome::BrowserListObserver:

https://codereview.chromium.org/25603004/diff/194001/chrome/browser/lifetime/...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://codereview.chromium.org/25603004/diff/194001/chrome/browser/lifetime/...
chrome/browser/lifetime/application_lifetime.cc:104: // If there are no
browsers, send the APP_TERMINATING action here. Otherwise,
Please update this comment too, it doesn't capture the condition that is being
tested (and I'm having trouble understanding what is going on without looking up
other function definitions).

https://codereview.chromium.org/25603004/diff/194001/chrome/browser/lifetime/...
File chrome/browser/lifetime/application_lifetime.h (right):

https://codereview.chromium.org/25603004/diff/194001/chrome/browser/lifetime/...
chrome/browser/lifetime/application_lifetime.h:62: void CloseAllBrowsers();
Update the comment to mention under what circumstances this will cause the
process to quit.

[Sam McNally, 2013-10-21 04:31:02]

On 2013/10/20 23:46:21, benwells wrote:
> What happens if the process is started by an app? Does background mode get
> started in that case? Now we have fancy background mode suspension, it should
> start up suspended when the process is brought up by an app.

Done.

https://codereview.chromium.org/25603004/diff/194001/chrome/browser/backgroun...
File chrome/browser/background/background_mode_manager.cc (right):

https://codereview.chromium.org/25603004/diff/194001/chrome/browser/backgroun...
chrome/browser/background/background_mode_manager.cc:575:
chrome::EndKeepAlive();
On 2013/10/20 23:46:22, benwells wrote:
> Move the keepalive / status tray stuff into functions and reuse from here and
> Start / End Background mode. Or factor it out somehow. The simpler / more
> tractable the keep alive stuff the better; if it is wrong it introduces
> nastiness.
> 
> If you could find a nice way of using a scoped variable to manage the keep
alive
> it would be great, this would prevent adding / releasing keepalives
incorrectly.

Done.

https://codereview.chromium.org/25603004/diff/194001/chrome/browser/backgroun...
File chrome/browser/background/background_mode_manager.h (right):

https://codereview.chromium.org/25603004/diff/194001/chrome/browser/backgroun...
chrome/browser/background/background_mode_manager.h:70: // until for the current
browser session.
On 2013/10/20 23:46:22, benwells wrote:
> Nit: until for what?

Done.

https://codereview.chromium.org/25603004/diff/194001/chrome/browser/backgroun...
chrome/browser/background/background_mode_manager.h:77: virtual void
OnBrowserAdded(Browser* browser) OVERRIDE;
On 2013/10/20 23:46:22, benwells wrote:
> Nit: Make private, and add comment for what interface / class this is
overriding
> from, like:
>   // chrome::BrowserListObserver:

Done.

https://codereview.chromium.org/25603004/diff/194001/chrome/browser/lifetime/...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://codereview.chromium.org/25603004/diff/194001/chrome/browser/lifetime/...
chrome/browser/lifetime/application_lifetime.cc:104: // If there are no
browsers, send the APP_TERMINATING action here. Otherwise,
On 2013/10/20 23:46:22, benwells wrote:
> Please update this comment too, it doesn't capture the condition that is being
> tested (and I'm having trouble understanding what is going on without looking
up
> other function definitions).

Done.

https://codereview.chromium.org/25603004/diff/194001/chrome/browser/lifetime/...
File chrome/browser/lifetime/application_lifetime.h (right):

https://codereview.chromium.org/25603004/diff/194001/chrome/browser/lifetime/...
chrome/browser/lifetime/application_lifetime.h:62: void CloseAllBrowsers();
On 2013/10/20 23:46:22, benwells wrote:
> Update the comment to mention under what circumstances this will cause the
> process to quit.

Done.

[benwells, 2013-10-21 05:57:22]

just questions now...

https://codereview.chromium.org/25603004/diff/462001/chrome/browser/app_contr...
File chrome/browser/app_controller_mac.mm (left):

https://codereview.chromium.org/25603004/diff/462001/chrome/browser/app_contr...
chrome/browser/app_controller_mac.mm:364: if
(!browser_shutdown::IsTryingToQuit()) {
Why was this check here? Or put another way, why have you removed it?

https://codereview.chromium.org/25603004/diff/462001/chrome/browser/backgroun...
File chrome/browser/background/background_mode_manager.cc (right):

https://codereview.chromium.org/25603004/diff/462001/chrome/browser/backgroun...
chrome/browser/background/background_mode_manager.cc:192:
SuspendBackgroundMode();
Relying on these two flags feels fragile. E.g. --load-and-launch-app is another
flag you should probably check (probably as right now it won't matter, but once
all this lifetime stuff is sorted out and we fix it, it will).

Can you just suspend if !kNoStartupWindow and then let the browser window
opening unsuspend?

https://codereview.chromium.org/25603004/diff/462001/chrome/browser/backgroun...
chrome/browser/background/background_mode_manager.cc:573:
CreateStatusTrayIcon();
Nit: early return instead of else.

https://codereview.chromium.org/25603004/diff/462001/chrome/browser/backgroun...
chrome/browser/background/background_mode_manager.cc:681:
UpdateKeepAliveAndTrayIcon();
This only happened before if !status_icon, and the comment talks about this as
well. Either keep the logic the same, or if it is worthy of changing (maybe it
doesn't matter), update the comment.

https://codereview.chromium.org/25603004/diff/462001/chrome/browser/lifetime/...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://codereview.chromium.org/25603004/diff/462001/chrome/browser/lifetime/...
chrome/browser/lifetime/application_lifetime.cc:109:
(browser_shutdown::IsTryingToQuit() || !chrome::WillKeepAlive()))) {
Why did the SetTryingToQuit(true) line get removed here?

https://codereview.chromium.org/25603004/diff/462001/chrome/browser/lifetime/...
File chrome/browser/lifetime/browser_close_manager_browsertest.cc (right):

https://codereview.chromium.org/25603004/diff/462001/chrome/browser/lifetime/...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:776:
EXPECT_TRUE(static_cast<FakeBackgroundModeManager*>(
Nit: factor out this horribleness into IsBackgroundModeSuspended() in
BrowserCloseManagerWithBackgroundModeBrowserTest.

[Sam McNally, 2013-10-21 06:45:35]

https://codereview.chromium.org/25603004/diff/462001/chrome/browser/app_contr...
File chrome/browser/app_controller_mac.mm (left):

https://codereview.chromium.org/25603004/diff/462001/chrome/browser/app_contr...
chrome/browser/app_controller_mac.mm:364: if
(!browser_shutdown::IsTryingToQuit()) {
On 2013/10/21 05:57:22, benwells wrote:
> Why was this check here? Or put another way, why have you removed it?

Previously, CloseAllBrowsers called SetTryingToQuit. Now, the callers need to
call SetTryingToQuit so giving up if we're already trying to quit would
quitting.

https://codereview.chromium.org/25603004/diff/462001/chrome/browser/backgroun...
File chrome/browser/background/background_mode_manager.cc (right):

https://codereview.chromium.org/25603004/diff/462001/chrome/browser/backgroun...
chrome/browser/background/background_mode_manager.cc:192:
SuspendBackgroundMode();
On 2013/10/21 05:57:22, benwells wrote:
> Relying on these two flags feels fragile. E.g. --load-and-launch-app is
another
> flag you should probably check (probably as right now it won't matter, but
once
> all this lifetime stuff is sorted out and we fix it, it will).
> 
> Can you just suspend if !kNoStartupWindow and then let the browser window
> opening unsuspend?

Done.

https://codereview.chromium.org/25603004/diff/462001/chrome/browser/backgroun...
chrome/browser/background/background_mode_manager.cc:573:
CreateStatusTrayIcon();
On 2013/10/21 05:57:22, benwells wrote:
> Nit: early return instead of else.

Done.

https://codereview.chromium.org/25603004/diff/462001/chrome/browser/backgroun...
chrome/browser/background/background_mode_manager.cc:681:
UpdateKeepAliveAndTrayIcon();
On 2013/10/21 05:57:22, benwells wrote:
> This only happened before if !status_icon, and the comment talks about this as
> well. Either keep the logic the same, or if it is worthy of changing (maybe it
> doesn't matter), update the comment.

Done.

https://codereview.chromium.org/25603004/diff/462001/chrome/browser/lifetime/...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://codereview.chromium.org/25603004/diff/462001/chrome/browser/lifetime/...
chrome/browser/lifetime/application_lifetime.cc:109:
(browser_shutdown::IsTryingToQuit() || !chrome::WillKeepAlive()))) {
On 2013/10/21 05:57:22, benwells wrote:
> Why did the SetTryingToQuit(true) line get removed here?

Done.

https://codereview.chromium.org/25603004/diff/462001/chrome/browser/lifetime/...
File chrome/browser/lifetime/browser_close_manager_browsertest.cc (right):

https://codereview.chromium.org/25603004/diff/462001/chrome/browser/lifetime/...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:776:
EXPECT_TRUE(static_cast<FakeBackgroundModeManager*>(
On 2013/10/21 05:57:22, benwells wrote:
> Nit: factor out this horribleness into IsBackgroundModeSuspended() in
> BrowserCloseManagerWithBackgroundModeBrowserTest.

Done.

[benwells, 2013-10-21 21:12:51]

https://codereview.chromium.org/25603004/diff/462001/chrome/browser/app_contr...
File chrome/browser/app_controller_mac.mm (left):

https://codereview.chromium.org/25603004/diff/462001/chrome/browser/app_contr...
chrome/browser/app_controller_mac.mm:364: if
(!browser_shutdown::IsTryingToQuit()) {
On 2013/10/21 06:45:35, Sam McNally wrote:
> On 2013/10/21 05:57:22, benwells wrote:
> > Why was this check here? Or put another way, why have you removed it?
> 
> Previously, CloseAllBrowsers called SetTryingToQuit. Now, the callers need to
> call SetTryingToQuit so giving up if we're already trying to quit would
> quitting.

The check seems odd. I'm worried there an obscure case where this code path
could be hit while quitting, and needed to be prevented. Could you look up when
the if clause was added and see if it was in response to a bug?

https://codereview.chromium.org/25603004/diff/462001/chrome/browser/lifetime/...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://codereview.chromium.org/25603004/diff/462001/chrome/browser/lifetime/...
chrome/browser/lifetime/application_lifetime.cc:109:
(browser_shutdown::IsTryingToQuit() || !chrome::WillKeepAlive()))) {
On 2013/10/21 06:45:35, Sam McNally wrote:
> On 2013/10/21 05:57:22, benwells wrote:
> > Why did the SetTryingToQuit(true) line get removed here?
> 
> Done.

Sorry, I'm probably being dense here but why don't you do it  for batch shutdown
as well? My understanding is this code path is hit if the user asks to exit,
there are no browser windows, and there is nothing else that will keep the
process alive.

In that case shouldn't TryingToQuit be set true?

https://codereview.chromium.org/25603004/diff/462001/chrome/browser/lifetime/...
File chrome/browser/lifetime/browser_close_manager_browsertest.cc (right):

https://codereview.chromium.org/25603004/diff/462001/chrome/browser/lifetime/...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:776:
EXPECT_TRUE(static_cast<FakeBackgroundModeManager*>(
On 2013/10/21 06:45:35, Sam McNally wrote:
> On 2013/10/21 05:57:22, benwells wrote:
> > Nit: factor out this horribleness into IsBackgroundModeSuspended() in
> > BrowserCloseManagerWithBackgroundModeBrowserTest.
> 
> Done.

You missed this one.

[Sam McNally, 2013-10-22 00:16:27]

https://codereview.chromium.org/25603004/diff/462001/chrome/browser/app_contr...
File chrome/browser/app_controller_mac.mm (left):

https://codereview.chromium.org/25603004/diff/462001/chrome/browser/app_contr...
chrome/browser/app_controller_mac.mm:364: if
(!browser_shutdown::IsTryingToQuit()) {
On 2013/10/21 21:12:51, benwells wrote:
> On 2013/10/21 06:45:35, Sam McNally wrote:
> > On 2013/10/21 05:57:22, benwells wrote:
> > > Why was this check here? Or put another way, why have you removed it?
> > 
> > Previously, CloseAllBrowsers called SetTryingToQuit. Now, the callers need
to
> > call SetTryingToQuit so giving up if we're already trying to quit would
> > quitting.
> 
> The check seems odd. I'm worried there an obscure case where this code path
> could be hit while quitting, and needed to be prevented. Could you look up
when
> the if clause was added and see if it was in response to a bug?

Done.

https://codereview.chromium.org/25603004/diff/462001/chrome/browser/lifetime/...
File chrome/browser/lifetime/browser_close_manager_browsertest.cc (right):

https://codereview.chromium.org/25603004/diff/462001/chrome/browser/lifetime/...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:776:
EXPECT_TRUE(static_cast<FakeBackgroundModeManager*>(
On 2013/10/21 21:12:51, benwells wrote:
> On 2013/10/21 06:45:35, Sam McNally wrote:
> > On 2013/10/21 05:57:22, benwells wrote:
> > > Nit: factor out this horribleness into IsBackgroundModeSuspended() in
> > > BrowserCloseManagerWithBackgroundModeBrowserTest.
> > 
> > Done.
> 
> You missed this one.

Done.

[benwells, 2013-10-22 00:22:08]

lgtm

[Sam McNally, 2013-10-22 00:28:45]

+atwilson for chrome/browser/background
+sky for the rest

[sky, 2013-10-22 15:41:18]

https://codereview.chromium.org/25603004/diff/1022001/chrome/browser/lifetime...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://codereview.chromium.org/25603004/diff/1022001/chrome/browser/lifetime...
chrome/browser/lifetime/application_lifetime.cc:96: #if !defined(OS_MACOSX)
Is there a reason you set trying to quit after sending the notification? Seems
like it would make more sense to set it first so that anyone listening to the
notification could see we want to quit.

https://codereview.chromium.org/25603004/diff/1022001/chrome/browser/lifetime...
chrome/browser/lifetime/application_lifetime.cc:97: if (try_to_quit_application)
{
nit: no {}

https://codereview.chromium.org/25603004/diff/1022001/chrome/browser/lifetime...
File chrome/browser/lifetime/browser_close_manager_browsertest.cc (right):

https://codereview.chromium.org/25603004/diff/1022001/chrome/browser/lifetime...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:765: };
private:DISALLOW...

https://codereview.chromium.org/25603004/diff/1022001/chrome/browser/lifetime...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:767:
IN_PROC_BROWSER_TEST_P(BrowserCloseManagerWithBackgroundModeBrowserTest,
Please add descriptions as to what tests do. That is, what they are intended to
verify is working.

[Sam McNally, 2013-10-22 22:58:29]

https://codereview.chromium.org/25603004/diff/1022001/chrome/browser/lifetime...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://codereview.chromium.org/25603004/diff/1022001/chrome/browser/lifetime...
chrome/browser/lifetime/application_lifetime.cc:96: #if !defined(OS_MACOSX)
On 2013/10/22 15:41:18, sky wrote:
> Is there a reason you set trying to quit after sending the notification? Seems
> like it would make more sense to set it first so that anyone listening to the
> notification could see we want to quit.

Done.

https://codereview.chromium.org/25603004/diff/1022001/chrome/browser/lifetime...
chrome/browser/lifetime/application_lifetime.cc:97: if (try_to_quit_application)
{
On 2013/10/22 15:41:18, sky wrote:
> nit: no {}

Done.

https://codereview.chromium.org/25603004/diff/1022001/chrome/browser/lifetime...
File chrome/browser/lifetime/browser_close_manager_browsertest.cc (right):

https://codereview.chromium.org/25603004/diff/1022001/chrome/browser/lifetime...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:765: };
On 2013/10/22 15:41:18, sky wrote:
> private:DISALLOW...

Done.

https://codereview.chromium.org/25603004/diff/1022001/chrome/browser/lifetime...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:767:
IN_PROC_BROWSER_TEST_P(BrowserCloseManagerWithBackgroundModeBrowserTest,
On 2013/10/22 15:41:18, sky wrote:
> Please add descriptions as to what tests do. That is, what they are intended
to
> verify is working.

Done.

[sky, 2013-10-22 23:39:17]

LGTM

[Andrew T Wilson (Slow), 2013-10-23 10:00:21]

https://codereview.chromium.org/25603004/diff/1142001/chrome/browser/backgrou...
File chrome/browser/background/background_mode_manager.cc (right):

https://codereview.chromium.org/25603004/diff/1142001/chrome/browser/backgrou...
chrome/browser/background/background_mode_manager.cc:620: // then we won't
notify the user.
This means that platforms without status trays will never get an app installed
notification (like Mac). Now, we don't currently display an app installed
notification on Mac, but that's a bug, not a feature.

Why are we skipping the user notification if there's no status icon? What's the
case this addresses? It's bad to let a background app get installed and turn on
background mode, but not notify the user.

https://codereview.chromium.org/25603004/diff/1142001/chrome/browser/backgrou...
File chrome/browser/background/background_mode_manager_unittest.cc (right):

https://codereview.chromium.org/25603004/diff/1142001/chrome/browser/backgrou...
chrome/browser/background/background_mode_manager_unittest.cc:123:
manager.SetBackgroundAppCount(1);
How would an app get installed in this profile while we're suspended? Is sync
still running or something?

[Sam McNally, 2013-10-24 02:01:37]

https://codereview.chromium.org/25603004/diff/1142001/chrome/browser/backgrou...
File chrome/browser/background/background_mode_manager.cc (right):

https://codereview.chromium.org/25603004/diff/1142001/chrome/browser/backgrou...
chrome/browser/background/background_mode_manager.cc:620: // then we won't
notify the user.
On 2013/10/23 10:00:21, Andrew T Wilson wrote:
> This means that platforms without status trays will never get an app installed
> notification (like Mac). Now, we don't currently display an app installed
> notification on Mac, but that's a bug, not a feature.
> 
> Why are we skipping the user notification if there's no status icon? What's
the
> case this addresses? It's bad to let a background app get installed and turn
on
> background mode, but not notify the user.

Done.

https://codereview.chromium.org/25603004/diff/1142001/chrome/browser/backgrou...
File chrome/browser/background/background_mode_manager_unittest.cc (right):

https://codereview.chromium.org/25603004/diff/1142001/chrome/browser/backgrou...
chrome/browser/background/background_mode_manager_unittest.cc:123:
manager.SetBackgroundAppCount(1);
On 2013/10/23 10:00:21, Andrew T Wilson wrote:
> How would an app get installed in this profile while we're suspended? Is sync
> still running or something?

Yes, sync is still running. I suspect newly added force-installed
extensions/apps could also trigger this.

[Andrew T Wilson (Slow), 2013-10-25 11:32:44]

LGTM if you remove the if statement I flagged below. If you feel you can't
remove that if statement, then we should chat more.

https://codereview.chromium.org/25603004/diff/1252001/chrome/browser/backgrou...
File chrome/browser/background/background_mode_manager.cc (right):

https://codereview.chromium.org/25603004/diff/1252001/chrome/browser/backgrou...
chrome/browser/background/background_mode_manager.cc:622: return;
I'm still not sure about this change. I think we ought to try to display the
notification no matter what - it seems really bad to allow a background app to
get installed and not display it.

What specific use case is this if statement addressing?

[Sam McNally, 2013-10-28 00:24:54]

https://codereview.chromium.org/25603004/diff/1252001/chrome/browser/backgrou...
File chrome/browser/background/background_mode_manager.cc (right):

https://codereview.chromium.org/25603004/diff/1252001/chrome/browser/backgrou...
chrome/browser/background/background_mode_manager.cc:622: return;
On 2013/10/25 11:32:44, Andrew T Wilson wrote:
> I'm still not sure about this change. I think we ought to try to display the
> notification no matter what - it seems really bad to allow a background app to
> get installed and not display it.
> 
> What specific use case is this if statement addressing? 

I thought it was from before, but it wasn't. Removed it.

[commit-bot: I haz the power, 2013-10-28 00:28:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sammc@chromium.org/25603004/1452001

[commit-bot: I haz the power, 2013-10-28 01:04:55]

Retried try job too often on ios_dbg_simulator for step(s) content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-10-28 01:31:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sammc@chromium.org/25603004/1452001

[commit-bot: I haz the power, 2013-10-28 06:27:22]

Change committed as 231284