Add BackgroundModeOptimizer that can restart the browser

Add BackgroundModeOptimizer that can restart the browser

Its initialization is behind the BackgroundModeAllowRestart
flag, and is supported only on Windows and Linux

BUG=585080
Committed: https://crrev.com/3eaf67ed9424c847489410ec5956ddd1bf9a1557
Cr-Commit-Position: refs/heads/master@{#407742}

[dgn, 2016-04-28 10:12:38]

Description was changed from

==========
t cl try
Add BackgroundModeOptimizer that can restart the browser

Its initialization is behind the BackgroundModeAllowRestart
flag, and is supported only on Windows and Linux

BUG=585080
==========

to

==========
Add BackgroundModeOptimizer that can restart the browser

Its initialization is behind the BackgroundModeAllowRestart
flag, and is supported only on Windows and Linux

BUG=585080
==========

[dgn, 2016-05-18 13:57:08]

Patchset #1 (id:1) has been deleted

[dgn, 2016-05-18 13:57:17]

Patchset #1 (id:20001) has been deleted

[dgn, 2016-05-18 13:57:25]

Patchset #1 (id:40001) has been deleted

[dgn, 2016-05-18 15:17:25]

dgn@chromium.org changed reviewers:
+ bauerb@chromium.org, sky@chromium.org

[dgn, 2016-05-18 15:17:25]

PTAL. PatchSet1 has the tests passing with the flag enabled

[sky, 2016-05-18 15:29:52]

I'm not familiar with this work. Is there a design doc you can point me at?

[dgn, 2016-05-18 15:30:52]

On 2016/05/18 15:29:52, sky wrote:
> I'm not familiar with this work. Is there a design doc you can point me at?

Here it is:
https://docs.google.com/document/d/1AKl7lh4v0At6bx10mu2rhWH8I54e2yVH20a6vO-as...

[sky, 2016-05-18 19:16:28]

On 2016/05/18 15:30:52, dgn wrote:
> On 2016/05/18 15:29:52, sky wrote:
> > I'm not familiar with this work. Is there a design doc you can point me at?
> 
> Here it is:
>
https://docs.google.com/document/d/1AKl7lh4v0At6bx10mu2rhWH8I54e2yVH20a6vO-as...

Doing a restart could have subtle ramifications. I think you should send this
proposal to eng-review.

[sky, 2016-05-18 19:23:06]

Can you write a browser test? It would be good to verify that chrome restarts
with the appropriate flag as appropriate.

https://codereview.chromium.org/1931503002/diff/80001/chrome/browser/backgrou...
File chrome/browser/background/background_mode_manager.h (right):

https://codereview.chromium.org/1931503002/diff/80001/chrome/browser/backgrou...
chrome/browser/background/background_mode_manager.h:94: void
EndBackgroundMode();
Move implementation to match new position.

https://codereview.chromium.org/1931503002/diff/80001/chrome/browser/backgrou...
File chrome/browser/background/background_mode_optimizer.cc (right):

https://codereview.chromium.org/1931503002/diff/80001/chrome/browser/backgrou...
chrome/browser/background/background_mode_optimizer.cc:24: return
std::unique_ptr<BackgroundModeOptimizer>();
nit: return nullptr here and 31.

https://codereview.chromium.org/1931503002/diff/80001/chrome/browser/backgrou...
chrome/browser/background/background_mode_optimizer.cc:67:
g_browser_process->background_mode_manager()->EndBackgroundMode();
If the restart attempt fails, is the background mode manager then in a weird
state?

https://codereview.chromium.org/1931503002/diff/80001/chrome/browser/backgrou...
File chrome/browser/background/background_mode_optimizer.h (right):

https://codereview.chromium.org/1931503002/diff/80001/chrome/browser/backgrou...
chrome/browser/background/background_mode_optimizer.h:23: static
std::unique_ptr<BackgroundModeOptimizer> Create();
nit: constructor/destructor before other methods.

https://codereview.chromium.org/1931503002/diff/80001/chrome/browser/backgrou...
chrome/browser/background/background_mode_optimizer.h:32: void
TryBrowserRestart();
nit: newline between 31/32 and a description for TryBrowserRestart.

https://codereview.chromium.org/1931503002/diff/80001/chrome/browser/browser_...
File chrome/browser/browser_shutdown.cc (right):

https://codereview.chromium.org/1931503002/diff/80001/chrome/browser/browser_...
chrome/browser/browser_shutdown.cc:246: if (g_should_restart_in_background)
Having a global is error prone. Can you make this a parameter to this function?

[Bernhard Bauer, 2016-05-19 08:56:56]

https://codereview.chromium.org/1931503002/diff/80001/chrome/browser/backgrou...
File chrome/browser/background/background_mode_manager.h (right):

https://codereview.chromium.org/1931503002/diff/80001/chrome/browser/backgrou...
chrome/browser/background/background_mode_manager.h:92: // Invoked to take
Chrome out of KeepAlive mode - chrome stops running in
Nit: capitalize "Chrome".

[dgn, 2016-06-20 17:45:19]

PTAL. Test still TBD

https://codereview.chromium.org/1931503002/diff/80001/chrome/browser/backgrou...
File chrome/browser/background/background_mode_manager.h (right):

https://codereview.chromium.org/1931503002/diff/80001/chrome/browser/backgrou...
chrome/browser/background/background_mode_manager.h:92: // Invoked to take
Chrome out of KeepAlive mode - chrome stops running in
On 2016/05/19 08:56:56, Bernhard Bauer wrote:
> Nit: capitalize "Chrome".

Done.

https://codereview.chromium.org/1931503002/diff/80001/chrome/browser/backgrou...
chrome/browser/background/background_mode_manager.h:94: void
EndBackgroundMode();
On 2016/05/18 19:23:05, sky wrote:
> Move implementation to match new position.

Made this private as it was before, introduced another method to replace it.

https://codereview.chromium.org/1931503002/diff/80001/chrome/browser/backgrou...
File chrome/browser/background/background_mode_optimizer.cc (right):

https://codereview.chromium.org/1931503002/diff/80001/chrome/browser/backgrou...
chrome/browser/background/background_mode_optimizer.cc:24: return
std::unique_ptr<BackgroundModeOptimizer>();
On 2016/05/18 19:23:06, sky wrote:
> nit: return nullptr here and 31.

Done.

https://codereview.chromium.org/1931503002/diff/80001/chrome/browser/backgrou...
chrome/browser/background/background_mode_optimizer.cc:67:
g_browser_process->background_mode_manager()->EndBackgroundMode();
On 2016/05/18 19:23:06, sky wrote:
> If the restart attempt fails, is the background mode manager then in a weird
> state?

The restart specific flags are set unconditionally and we check if we can
shutdown afterwards, trying to close browsers, which would affect the KeepAlive
state. We already get here only when the KeepAlive state is favourable. The
other thing is IsTryingToQuit(), but we do check it here.

So the restart attempt should succeed. I'm not sure how to check for restart
failure either, as we don't have return value or other flags that are set when
the whole thing is done. Well, the browser is supposed to restart when that
works.

https://codereview.chromium.org/1931503002/diff/80001/chrome/browser/backgrou...
File chrome/browser/background/background_mode_optimizer.h (right):

https://codereview.chromium.org/1931503002/diff/80001/chrome/browser/backgrou...
chrome/browser/background/background_mode_optimizer.h:23: static
std::unique_ptr<BackgroundModeOptimizer> Create();
On 2016/05/18 19:23:06, sky wrote:
> nit: constructor/destructor before other methods.

Done.

https://codereview.chromium.org/1931503002/diff/80001/chrome/browser/backgrou...
chrome/browser/background/background_mode_optimizer.h:32: void
TryBrowserRestart();
On 2016/05/18 19:23:06, sky wrote:
> nit: newline between 31/32 and a description for TryBrowserRestart.

Done.

https://codereview.chromium.org/1931503002/diff/80001/chrome/browser/browser_...
File chrome/browser/browser_shutdown.cc (right):

https://codereview.chromium.org/1931503002/diff/80001/chrome/browser/browser_...
chrome/browser/browser_shutdown.cc:246: if (g_should_restart_in_background)
On 2016/05/18 19:23:06, sky wrote:
> Having a global is error prone. Can you make this a parameter to this
function?

Done. It made it a static class member in BackgroundModeManager. Sadly I
couldn't make it instance member because the BMM gets cleared by the time we
want to read the value.

[sky, 2016-06-20 19:31:55]

https://codereview.chromium.org/1931503002/diff/80001/chrome/browser/backgrou...
File chrome/browser/background/background_mode_optimizer.cc (right):

https://codereview.chromium.org/1931503002/diff/80001/chrome/browser/backgrou...
chrome/browser/background/background_mode_optimizer.cc:67:
g_browser_process->background_mode_manager()->EndBackgroundMode();
On 2016/06/20 17:45:18, dgn wrote:
> On 2016/05/18 19:23:06, sky wrote:
> > If the restart attempt fails, is the background mode manager then in a weird
> > state?
> 
> The restart specific flags are set unconditionally and we check if we can
> shutdown afterwards, trying to close browsers, which would affect the
KeepAlive
> state. We already get here only when the KeepAlive state is favourable. The
> other thing is IsTryingToQuit(), but we do check it here.
> 
> So the restart attempt should succeed. I'm not sure how to check for restart
> failure either, as we don't have return value or other flags that are set when
> the whole thing is done. Well, the browser is supposed to restart when that
> works.

I think you should make AttemptRestart return a status. Otherwise it's easy to
break this. Alternatively an observer that is notified if the restart fails.

[dgn, 2016-07-05 19:54:12]

On 2016/06/20 19:31:55, sky wrote:
> I think you should make AttemptRestart return a status. Otherwise it's easy to
> break this. Alternatively an observer that is notified if the restart fails.


PTAL. I addressed that by resetting the flags in
BrowserCloseManager::CancelBrowserClose(). WDYT?
Still TBD: tests.

[sky, 2016-07-06 16:55:33]

https://codereview.chromium.org/1931503002/diff/280001/chrome/browser/backgro...
File chrome/browser/background/background_mode_manager.h (right):

https://codereview.chromium.org/1931503002/diff/280001/chrome/browser/backgro...
chrome/browser/background/background_mode_manager.h:85: static bool
ShouldRestartInBackground();
nit: inline these are these are trivially.

https://codereview.chromium.org/1931503002/diff/280001/chrome/browser/backgro...
File chrome/browser/background/background_mode_optimizer.cc (right):

https://codereview.chromium.org/1931503002/diff/280001/chrome/browser/backgro...
chrome/browser/background/background_mode_optimizer.cc:19: // static
nit: make order match header.

https://codereview.chromium.org/1931503002/diff/280001/chrome/browser/browser...
File chrome/browser/browser_shutdown.h (right):

https://codereview.chromium.org/1931503002/diff/280001/chrome/browser/browser...
chrome/browser/browser_shutdown.h:46: void ShutdownPostThreadsStop(bool
restart_last_session,
nit: use bitmask to improve readability. Also, document what
restart_in_background means.

https://codereview.chromium.org/1931503002/diff/280001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1931503002/diff/280001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:2025: restart_in_background =
BackgroundModeManager::ShouldRestartInBackground();
Doesn't this only matter if restart_last_session_?

https://codereview.chromium.org/1931503002/diff/280001/chrome/browser/lifetim...
File chrome/browser/lifetime/application_lifetime.h (right):

https://codereview.chromium.org/1931503002/diff/280001/chrome/browser/lifetim...
chrome/browser/lifetime/application_lifetime.h:89: // Calling |AttemptRestart|
unconditionally sets some preference to be used
nit: generally | is used for arguments and members, where as () is used for
functions, e.g. AttemptRestart().

https://codereview.chromium.org/1931503002/diff/280001/chrome/browser/lifetim...
chrome/browser/lifetime/application_lifetime.h:93: void ResetRestartPrefs();
Can this move to browser_shutdown::SetTryingToQuit if quitting is false? Having
this public is confusing and not something we normally want folks calling,
right?

[dgn, 2016-07-22 13:50:11]

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

[commit-bot: I haz the power, 2016-07-22 13:50:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 14:02:27]

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

[commit-bot: I haz the power, 2016-07-22 14:02:28]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[dgn, 2016-07-22 14:23:45]

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

[commit-bot: I haz the power, 2016-07-22 14:23:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 14:58:29]

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

[commit-bot: I haz the power, 2016-07-22 14:58:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dgn, 2016-07-22 15:17:36]

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

[commit-bot: I haz the power, 2016-07-22 15:17:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dgn, 2016-07-22 15:18:09]

PTAL

https://codereview.chromium.org/1931503002/diff/280001/chrome/browser/backgro...
File chrome/browser/background/background_mode_manager.h (right):

https://codereview.chromium.org/1931503002/diff/280001/chrome/browser/backgro...
chrome/browser/background/background_mode_manager.h:85: static bool
ShouldRestartInBackground();
On 2016/07/06 16:55:33, sky wrote:
> nit: inline these are these are trivially.

Done.

https://codereview.chromium.org/1931503002/diff/280001/chrome/browser/backgro...
File chrome/browser/background/background_mode_optimizer.cc (right):

https://codereview.chromium.org/1931503002/diff/280001/chrome/browser/backgro...
chrome/browser/background/background_mode_optimizer.cc:19: // static
On 2016/07/06 16:55:33, sky wrote:
> nit: make order match header.

Done.

https://codereview.chromium.org/1931503002/diff/280001/chrome/browser/browser...
File chrome/browser/browser_shutdown.h (right):

https://codereview.chromium.org/1931503002/diff/280001/chrome/browser/browser...
chrome/browser/browser_shutdown.h:46: void ShutdownPostThreadsStop(bool
restart_last_session,
On 2016/07/06 16:55:33, sky wrote:
> nit: use bitmask to improve readability. Also, document what
> restart_in_background means.

Done.

https://codereview.chromium.org/1931503002/diff/280001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1931503002/diff/280001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:2025: restart_in_background =
BackgroundModeManager::ShouldRestartInBackground();
On 2016/07/06 16:55:33, sky wrote:
> Doesn't this only matter if restart_last_session_?

Yes, made that explicit here.

https://codereview.chromium.org/1931503002/diff/280001/chrome/browser/lifetim...
File chrome/browser/lifetime/application_lifetime.h (right):

https://codereview.chromium.org/1931503002/diff/280001/chrome/browser/lifetim...
chrome/browser/lifetime/application_lifetime.h:89: // Calling |AttemptRestart|
unconditionally sets some preference to be used
On 2016/07/06 16:55:33, sky wrote:
> nit: generally | is used for arguments and members, where as () is used for
> functions, e.g. AttemptRestart().

Ack.

https://codereview.chromium.org/1931503002/diff/280001/chrome/browser/lifetim...
chrome/browser/lifetime/application_lifetime.h:93: void ResetRestartPrefs();
On 2016/07/06 16:55:33, sky wrote:
> Can this move to browser_shutdown::SetTryingToQuit if quitting is false?
Having
> this public is confusing and not something we normally want folks calling,
> right?

Good point, done.

[commit-bot: I haz the power, 2016-07-22 16:04:59]

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

[commit-bot: I haz the power, 2016-07-22 16:05:00]

Dry run: This issue passed the CQ dry run.

[sky, 2016-07-22 16:57:52]

https://codereview.chromium.org/1931503002/diff/340001/chrome/browser/backgro...
File chrome/browser/background/background_mode_manager.cc (right):

https://codereview.chromium.org/1931503002/diff/340001/chrome/browser/backgro...
chrome/browser/background/background_mode_manager.cc:96: bool
BackgroundModeManager::should_restart_in_background_ = false;
in general we prefix statics like this with
// static
(on the previous line)

https://codereview.chromium.org/1931503002/diff/340001/chrome/browser/backgro...
chrome/browser/background/background_mode_manager.cc:293:
BackgroundModeManager::should_restart_in_background_ = false;
It isn't obvious why you need this here. Please add a comment. Also, I don't
think you need BackgroundModeManager::

https://codereview.chromium.org/1931503002/diff/340001/chrome/browser/backgro...
File chrome/browser/background/background_mode_manager.h (right):

https://codereview.chromium.org/1931503002/diff/340001/chrome/browser/backgro...
chrome/browser/background/background_mode_manager.h:86: return
BackgroundModeManager::should_restart_in_background_;
Why do you need BackgroundModeManager:: here and line 90?

https://codereview.chromium.org/1931503002/diff/340001/chrome/browser/backgro...
chrome/browser/background/background_mode_manager.h:89: static void
should_restart_in_background(bool enable) {
set_should...

https://codereview.chromium.org/1931503002/diff/340001/chrome/browser/browser...
File chrome/browser/browser_shutdown.cc (right):

https://codereview.chromium.org/1931503002/diff/340001/chrome/browser/browser...
chrome/browser/browser_shutdown.cc:342: // Reset the restart-related
preferences.
This just documents what the code is doing. Instead document why this code is
here. Why do you need to reset restart-related preferences?

[dgn, 2016-07-25 13:09:11]

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

[commit-bot: I haz the power, 2016-07-25 13:09:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-25 13:41:03]

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

[commit-bot: I haz the power, 2016-07-25 13:41:04]

Dry run: This issue passed the CQ dry run.

[dgn, 2016-07-25 17:32:28]

PTAL

https://codereview.chromium.org/1931503002/diff/340001/chrome/browser/backgro...
File chrome/browser/background/background_mode_manager.cc (right):

https://codereview.chromium.org/1931503002/diff/340001/chrome/browser/backgro...
chrome/browser/background/background_mode_manager.cc:96: bool
BackgroundModeManager::should_restart_in_background_ = false;
On 2016/07/22 16:57:52, sky wrote:
> in general we prefix statics like this with
> // static
> (on the previous line)

Ah! on variables too, okay. Done.

https://codereview.chromium.org/1931503002/diff/340001/chrome/browser/backgro...
chrome/browser/background/background_mode_manager.cc:293:
BackgroundModeManager::should_restart_in_background_ = false;
On 2016/07/22 16:57:52, sky wrote:
> It isn't obvious why you need this here. Please add a comment. Also, I don't
> think you need BackgroundModeManager::

Not needed. Removed.

https://codereview.chromium.org/1931503002/diff/340001/chrome/browser/backgro...
File chrome/browser/background/background_mode_manager.h (right):

https://codereview.chromium.org/1931503002/diff/340001/chrome/browser/backgro...
chrome/browser/background/background_mode_manager.h:86: return
BackgroundModeManager::should_restart_in_background_;
On 2016/07/22 16:57:52, sky wrote:
> Why do you need BackgroundModeManager:: here and line 90?

Not needed. Removed

https://codereview.chromium.org/1931503002/diff/340001/chrome/browser/backgro...
chrome/browser/background/background_mode_manager.h:89: static void
should_restart_in_background(bool enable) {
On 2016/07/22 16:57:52, sky wrote:
> set_should...

Done.

https://codereview.chromium.org/1931503002/diff/340001/chrome/browser/browser...
File chrome/browser/browser_shutdown.cc (right):

https://codereview.chromium.org/1931503002/diff/340001/chrome/browser/browser...
chrome/browser/browser_shutdown.cc:342: // Reset the restart-related
preferences.
On 2016/07/22 16:57:52, sky wrote:
> This just documents what the code is doing. Instead document why this code is
> here. Why do you need to reset restart-related preferences?

Done.

[sky, 2016-07-25 20:05:03]

LGTM

[dgn, 2016-07-26 08:57:13]

The CQ bit was checked by dgn@chromium.org

[commit-bot: I haz the power, 2016-07-26 08:57:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dgn, 2016-07-26 08:59:52]

On 2016/07/25 20:05:03, sky wrote:
> LGTM

Thanks!

[commit-bot: I haz the power, 2016-07-26 09:02:44]

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

[commit-bot: I haz the power, 2016-07-26 09:02:46]

Failed to apply the patch.

[commit-bot: I haz the power, 2016-07-26 09:04:37]

Description was changed from

==========
Add BackgroundModeOptimizer that can restart the browser

Its initialization is behind the BackgroundModeAllowRestart
flag, and is supported only on Windows and Linux

BUG=585080
==========

to

==========
Add BackgroundModeOptimizer that can restart the browser

Its initialization is behind the BackgroundModeAllowRestart
flag, and is supported only on Windows and Linux

BUG=585080

Committed: https://crrev.com/3eaf67ed9424c847489410ec5956ddd1bf9a1557
Cr-Commit-Position: refs/heads/master@{#407742}
==========

[commit-bot: I haz the power, 2016-07-26 09:04:38]

Patchset 16 (id:??) landed as
https://crrev.com/3eaf67ed9424c847489410ec5956ddd1bf9a1557
Cr-Commit-Position: refs/heads/master@{#407742}