Add ScopedKeepAlive to c/b/lifetime

Add ScopedKeepAlive to c/b/lifetime

It moves from c/b/apps as it can be reused more generally across
//chrome/browser. It works with the new KeepAliveRegistry which is
the class keeping all the relevant information in an accessible place.

This patch also reuses ScopedKeepAlive instead of
c/b/ui/views/AutoKeepAlive

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

[dgn, 2016-02-19 17:16:17]

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

[dgn, 2016-02-19 17:16:18]

PTAL

[Bernhard Bauer, 2016-02-19 17:39:28]

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

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/backgrou...
chrome/browser/background/background_mode_manager.cc:766: keep_alive_.reset(new
ScopedKeepAlive(
Nit: braces

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/backgrou...
chrome/browser/background/background_mode_manager.cc:774: keep_alive_.reset();
You can unconditionally reset it; it will be a no-op if it was previously null.

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/lifetime...
File chrome/browser/lifetime/keep_alive_registry.cc (right):

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/lifetime...
chrome/browser/lifetime/keep_alive_registry.cc:11: // Helper function to remove
one token from the multisets.
Empty line after opening the namespace.

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/lifetime...
chrome/browser/lifetime/keep_alive_registry.cc:14: if (it != container.end())
I would DCHECK this -- it shouldn't be possible to remove a nonexisting token.

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/lifetime...
chrome/browser/lifetime/keep_alive_registry.cc:17: }
// namespace

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/lifetime...
chrome/browser/lifetime/keep_alive_registry.cc:27:
DCHECK_EQ(registered_tokens_.size(), 0u);
Put the expected value first for nicer error messages in case of failure.

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

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/lifetime...
chrome/browser/lifetime/keep_alive_registry.h:20: class KeepAliveRegistry {
You might be able to move this whole class into an anonymous namespace in
scoped_alive.cc. No one else should use this.

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/lifetime...
chrome/browser/lifetime/keep_alive_registry.h:34: std::multiset<std::string>
reset_allowed_tokens_;
|restart_allowed_tokens_|?

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/lifetime...
File chrome/browser/lifetime/scoped_keep_alive.h (right):

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/lifetime...
chrome/browser/lifetime/scoped_keep_alive.h:17: explicit ScopedKeepAlive(const
std::string& token,
Explain what the token is for?

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/lifetime...
chrome/browser/lifetime/scoped_keep_alive.h:18: OptimizationOptions opt =
OptimizationOptions::NONE);
I'm not very happy about the default argument here -- I think I would rather
have every client think about what optimizations can be applied. I might even
prefer using a list of two-state enums, just to make sure that every reader of
the code sees which optimizations we're talking about (rather than e.g. using a
bitfield, where seeing just NONE doesn't tell the reader which possible
optimizations there are).

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (left):

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:354: new
ChromeAppDelegate(scoped_ptr<ScopedKeepAlive>())),
Urr, this was a null pointer, no? (I.e. no keep alive)

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:353:
scoped_ptr<extensions::AppDelegate>(new ChromeAppDelegate(
Y'know, I'm thinking that ChromeAppDelegate should be what creates the
ScopedKeepAlive, and it should instead just take a boolean that says whether to
keep the browser alive or not.

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/auto_keep_alive.cc (right):

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/auto_keep_alive.cc:17: keep_alive.reset(new
ScopedKeepAlive("AutoView"));
Directly return the a new scoped_ptr here, and return nullptr below?

(In addition, you could invert the condition; might make things a bit neater.)

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/auto_keep_alive.h (right):

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/auto_keep_alive.h:14: // On ASH environments, will
return a null pointer.
Nit: "Ash" (only the first letter is capitalized).

[dgn, 2016-02-19 18:04:28]

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

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/backgrou...
chrome/browser/background/background_mode_manager.cc:766: keep_alive_.reset(new
ScopedKeepAlive(
On 2016/02/19 17:39:28, Bernhard Bauer wrote:
> Nit: braces

Done.

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/backgrou...
chrome/browser/background/background_mode_manager.cc:774: keep_alive_.reset();
On 2016/02/19 17:39:28, Bernhard Bauer wrote:
> You can unconditionally reset it; it will be a no-op if it was previously
null.

Done.

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/lifetime...
File chrome/browser/lifetime/keep_alive_registry.cc (right):

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/lifetime...
chrome/browser/lifetime/keep_alive_registry.cc:11: // Helper function to remove
one token from the multisets.
On 2016/02/19 17:39:28, Bernhard Bauer wrote:
> Empty line after opening the namespace.

Done.

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/lifetime...
chrome/browser/lifetime/keep_alive_registry.cc:14: if (it != container.end())
On 2016/02/19 17:39:28, Bernhard Bauer wrote:
> I would DCHECK this -- it shouldn't be possible to remove a nonexisting token.

Done.

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/lifetime...
chrome/browser/lifetime/keep_alive_registry.cc:17: }
On 2016/02/19 17:39:28, Bernhard Bauer wrote:
> // namespace

Done.

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/lifetime...
chrome/browser/lifetime/keep_alive_registry.cc:27:
DCHECK_EQ(registered_tokens_.size(), 0u);
On 2016/02/19 17:39:28, Bernhard Bauer wrote:
> Put the expected value first for nicer error messages in case of failure.

Done.

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

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/lifetime...
chrome/browser/lifetime/keep_alive_registry.h:20: class KeepAliveRegistry {
On 2016/02/19 17:39:28, Bernhard Bauer wrote:
> You might be able to move this whole class into an anonymous namespace in
> scoped_alive.cc. No one else should use this.

Can I do that at the end of the migration? I'm not sure what I will run into
while updating other parts of chrome. 
Also I will probably want to for testing.
And registering observers, which is a legit usage.

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/lifetime...
chrome/browser/lifetime/keep_alive_registry.h:34: std::multiset<std::string>
reset_allowed_tokens_;
On 2016/02/19 17:39:28, Bernhard Bauer wrote:
> |restart_allowed_tokens_|?

oops forgot to fix that. Done.

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/lifetime...
File chrome/browser/lifetime/scoped_keep_alive.h (right):

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/lifetime...
chrome/browser/lifetime/scoped_keep_alive.h:17: explicit ScopedKeepAlive(const
std::string& token,
On 2016/02/19 17:39:28, Bernhard Bauer wrote:
> Explain what the token is for?

Done.

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (left):

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:354: new
ChromeAppDelegate(scoped_ptr<ScopedKeepAlive>())),
On 2016/02/19 17:39:28, Bernhard Bauer wrote:
> Urr, this was a null pointer, no? (I.e. no keep alive)

Done in Patch 4

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/auto_keep_alive.cc (right):

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/auto_keep_alive.cc:17: keep_alive.reset(new
ScopedKeepAlive("AutoView"));
On 2016/02/19 17:39:28, Bernhard Bauer wrote:
> Directly return the a new scoped_ptr here, and return nullptr below?
> 
> (In addition, you could invert the condition; might make things a bit neater.)

Done.

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/auto_keep_alive.h (right):

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/auto_keep_alive.h:14: // On ASH environments, will
return a null pointer.
On 2016/02/19 17:39:28, Bernhard Bauer wrote:
> Nit: "Ash" (only the first letter is capitalized).

Done.

[dgn, 2016-02-19 19:14:22]

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

[dgn, 2016-02-19 19:14:23]

sky@chromium.org: PTAL. This is the first part of the KeepAlive unification
changes. I plan to have 3 or 4 CLs after that to be done with it.

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/lifetime...
File chrome/browser/lifetime/scoped_keep_alive.h (right):

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/lifetime...
chrome/browser/lifetime/scoped_keep_alive.h:18: OptimizationOptions opt =
OptimizationOptions::NONE);
On 2016/02/19 17:39:28, Bernhard Bauer wrote:
> I'm not very happy about the default argument here -- I think I would rather
> have every client think about what optimizations can be applied. I might even
> prefer using a list of two-state enums, just to make sure that every reader of
> the code sees which optimizations we're talking about (rather than e.g. using
a
> bitfield, where seeing just NONE doesn't tell the reader which possible
> optimizations there are).

Okay. I'll remove the argument for now then, and add one back when I implement
the observer, since I'll have to revisit all the call sites to update the
arguments anyway. It probably makes more sense that way.

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:353:
scoped_ptr<extensions::AppDelegate>(new ChromeAppDelegate(
On 2016/02/19 17:39:28, Bernhard Bauer wrote:
> Y'know, I'm thinking that ChromeAppDelegate should be what creates the
> ScopedKeepAlive, and it should instead just take a boolean that says whether
to
> keep the browser alive or not.

Hum.. what about the optimization flags in that case?

[sky, 2016-02-19 21:02:49]

I'm also not a fan of the random strings everywhere. WDYT of an enum?

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

https://codereview.chromium.org/1708343002/diff/120001/chrome/browser/backgro...
chrome/browser/background/background_mode_manager.cc:686:
base::Unretained(this)));
How do you know the unretained is safe here?

https://codereview.chromium.org/1708343002/diff/120001/chrome/browser/lifetim...
File chrome/browser/lifetime/keep_alive_registry.cc (right):

https://codereview.chromium.org/1708343002/diff/120001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_registry.cc:26:
KeepAliveRegistry::KeepAliveRegistry() {}
nit: make declaration/definition order match (see style guide).

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

https://codereview.chromium.org/1708343002/diff/120001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_registry.h:25: virtual ~KeepAliveRegistry();
Why the virtual?

https://codereview.chromium.org/1708343002/diff/120001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_registry.h:27: std::multiset<std::string>
registered_tokens_;
Is there a reason why you aren't using a map<string,int> ? I get that multiset
works, but it's a bit esoteric for this use case.

https://codereview.chromium.org/1708343002/diff/120001/chrome/browser/lifetim...
File chrome/browser/lifetime/scoped_keep_alive.h (right):

https://codereview.chromium.org/1708343002/diff/120001/chrome/browser/lifetim...
chrome/browser/lifetime/scoped_keep_alive.h:32: std::string token_;
const

https://codereview.chromium.org/1708343002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/auto_keep_alive.cc (right):

https://codereview.chromium.org/1708343002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/auto_keep_alive.cc:11: chrome::HostDesktopType
desktop_type =
desktop_type is effectively dead and in the process of being removed. If
chromeos doesn't need the keepalive you'll have to remove calls to this code
from there.

[dgn, 2016-02-22 18:01:20]

PTAL

I replaced the strings by struct pointers. These structs have a label member
that can hold the strings. They are intended for debug purposes (could be a
chrome:// page at some point) so keeping a string there is interesting. If
needed, we can have macros that will keep the strings out of prod builds, but as
static members I don't think they have much impact.

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:353:
scoped_ptr<extensions::AppDelegate>(new ChromeAppDelegate(
On 2016/02/19 17:39:28, Bernhard Bauer wrote:
> Y'know, I'm thinking that ChromeAppDelegate should be what creates the
> ScopedKeepAlive, and it should instead just take a boolean that says whether
to
> keep the browser alive or not.

Done.

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

https://codereview.chromium.org/1708343002/diff/120001/chrome/browser/backgro...
chrome/browser/background/background_mode_manager.cc:686:
base::Unretained(this)));
On 2016/02/19 21:02:49, sky wrote:
> How do you know the unretained is safe here?

It's covered by the keep_alive_for_startup_ check, which keeps the browser alive
and because BackgroundModeManager is owned by browser_process, it will stay
around until browser_process shuts down, which is caused by removing keep
alives. I'll add a comment to say that.

https://codereview.chromium.org/1708343002/diff/120001/chrome/browser/lifetim...
File chrome/browser/lifetime/keep_alive_registry.cc (right):

https://codereview.chromium.org/1708343002/diff/120001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_registry.cc:26:
KeepAliveRegistry::KeepAliveRegistry() {}
On 2016/02/19 21:02:49, sky wrote:
> nit: make declaration/definition order match (see style guide).

Done.

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

https://codereview.chromium.org/1708343002/diff/120001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_registry.h:25: virtual ~KeepAliveRegistry();
On 2016/02/19 21:02:49, sky wrote:
> Why the virtual?

No subclass involved here, so removed it.

https://codereview.chromium.org/1708343002/diff/120001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_registry.h:27: std::multiset<std::string>
registered_tokens_;
On 2016/02/19 21:02:49, sky wrote:
> Is there a reason why you aren't using a map<string,int> ? I get that multiset
> works, but it's a bit esoteric for this use case.

With the int being the number of registered tokens with the same name? For the
optimizations later, I plan to have another multiset for tokens allowing
restart, and comparing the size of the 2 multisets to determine when restart is
safe. That would not work with maps and counts. I would have to maintain a
separate counter or compute it everytime, or do something else to track state

https://codereview.chromium.org/1708343002/diff/120001/chrome/browser/lifetim...
File chrome/browser/lifetime/scoped_keep_alive.h (right):

https://codereview.chromium.org/1708343002/diff/120001/chrome/browser/lifetim...
chrome/browser/lifetime/scoped_keep_alive.h:32: std::string token_;
On 2016/02/19 21:02:49, sky wrote:
> const

Done.

https://codereview.chromium.org/1708343002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/auto_keep_alive.cc (right):

https://codereview.chromium.org/1708343002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/auto_keep_alive.cc:11: chrome::HostDesktopType
desktop_type =
On 2016/02/19 21:02:49, sky wrote:
> desktop_type is effectively dead and in the process of being removed. If
> chromeos doesn't need the keepalive you'll have to remove calls to this code
> from there.

Chrome OS doesn't require it because it always increments the keepalive count
during initialization: 
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

That's a special case that I feel should not be needed, but if it has to be
addressed that should be a separate issue. Should I file it?

In the meantime I'll just replace it by excluding ChromeOS when creating the
keepalive

https://codereview.chromium.org/1708343002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/user_manager_view.cc (right):

https://codereview.chromium.org/1708343002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:237:
keep_alive_(AutoKeepAlive(nullptr)),
not sure about this one. AFAIK it is not used on Chrome OS. But it's included in
the files compiled for that platform...

[Bernhard Bauer, 2016-02-22 18:19:38]

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

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/lifetime...
chrome/browser/lifetime/keep_alive_registry.h:20: class KeepAliveRegistry {
On 2016/02/19 18:04:28, dgn wrote:
> On 2016/02/19 17:39:28, Bernhard Bauer wrote:
> > You might be able to move this whole class into an anonymous namespace in
> > scoped_alive.cc. No one else should use this.
> 
> Can I do that at the end of the migration? I'm not sure what I will run into
> while updating other parts of chrome. 
> Also I will probably want to for testing.
> And registering observers, which is a legit usage.

Okay… Could you make the Register/Unregister methods private and friend
ScopedKeepAlive? Or at the very least, add a comment that these shouldn't be
used directly.

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

https://codereview.chromium.org/1708343002/diff/140001/chrome/browser/backgro...
chrome/browser/background/background_mode_manager.cc:88: static const
KeepAliveOptions g_keep_alive_options = {"Background"};
The static isn't necessary (outside of a class "static" means "internal
linkage", which constants automatically get, and in addition it's in an
anonymous namespace, which has the same effect).

I would also use kConstantNotation for this.

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

https://codereview.chromium.org/1708343002/diff/140001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_options.h:11: const char label[32];
I think const char* should work here; no need to artificially limit the size of
the label (and then have to worry about what would happen if someone were to use
a longer label...).

https://codereview.chromium.org/1708343002/diff/140001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_options.h:14: // When registering a token,
indictations of which kind of optimizations would
Nit: "indications".

Also, the passive voice is used a bit too much in this sentence. Maybe just,
"[...] we should add [...]"

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

https://codereview.chromium.org/1708343002/diff/140001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_registry.cc:33: LOG(ERROR) << "DGN -
Increment";
You'll remember to remove these before committing, right? 😃

https://codereview.chromium.org/1708343002/diff/140001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/app_list_service_views.cc (right):

https://codereview.chromium.org/1708343002/diff/140001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/app_list_service_views.cc:19: static const
KeepAliveOptions g_keep_alive_options = {"AppListServiceViews"};
Same as above. Also (as mentioned above), the anonymous namespace isn't really
necessary if you only have consts, so you could remove it (but OTOH, it doesn't
hurt either. I'm mostly mentioning this for completeness / education).

[dgn, 2016-02-22 19:07:19]

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

https://codereview.chromium.org/1708343002/diff/40001/chrome/browser/lifetime...
chrome/browser/lifetime/keep_alive_registry.h:20: class KeepAliveRegistry {
On 2016/02/22 18:19:38, Bernhard Bauer wrote:
> On 2016/02/19 18:04:28, dgn wrote:
> > On 2016/02/19 17:39:28, Bernhard Bauer wrote:
> > > You might be able to move this whole class into an anonymous namespace in
> > > scoped_alive.cc. No one else should use this.
> > 
> > Can I do that at the end of the migration? I'm not sure what I will run into
> > while updating other parts of chrome. 
> > Also I will probably want to for testing.
> > And registering observers, which is a legit usage.
> 
> Okay… Could you make the Register/Unregister methods private and friend
> ScopedKeepAlive? Or at the very least, add a comment that these shouldn't be
> used directly.

Done.

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

https://codereview.chromium.org/1708343002/diff/140001/chrome/browser/backgro...
chrome/browser/background/background_mode_manager.cc:88: static const
KeepAliveOptions g_keep_alive_options = {"Background"};
On 2016/02/22 18:19:38, Bernhard Bauer wrote:
> The static isn't necessary (outside of a class "static" means "internal
> linkage", which constants automatically get, and in addition it's in an
> anonymous namespace, which has the same effect).
> 
> I would also use kConstantNotation for this.

Thanks, I didn't know that about const. Done

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

https://codereview.chromium.org/1708343002/diff/140001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_options.h:11: const char label[32];
On 2016/02/22 18:19:38, Bernhard Bauer wrote:
> I think const char* should work here; no need to artificially limit the size
of
> the label (and then have to worry about what would happen if someone were to
use
> a longer label...).

Thanks, done. If it's longer the compiler would detect it and show an error.

https://codereview.chromium.org/1708343002/diff/140001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_options.h:14: // When registering a token,
indictations of which kind of optimizations would
On 2016/02/22 18:19:38, Bernhard Bauer wrote:
> Nit: "indications".
> 
> Also, the passive voice is used a bit too much in this sentence. Maybe just,
> "[...] we should add [...]"

Done.

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

https://codereview.chromium.org/1708343002/diff/140001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_registry.cc:33: LOG(ERROR) << "DGN -
Increment";
On 2016/02/22 18:19:38, Bernhard Bauer wrote:
> You'll remember to remove these before committing, right? 😃

Yup, removed now.

https://codereview.chromium.org/1708343002/diff/140001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/app_list_service_views.cc (right):

https://codereview.chromium.org/1708343002/diff/140001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/app_list_service_views.cc:19: static const
KeepAliveOptions g_keep_alive_options = {"AppListServiceViews"};
On 2016/02/22 18:19:38, Bernhard Bauer wrote:
> Same as above. Also (as mentioned above), the anonymous namespace isn't really
> necessary if you only have consts, so you could remove it (but OTOH, it
doesn't
> hurt either. I'm mostly mentioning this for completeness / education).

Thanks. Removed since there's nothing else in the anonymous namespace.

[sky, 2016-02-22 21:26:05]

https://codereview.chromium.org/1708343002/diff/160001/chrome/browser/lifetim...
File chrome/browser/lifetime/keep_alive_options.h (right):

https://codereview.chromium.org/1708343002/diff/160001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_options.h:11: const char* label;
Can you explain why a string is better than an enum here? An enum provides the
context and is much easier in the call sites.

[dgn, 2016-02-23 12:58:06]

https://codereview.chromium.org/1708343002/diff/160001/chrome/browser/lifetim...
File chrome/browser/lifetime/keep_alive_options.h (right):

https://codereview.chromium.org/1708343002/diff/160001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_options.h:11: const char* label;
On 2016/02/22 21:26:05, sky wrote:
> Can you explain why a string is better than an enum here? An enum provides the
> context and is much easier in the call sites.

The idea was that each call site would use a descriptive string, that would give
some information about the purpose of that KeepAlive. Those strings can be
displayed for debug purposes, and for example could be shown in an about://
page. 

Having one enum per call site would require maintaining a centralized list, and
have c/b/lifetime be aware of all the call sites, which I think would be
strange. Also, we would still have to add the string mappings that go with that
list for debugging.

From the Release point of view, enums have their advantages. I don't feel
strongly about that, I can change it if you want.

[sky, 2016-02-23 21:16:24]

Maintaining the centralized lists feels no different than centralizing
the switches. I prefer the enum, with an enum->string mapping if you
need it for debugging.

  -Scott

On Tue, Feb 23, 2016 at 4:58 AM,  <dgn@chromium.org> wrote:
>
>
https://codereview.chromium.org/1708343002/diff/160001/chrome/browser/lifetim...
> File chrome/browser/lifetime/keep_alive_options.h (right):
>
>
https://codereview.chromium.org/1708343002/diff/160001/chrome/browser/lifetim...
> chrome/browser/lifetime/keep_alive_options.h:11: const char* label;
> On 2016/02/22 21:26:05, sky wrote:
>> Can you explain why a string is better than an enum here? An enum
> provides the
>> context and is much easier in the call sites.
>
> The idea was that each call site would use a descriptive string, that
> would give some information about the purpose of that KeepAlive. Those
> strings can be displayed for debug purposes, and for example could be
> shown in an about:// page.
>
> Having one enum per call site would require maintaining a centralized
> list, and have c/b/lifetime be aware of all the call sites, which I
> think would be strange. Also, we would still have to add the string
> mappings that go with that list for debugging.
>
> From the Release point of view, enums have their advantages. I don't
> feel strongly about that, I can change it if you want.
>
> https://codereview.chromium.org/1708343002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dgn, 2016-02-23 23:24:31]

On 2016/02/23 21:16:24, sky wrote:
> Maintaining the centralized lists feels no different than centralizing
> the switches. I prefer the enum, with an enum->string mapping if you
> need it for debugging.
> 
>   -Scott

Sure, done. PTAL.

[sky, 2016-02-24 05:08:23]

https://codereview.chromium.org/1708343002/diff/220001/chrome/browser/lifetim...
File chrome/browser/lifetime/keep_alive_types.h (right):

https://codereview.chromium.org/1708343002/diff/220001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_types.h:14: kBackgroundModeManager,
enums are ALL_CAPS.

https://codereview.chromium.org/1708343002/diff/220001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_types.h:27: struct KeepAliveParams {
Why do you need the struct? If different arguments are needed in the future
change the signature of ScopedKeepAlive.

[dgn, 2016-02-24 11:24:04]

PTAL

https://codereview.chromium.org/1708343002/diff/220001/chrome/browser/lifetim...
File chrome/browser/lifetime/keep_alive_types.h (right):

https://codereview.chromium.org/1708343002/diff/220001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_types.h:10: namespace keep_alive {
I plan to add the optimization options there, e.g.
keep_alive::RestartOption::ENABLED

https://codereview.chromium.org/1708343002/diff/220001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_types.h:14: kBackgroundModeManager,
On 2016/02/24 05:08:22, sky wrote:
> enums are ALL_CAPS.

Thanks, I mixed it up with the Google Style Guide. Fixed.

https://codereview.chromium.org/1708343002/diff/220001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_types.h:27: struct KeepAliveParams {
On 2016/02/24 05:08:22, sky wrote:
> Why do you need the struct? If different arguments are needed in the future
> change the signature of ScopedKeepAlive.

Not needed, but I preferred the way structs allows to declare the configuration
and make it accessible. Changed it.

[sky, 2016-02-24 18:53:21]

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

https://codereview.chromium.org/1708343002/diff/120001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_registry.h:27: std::multiset<std::string>
registered_tokens_;
On 2016/02/22 18:01:20, dgn wrote:
> On 2016/02/19 21:02:49, sky wrote:
> > Is there a reason why you aren't using a map<string,int> ? I get that
multiset
> > works, but it's a bit esoteric for this use case.
> 
> With the int being the number of registered tokens with the same name? For the
> optimizations later, I plan to have another multiset for tokens allowing
> restart, and comparing the size of the 2 multisets to determine when restart
is
> safe. That would not work with maps and counts. I would have to maintain a
> separate counter or compute it everytime, or do something else to track state

Couldn't you just as well use two map<enum,int> and compare the sizes? Again, it
doesn't seem to me that you need a multiset here. If it makes sense in the
future, you can always change it, but for the time being a map<enum,int> is more
straightforward.

https://codereview.chromium.org/1708343002/diff/240001/chrome/browser/lifetim...
File chrome/browser/lifetime/keep_alive_types.h (right):

https://codereview.chromium.org/1708343002/diff/240001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_types.h:10: namespace keep_alive {
If you're going to use a namespace, then it should be based on path (lifetime)
and you should use it in all these files. I wouldn't bother with a namespace,
instead name the enum better, perhaps KeepAliveOrigin.

https://codereview.chromium.org/1708343002/diff/240001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_types.h:27: #ifndef NDEBUG
#if !defined(NDEBUG)

[dgn, 2016-02-24 19:55:45]

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

https://codereview.chromium.org/1708343002/diff/120001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_registry.h:27: std::multiset<std::string>
registered_tokens_;
On 2016/02/24 18:53:21, sky wrote:
> On 2016/02/22 18:01:20, dgn wrote:
> > On 2016/02/19 21:02:49, sky wrote:
> > > Is there a reason why you aren't using a map<string,int> ? I get that
> multiset
> > > works, but it's a bit esoteric for this use case.
> > 
> > With the int being the number of registered tokens with the same name? For
the
> > optimizations later, I plan to have another multiset for tokens allowing
> > restart, and comparing the size of the 2 multisets to determine when restart
> is
> > safe. That would not work with maps and counts. I would have to maintain a
> > separate counter or compute it everytime, or do something else to track
state
> 
> Couldn't you just as well use two map<enum,int> and compare the sizes? Again,
it
> doesn't seem to me that you need a multiset here. If it makes sense in the
> future, you can always change it, but for the time being a map<enum,int> is
more
> straightforward.

I think we had different assumptions. I started working on a CL with restart
here: https://codereview.chromium.org/1725883002/

I can have the case where the maps would look like:

 registered | restart_allowed
------------+-----------------
  Foo - 1   |   Foo - 1
  Bar - 2   |   Bar - 1


There are 3 keep_alive registered in one and 2 in the other. So the size doesn't
help and I need to sum the ints there.

For this I am assuming that since call site would have control of their options
and they can mix the different options the way they want regardless of the
KeepAliveOrigin value.

If we assume that one KeepAliveOrigin always has a certain set of options that
does not change, then yes, the maps works fine. But then I might as well
centralize all the options and the only thing the caller would ever see would be
the KeepAliveOrigin. I would prefer if that was expressed in the caller's file,
that's the kind of things that seem like they gain at being explicitly present,
for reading the code or reviews...

So in the case where we want to have fixed origin-options combinations, I could
go with maps and structs declared in keep_alive_types (or something like that),
but if we want to let callers register KeepAlives as will, we would go with
multisets and a registration by providing all the options separately?

Did I understand correctly?

[sky, 2016-02-24 20:01:26]

Why not a total count separate from the map then?

  -Scott

On Wed, Feb 24, 2016 at 11:55 AM,  <dgn@chromium.org> wrote:
>
>
https://codereview.chromium.org/1708343002/diff/120001/chrome/browser/lifetim...
> File chrome/browser/lifetime/keep_alive_registry.h (right):
>
>
https://codereview.chromium.org/1708343002/diff/120001/chrome/browser/lifetim...
> chrome/browser/lifetime/keep_alive_registry.h:27:
> std::multiset<std::string> registered_tokens_;
> On 2016/02/24 18:53:21, sky wrote:
>> On 2016/02/22 18:01:20, dgn wrote:
>> > On 2016/02/19 21:02:49, sky wrote:
>> > > Is there a reason why you aren't using a map<string,int> ? I get
> that
>> multiset
>> > > works, but it's a bit esoteric for this use case.
>> >
>> > With the int being the number of registered tokens with the same
> name? For the
>> > optimizations later, I plan to have another multiset for tokens
> allowing
>> > restart, and comparing the size of the 2 multisets to determine when
> restart
>> is
>> > safe. That would not work with maps and counts. I would have to
> maintain a
>> > separate counter or compute it everytime, or do something else to
> track state
>>
>> Couldn't you just as well use two map<enum,int> and compare the sizes?
> Again, it
>> doesn't seem to me that you need a multiset here. If it makes sense in
> the
>> future, you can always change it, but for the time being a
> map<enum,int> is more
>> straightforward.
>
> I think we had different assumptions. I started working on a CL with
> restart here: https://codereview.chromium.org/1725883002/
>
> I can have the case where the maps would look like:
>
> registered | restart_allowed
> ------------+-----------------
> Foo - 1 | Foo - 1
> Bar - 2 | Bar - 1
>
>
> There are 3 keep_alive registered in one and 2 in the other. So the size
> doesn't help and I need to sum the ints there.
>
> For this I am assuming that since call site would have control of their
> options and they can mix the different options the way they want
> regardless of the KeepAliveOrigin value.
>
> If we assume that one KeepAliveOrigin always has a certain set of
> options that does not change, then yes, the maps works fine. But then I
> might as well centralize all the options and the only thing the caller
> would ever see would be the KeepAliveOrigin. I would prefer if that was
> expressed in the caller's file, that's the kind of things that seem like
> they gain at being explicitly present, for reading the code or
> reviews...
>
> So in the case where we want to have fixed origin-options combinations,
> I could go with maps and structs declared in keep_alive_types (or
> something like that), but if we want to let callers register KeepAlives
> as will, we would go with multisets and a registration by providing all
> the options separately?
>
> Did I understand correctly?
>
> https://codereview.chromium.org/1708343002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dgn, 2016-02-24 20:14:05]

On 2016/02/24 20:01:26, sky wrote:
> Why not a total count separate from the map then?
> 
>   -Scott
> 
>
https://codereview.chromium.org/1708343002/diff/120001/chrome/browser/lifetim...
> > chrome/browser/lifetime/keep_alive_registry.h:27:
> > std::multiset<std::string> registered_tokens_;
> > On 2016/02/24 18:53:21, sky wrote:
> >> On 2016/02/22 18:01:20, dgn wrote:
> >> > On 2016/02/19 21:02:49, sky wrote:
> >> > > Is there a reason why you aren't using a map<string,int> ? I get
> > that
> >> multiset
> >> > > works, but it's a bit esoteric for this use case.
> >> >
> >> > With the int being the number of registered tokens with the same
> > name? For the
> >> > optimizations later, I plan to have another multiset for tokens
> > allowing
> >> > restart, and comparing the size of the 2 multisets to determine when
> > restart
> >> is
> >> > safe. That would not work with maps and counts. I would have to
> > maintain a
> >> > separate counter or compute it everytime, or do something else to
> > track state
> >>
> >> Couldn't you just as well use two map<enum,int> and compare the sizes?
> > Again, it
> >> doesn't seem to me that you need a multiset here. If it makes sense in
> > the
> >> future, you can always change it, but for the time being a
> > map<enum,int> is more
> >> straightforward.
> >
> > I think we had different assumptions. I started working on a CL with
> > restart here: https://codereview.chromium.org/1725883002/
> >
> > I can have the case where the maps would look like:
> >
> > registered | restart_allowed
> > ------------+-----------------
> > Foo - 1 | Foo - 1
> > Bar - 2 | Bar - 1
> >
> >
> > There are 3 keep_alive registered in one and 2 in the other. So the size
> > doesn't help and I need to sum the ints there.
> >
> > For this I am assuming that since call site would have control of their
> > options and they can mix the different options the way they want
> > regardless of the KeepAliveOrigin value.
> >
> > If we assume that one KeepAliveOrigin always has a certain set of
> > options that does not change, then yes, the maps works fine. But then I
> > might as well centralize all the options and the only thing the caller
> > would ever see would be the KeepAliveOrigin. I would prefer if that was
> > expressed in the caller's file, that's the kind of things that seem like
> > they gain at being explicitly present, for reading the code or
> > reviews...
> >
> > So in the case where we want to have fixed origin-options combinations,
> > I could go with maps and structs declared in keep_alive_types (or
> > something like that), but if we want to let callers register KeepAlives
> > as will, we would go with multisets and a registration by providing all
> > the options separately?
> >
> > Did I understand correctly?


For debugging purposes, we would lose the information about what registered with
which option. But okay, I can do that.

[dgn, 2016-02-24 21:04:18]

PTAL

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

https://codereview.chromium.org/1708343002/diff/120001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_registry.h:27: std::multiset<std::string>
registered_tokens_;
On 2016/02/24 19:55:45, dgn wrote:
> On 2016/02/24 18:53:21, sky wrote:
> > On 2016/02/22 18:01:20, dgn wrote:
> > > On 2016/02/19 21:02:49, sky wrote:
> > > > Is there a reason why you aren't using a map<string,int> ? I get that
> > multiset
> > > > works, but it's a bit esoteric for this use case.
> > > 
> > > With the int being the number of registered tokens with the same name? For
> the
> > > optimizations later, I plan to have another multiset for tokens allowing
> > > restart, and comparing the size of the 2 multisets to determine when
restart
> > is
> > > safe. That would not work with maps and counts. I would have to maintain a
> > > separate counter or compute it everytime, or do something else to track
> state
> > 
> > Couldn't you just as well use two map<enum,int> and compare the sizes?
Again,
> it
> > doesn't seem to me that you need a multiset here. If it makes sense in the
> > future, you can always change it, but for the time being a map<enum,int> is
> more
> > straightforward.
> 
> I think we had different assumptions. I started working on a CL with restart
> here: https://codereview.chromium.org/1725883002/
> 
> I can have the case where the maps would look like:
> 
>  registered | restart_allowed
> ------------+-----------------
>   Foo - 1   |   Foo - 1
>   Bar - 2   |   Bar - 1
> 
> 
> There are 3 keep_alive registered in one and 2 in the other. So the size
doesn't
> help and I need to sum the ints there.
> 
> For this I am assuming that since call site would have control of their
options
> and they can mix the different options the way they want regardless of the
> KeepAliveOrigin value.
> 
> If we assume that one KeepAliveOrigin always has a certain set of options that
> does not change, then yes, the maps works fine. But then I might as well
> centralize all the options and the only thing the caller would ever see would
be
> the KeepAliveOrigin. I would prefer if that was expressed in the caller's
file,
> that's the kind of things that seem like they gain at being explicitly
present,
> for reading the code or reviews...
> 
> So in the case where we want to have fixed origin-options combinations, I
could
> go with maps and structs declared in keep_alive_types (or something like
that),
> but if we want to let callers register KeepAlives as will, we would go with
> multisets and a registration by providing all the options separately?
> 
> Did I understand correctly?

Done.

https://codereview.chromium.org/1708343002/diff/240001/chrome/browser/lifetim...
File chrome/browser/lifetime/keep_alive_types.h (right):

https://codereview.chromium.org/1708343002/diff/240001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_types.h:10: namespace keep_alive {
On 2016/02/24 18:53:21, sky wrote:
> If you're going to use a namespace, then it should be based on path (lifetime)
> and you should use it in all these files. I wouldn't bother with a namespace,
> instead name the enum better, perhaps KeepAliveOrigin.

Done.

https://codereview.chromium.org/1708343002/diff/240001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_types.h:27: #ifndef NDEBUG
On 2016/02/24 18:53:21, sky wrote:
> #if !defined(NDEBUG)

I followed the ruling from here:
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/PD8M_BJ6AAc...

Codesearch: 
"#ifndef NDEBUG" -> 721 hits
"#if !defined(NDEBUG)" -> 227 hits

Did the convention change recently?

[sky, 2016-02-25 00:25:18]

https://codereview.chromium.org/1708343002/diff/240001/chrome/browser/lifetim...
File chrome/browser/lifetime/keep_alive_types.h (right):

https://codereview.chromium.org/1708343002/diff/240001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_types.h:27: #ifndef NDEBUG
On 2016/02/24 21:04:18, dgn wrote:
> On 2016/02/24 18:53:21, sky wrote:
> > #if !defined(NDEBUG)
> 
> I followed the ruling from here:
>
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/PD8M_BJ6AAc...
> 
> Codesearch: 
> "#ifndef NDEBUG" -> 721 hits
> "#if !defined(NDEBUG)" -> 227 hits
> 
> Did the convention change recently?

GAH! Ok, ignore me.

https://codereview.chromium.org/1708343002/diff/280001/chrome/browser/lifetim...
File chrome/browser/lifetime/keep_alive_registry.cc (right):

https://codereview.chromium.org/1708343002/diff/280001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_registry.cc:55:
--registered_keep_alives_[origin];
remove from rendered_keep_alives if count == 0

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

https://codereview.chromium.org/1708343002/diff/280001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_registry.h:27: virtual void HugBrowser();
'hug', eh? How about PinBrowser(), PreventBrowserFromClosing().

https://codereview.chromium.org/1708343002/diff/280001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_registry.h:30: virtual void ReleaseBrowser();
I'm not a fan of virtual functions like this just for testing (especially when
that testing involves gmock).
Seems like your tests can be covered by calling WillKeepAlive(). Do you really
need the virtuals here?

https://codereview.chromium.org/1708343002/diff/280001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_registry.h:51: int registered_count_;
You need to initialize this to 0.

[dgn, 2016-02-25 17:19:46]

PTAL

https://codereview.chromium.org/1708343002/diff/280001/chrome/browser/lifetim...
File chrome/browser/lifetime/keep_alive_registry.cc (right):

https://codereview.chromium.org/1708343002/diff/280001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_registry.cc:55:
--registered_keep_alives_[origin];
On 2016/02/25 00:25:18, sky wrote:
> remove from rendered_keep_alives if count == 0

Done.

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

https://codereview.chromium.org/1708343002/diff/280001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_registry.h:27: virtual void HugBrowser();
On 2016/02/25 00:25:18, sky wrote:
> 'hug', eh? How about PinBrowser(), PreventBrowserFromClosing().

Removed, since I'm not mocking the method anymore.

https://codereview.chromium.org/1708343002/diff/280001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_registry.h:30: virtual void ReleaseBrowser();
On 2016/02/25 00:25:18, sky wrote:
> I'm not a fan of virtual functions like this just for testing (especially when
> that testing involves gmock).
> Seems like your tests can be covered by calling WillKeepAlive(). Do you really
> need the virtuals here?

I wanted to check the call that has external effects and not only checking that
internal state with WillKeepAlive. So now I just exposed the keep alive count
from application_lifetime. I'll update the test when I get rid of the count
there.

https://codereview.chromium.org/1708343002/diff/280001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_registry.h:51: int registered_count_;
On 2016/02/25 00:25:18, sky wrote:
> You need to initialize this to 0.

Thanks. Done.

[sky, 2016-02-25 18:43:50]

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/lifetim...
File chrome/browser/lifetime/keep_alive_registry.cc (right):

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_registry.cc:36: auto count_per_origin_it =
registered_keep_alives_.find(origin);
Is there a reason you converted away from the simpler
registered_keep_alives_[origin]++ ?

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/lifetim...
File chrome/browser/lifetime/keep_alive_registry_unittest.cc (right):

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_registry_unittest.cc:13: class
KeepAliveRegistryTest : public testing::Test {
nit: remove this and instead using the TEST() macro.

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/lifetim...
File chrome/browser/lifetime/keep_alive_types.h (right):

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_types.h:12: TEST,
Is there a reason tests can't use one of the values below?

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/app_list_shower_views.cc (right):

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/app_list_shower_views.cc:33: keep_alive_.reset(new
ScopedKeepAlive(KeepAliveOrigin::APP_LIST_SHOWER));
Ugh! What a name (I get that you're matching the class name, which is equally
bizarre).

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/ui/view...
File chrome/browser/ui/views/panels/panel_view.cc (right):

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/ui/view...
chrome/browser/ui/views/panels/panel_view.cc:304: #if !defined(OS_CHROMEOS)
I think want !USE_ASH here.

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/user_manager_view.cc (right):

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:238: keep_alive_.reset(new
ScopedKeepAlive(KeepAliveOrigin::USER_MANAGER_VIEW));
I think you want !USE_ASH here too. The reason is that AutoKeepAlive(NULL) only
did something if not on ash.

[dgn, 2016-02-25 20:08:50]

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/lifetim...
File chrome/browser/lifetime/keep_alive_registry.cc (right):

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_registry.cc:36: auto count_per_origin_it =
registered_keep_alives_.find(origin);
On 2016/02/25 18:43:50, sky wrote:
> Is there a reason you converted away from the simpler
> registered_keep_alives_[origin]++ ?

I wanted to be sure about the initial value in the map. I searched around and
did not find much detailing the expected behavior. Just searched again and found
http://stackoverflow.com/a/9025981 which explains that the standard specifies it
should be 0 though. I'll change it back.

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/lifetim...
File chrome/browser/lifetime/keep_alive_types.h (right):

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_types.h:12: TEST,
On 2016/02/25 18:43:50, sky wrote:
> Is there a reason tests can't use one of the values below?

No...

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/app_list_shower_views.cc (right):

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/app_list_shower_views.cc:33: keep_alive_.reset(new
ScopedKeepAlive(KeepAliveOrigin::APP_LIST_SHOWER));
On 2016/02/25 18:43:50, sky wrote:
> Ugh! What a name (I get that you're matching the class name, which is equally
> bizarre).

Haha, I also had a double take while writing that.

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/ui/view...
File chrome/browser/ui/views/panels/panel_view.cc (right):

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/ui/view...
chrome/browser/ui/views/panels/panel_view.cc:304: #if !defined(OS_CHROMEOS)
On 2016/02/25 18:43:50, sky wrote:
> I think want !USE_ASH here.

Isn't there[1] where the count is incremented to balance the call being skipped
here?

It's guarded by OS_CHROMEOS

Another place it could have been done is [2], but it's guarded by !AURA so I'm
quite confused there...


[1]:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
[2]:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[sky, 2016-02-25 23:27:49]

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/ui/view...
File chrome/browser/ui/views/panels/panel_view.cc (right):

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/ui/view...
chrome/browser/ui/views/panels/panel_view.cc:304: #if !defined(OS_CHROMEOS)
On 2016/02/25 20:08:50, dgn wrote:
> On 2016/02/25 18:43:50, sky wrote:
> > I think want !USE_ASH here.
> 
> Isn't there[1] where the count is incremented to balance the call being
skipped
> here?
> 
> It's guarded by OS_CHROMEOS
> 
> Another place it could have been done is [2], but it's guarded by !AURA so I'm
> quite confused there...
> 
> 
> [1]:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

I think that increment should be outside the ifdef. It made sense at one time,
but not any more.

> [2]:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

I'm not sure on that one. If that code is only compiled on
windows/linux/chromeos, then USE_ASH is what you want. If that code is compiled
on mac, then it should be USE_ASH or MAC.

[dgn, 2016-02-26 11:51:59]

PTAL

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/lifetim...
File chrome/browser/lifetime/keep_alive_registry.cc (right):

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_registry.cc:36: auto count_per_origin_it =
registered_keep_alives_.find(origin);
On 2016/02/25 20:08:49, dgn wrote:
> On 2016/02/25 18:43:50, sky wrote:
> > Is there a reason you converted away from the simpler
> > registered_keep_alives_[origin]++ ?
> 
> I wanted to be sure about the initial value in the map. I searched around and
> did not find much detailing the expected behavior. Just searched again and
found
> http://stackoverflow.com/a/9025981 which explains that the standard specifies
it
> should be 0 though. I'll change it back.

Done.

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/lifetim...
File chrome/browser/lifetime/keep_alive_registry_unittest.cc (right):

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_registry_unittest.cc:13: class
KeepAliveRegistryTest : public testing::Test {
On 2016/02/25 18:43:50, sky wrote:
> nit: remove this and instead using the TEST() macro.

Done.

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/ui/view...
File chrome/browser/ui/views/panels/panel_view.cc (right):

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/ui/view...
chrome/browser/ui/views/panels/panel_view.cc:304: #if !defined(OS_CHROMEOS)
On 2016/02/25 23:27:49, sky wrote:
> On 2016/02/25 20:08:50, dgn wrote:
> > On 2016/02/25 18:43:50, sky wrote:
> > > I think want !USE_ASH here.
> > 
> > Isn't there[1] where the count is incremented to balance the call being
> skipped
> > here?
> > 
> > It's guarded by OS_CHROMEOS
> > 
> > Another place it could have been done is [2], but it's guarded by !AURA so
I'm
> > quite confused there...
> > 
> > 
> > [1]:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> 
> I think that increment should be outside the ifdef. It made sense at one time,
> but not any more.
> 

Thanks, I'll keep that in mind when changing that file.

> > [2]:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> 
> I'm not sure on that one. If that code is only compiled on
> windows/linux/chromeos, then USE_ASH is what you want. If that code is
compiled
> on mac, then it should be USE_ASH or MAC.


Okay. Changing that is out of scope if this CL anyway. I filed
https://crbug.com/590173 to track that. I'll just close it if it turns out to be
fine as it is.

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/ui/view...
chrome/browser/ui/views/panels/panel_view.cc:304: #if !defined(OS_CHROMEOS)
On 2016/02/25 18:43:50, sky wrote:
> I think want !USE_ASH here.

Done.

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/user_manager_view.cc (right):

https://codereview.chromium.org/1708343002/diff/300001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:238: keep_alive_.reset(new
ScopedKeepAlive(KeepAliveOrigin::USER_MANAGER_VIEW));
On 2016/02/25 18:43:50, sky wrote:
> I think you want !USE_ASH here too. The reason is that AutoKeepAlive(NULL)
only
> did something if not on ash.

Done.

[sky, 2016-02-26 15:59:39]

LGTM

[dgn, 2016-02-26 16:02:07]

On 2016/02/26 15:59:39, sky wrote:
> LGTM

Woohoo! Thanks for the prompt reviews!

[dgn, 2016-02-26 16:02:23]

The CQ bit was checked by dgn@chromium.org

[commit-bot: I haz the power, 2016-02-26 16:02:51]

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

[commit-bot: I haz the power, 2016-02-26 17:36:53]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2016-02-26 17:38:57]

Description was changed from

==========
Add ScopedKeepAlive to c/b/lifetime

It moves from c/b/apps as it can be reused more generally across
//chrome/browser. It works with the new KeepAliveRegistry which is
the class keeping all the relevant information in an accessible place.

This patch also reuses ScopedKeepAlive instead of
c/b/ui/views/AutoKeepAlive

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

to

==========
Add ScopedKeepAlive to c/b/lifetime

It moves from c/b/apps as it can be reused more generally across
//chrome/browser. It works with the new KeepAliveRegistry which is
the class keeping all the relevant information in an accessible place.

This patch also reuses ScopedKeepAlive instead of
c/b/ui/views/AutoKeepAlive

BUG=587926

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

[commit-bot: I haz the power, 2016-02-26 17:38:59]

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