Replace BrowserProces::AddRefModule/RemoveModule by ScopedKeepAlive

Replace BrowserProces::AddRefModule/RemoveModule by ScopedKeepAlive

Uses ScopedKeepAlive for Browser and menus, and remove references
to other separate counters to keep Chrome alive. KeepAliveRegistry
should now be the source of truth to look up what is keeping the
browser process running.

Doing so breaks many assumptions from the unit and browser tests,
so most changes are to fix them.

BUG=587926
Committed: https://crrev.com/fe075c81bffa961dfbc1bccc2e47501f45b88484
Cr-Commit-Position: refs/heads/master@{#381938}

[dgn, 2016-03-16 15:10:49]

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

[dgn, 2016-03-16 15:10:50]

PTAL. This should be the last CL for the KeepAlive unification.

https://codereview.chromium.org/1803143002/diff/80001/chrome/browser/apps/app...
File chrome/browser/apps/app_window_interactive_uitest.cc (left):

https://codereview.chromium.org/1803143002/diff/80001/chrome/browser/apps/app...
chrome/browser/apps/app_window_interactive_uitest.cc:532:
EXPECT_TRUE(KeepAliveRegistry::GetInstance()->IsKeepingAlive());
IsKeepingAlive now includes the browser windows as well. This test is concerned
about the keepalives related to the delegate only, so I made that accessible
instead.

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

https://codereview.chromium.org/1803143002/diff/80001/chrome/browser/lifetime...
chrome/browser/lifetime/application_lifetime.cc:294: ShutdownIfNoBrowsers();
Moved the method out of CloseAllBrowsers to make what we want to do clearer.

- GetTotalBrowserCount() gets checked in ShutdownIfNoBrowsers, and we only call
this when IsKeepingAlive is false anyway, which implies no browsers.
- g_disable_shutdown_for_testing gets now set for all unit tests, allowing to
skip the message loop check (which wasn't covering all unit tests in the first
place)

https://codereview.chromium.org/1803143002/diff/80001/chrome/browser/lifetime...
File chrome/browser/lifetime/keep_alive_registry.h (right):

https://codereview.chromium.org/1803143002/diff/80001/chrome/browser/lifetime...
chrome/browser/lifetime/keep_alive_registry.h:25: bool
IsOriginRegistered(KeepAliveOrigin origin) const;
Should I rename it xxForTesting? I suppose it could have some legitimate use in
non test code.

[sky, 2016-03-16 16:04:32]

https://codereview.chromium.org/1803143002/diff/80001/chrome/browser/browser_...
File chrome/browser/browser_process.h (right):

https://codereview.chromium.org/1803143002/diff/80001/chrome/browser/browser_...
chrome/browser/browser_process.h:190: virtual void Pin() = 0;
I don't like having these public as then anyone can call them, which we don't
want. How about having KeepAliveRegistry talk to a delegate that browserprocess
implements? That way you can make these protected.

[dgn, 2016-03-16 22:28:36]

PTAL

https://codereview.chromium.org/1803143002/diff/80001/chrome/browser/browser_...
File chrome/browser/browser_process.h (right):

https://codereview.chromium.org/1803143002/diff/80001/chrome/browser/browser_...
chrome/browser/browser_process.h:190: virtual void Pin() = 0;
On 2016/03/16 16:04:32, sky wrote:
> I don't like having these public as then anyone can call them, which we don't
> want. How about having KeepAliveRegistry talk to a delegate that
browserprocess
> implements? That way you can make these protected.

Good idea, it also allows to get rid of a lot of code that disabled shutdown for
unit tests, by just not registering the listener in TestingBrowserProcess, which
is nice.

I used KeepAliveStateObserver to do it. WDYT?

[sky, 2016-03-17 00:02:24]

I like using the observer!

https://codereview.chromium.org/1803143002/diff/80001/chrome/browser/lifetime...
File chrome/browser/lifetime/keep_alive_registry.h (right):

https://codereview.chromium.org/1803143002/diff/80001/chrome/browser/lifetime...
chrome/browser/lifetime/keep_alive_registry.h:25: bool
IsOriginRegistered(KeepAliveOrigin origin) const;
On 2016/03/16 15:10:50, dgn wrote:
> Should I rename it xxForTesting? I suppose it could have some legitimate use
in
> non test code.

I wouldn't bother.

https://codereview.chromium.org/1803143002/diff/140001/chrome/browser/browser...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/1803143002/diff/140001/chrome/browser/browser...
chrome/browser/browser_process_impl.cc:981: void
BrowserProcessImpl::OnKeepingAliveStateChanged(bool is_keeping_alive) {
Did you make sure we don't unnecessary call this quickly when going from false
to true?

https://codereview.chromium.org/1803143002/diff/140001/chrome/browser/browser...
chrome/browser/browser_process_impl.cc:986: };
nit: no {}

https://codereview.chromium.org/1803143002/diff/140001/chrome/browser/browser...
File chrome/browser/browser_process_impl.h (right):

https://codereview.chromium.org/1803143002/diff/140001/chrome/browser/browser...
chrome/browser/browser_process_impl.h:190: void Pin();
Move implementation to match new position.

https://codereview.chromium.org/1803143002/diff/140001/chrome/browser/lifetim...
File chrome/browser/lifetime/keep_alive_registry.h (right):

https://codereview.chromium.org/1803143002/diff/140001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_registry.h:54: void
OnKeepingAliveStateChanged(bool new_keeping_alive);
OnKeepAliveStateChanged?

[dgn, 2016-03-17 10:00:09]

PTAL

https://codereview.chromium.org/1803143002/diff/140001/chrome/browser/browser...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/1803143002/diff/140001/chrome/browser/browser...
chrome/browser/browser_process_impl.cc:981: void
BrowserProcessImpl::OnKeepingAliveStateChanged(bool is_keeping_alive) {
On 2016/03/17 00:02:24, sky wrote:
> Did you make sure we don't unnecessary call this quickly when going from false
> to true?

Which case are you referring to? In the normal startup, we want to call Pin:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/lif...

https://codereview.chromium.org/1803143002/diff/140001/chrome/browser/browser...
chrome/browser/browser_process_impl.cc:986: };
On 2016/03/17 00:02:24, sky wrote:
> nit: no {}

Do you mean no semicolon? Or no brackets on same line for the next function? Did
both.

https://codereview.chromium.org/1803143002/diff/140001/chrome/browser/browser...
File chrome/browser/browser_process_impl.h (right):

https://codereview.chromium.org/1803143002/diff/140001/chrome/browser/browser...
chrome/browser/browser_process_impl.h:190: void Pin();
On 2016/03/17 00:02:24, sky wrote:
> Move implementation to match new position.

Done.

https://codereview.chromium.org/1803143002/diff/140001/chrome/browser/lifetim...
File chrome/browser/lifetime/keep_alive_registry.h (right):

https://codereview.chromium.org/1803143002/diff/140001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_registry.h:54: void
OnKeepingAliveStateChanged(bool new_keeping_alive);
On 2016/03/17 00:02:24, sky wrote:
> OnKeepAliveStateChanged?

Done.

[sky, 2016-03-17 21:01:00]

LGTM

[dgn, 2016-03-18 09:57:17]

The CQ bit was checked by dgn@chromium.org

[commit-bot: I haz the power, 2016-03-18 09:57:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1803143002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1803143002/160001

[commit-bot: I haz the power, 2016-03-18 11:25:44]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-03-18 11:26:51]

Description was changed from

==========
Replace BrowserProces::AddRefModule/RemoveModule by ScopedKeepAlive

Uses ScopedKeepAlive for Browser and menus, and remove references
to other separate counters to keep Chrome alive. KeepAliveRegistry
should now be the source of truth to look up what is keeping the
browser process running.

Doing so breaks many assumptions from the unit and browser tests,
so most changes are to fix them.

BUG=587926
==========

to

==========
Replace BrowserProces::AddRefModule/RemoveModule by ScopedKeepAlive

Uses ScopedKeepAlive for Browser and menus, and remove references
to other separate counters to keep Chrome alive. KeepAliveRegistry
should now be the source of truth to look up what is keeping the
browser process running.

Doing so breaks many assumptions from the unit and browser tests,
so most changes are to fix them.

BUG=587926

Committed: https://crrev.com/fe075c81bffa961dfbc1bccc2e47501f45b88484
Cr-Commit-Position: refs/heads/master@{#381938}
==========

[commit-bot: I haz the power, 2016-03-18 11:26:52]

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