Remove Increment/DecrementKeepAliveCount from AppsClient

Remove Increment/DecrementKeepAliveCount from AppsClient

Only chrome is interested in implementing these methods.
Move the code to ChromeAppDelegate.

BUG=411352
Committed: https://crrev.com/f9f37147210c1abf1269dc97b7b0bdef4bf28a5f
Cr-Commit-Position: refs/heads/master@{#296625}

[hashimoto, 2014-09-22 09:53:03]

Patchset #1 (id:1) has been deleted

[hashimoto, 2014-09-22 09:57:10]

hashimoto@chromium.org changed reviewers:
+ tapted@chromium.org

[hashimoto, 2014-09-22 09:57:11]

This change may affect the behavior of AppListViewDelegate which uses
ChromeAppDelegate.
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...)
Is it OK to increment the keep-alive count for that case too?

[tapted, 2014-09-22 10:36:37]

+matt CC in case you have an opinion about the below

On 2014/09/22 09:57:11, hashimoto wrote:
> This change may affect the behavior of AppListViewDelegate which uses
> ChromeAppDelegate.
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...)
> Is it OK to increment the keep-alive count for that case too?

Thanks for raising this - unfortunately we can't increment the keepalive there.
I'm pretty sure it wouldn't be decremented until a call in response to
chrome::NOTIFICATION_APP_TERMINATING (which would need all the keepalives gone
in the first place)
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

A constructor argument probably makes the most sense. Other options could be a
function call that clears a keepalive implicitly created in the constructor, or
a separate subclass of AppDelegate for creating CustomLauncherPageContents. Or
something else. (matt - thoughts?)

If you go with one of the first two options, it's probably worth moving
chrome/browser/ui/app_list/scoped_keep_alive.h to chrome/browser/apps and using
that.

(but it looks like a nice change otherwise!)

[Matt Giuca, 2014-09-23 05:28:27]

> A constructor argument probably makes the most sense.

I agree this sounds like the best approach. Add a Boolean argument to the
constructor that determines whether to do the keepalive, and set it to false for
custom launcher pages.

Thanks!

[hashimoto, 2014-09-24 06:17:36]

Patchset #2 (id:40001) has been deleted

[hashimoto, 2014-09-24 06:34:17]

Changed ChromeAppDelegate's ctor to take scoped_ptr<ScopedKeepAlive> as an
argument.
PTAL

[tapted, 2014-09-24 07:12:41]

nice - lgtm with a few minor changes.

https://codereview.chromium.org/557833007/diff/60001/chrome/browser/apps/app_...
File chrome/browser/apps/app_browsertest_util.cc (right):

https://codereview.chromium.org/557833007/diff/60001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest_util.cc:206: new
ChromeAppDelegate(scoped_ptr<ScopedKeepAlive>()),
to maintain behaviour in tests, I think this should pass a new ScopedKeepAlive.
I think they typically have a browser window open the whole time, which holds
the keepalive. However, there will be more realistic coverage with the keepalive
on the app window as well.

https://codereview.chromium.org/557833007/diff/60001/chrome/browser/apps/scop...
File chrome/browser/apps/scoped_keep_alive.h (right):

https://codereview.chromium.org/557833007/diff/60001/chrome/browser/apps/scop...
chrome/browser/apps/scoped_keep_alive.h:13: explicit ScopedKeepAlive();
nit: since you're here.. we can probably drop this stray `explicit`.

https://codereview.chromium.org/557833007/diff/60001/chrome/browser/ui/apps/c...
File chrome/browser/ui/apps/chrome_app_delegate.cc (right):

https://codereview.chromium.org/557833007/diff/60001/chrome/browser/ui/apps/c...
chrome/browser/ui/apps/chrome_app_delegate.cc:15: #include
"chrome/browser/lifetime/application_lifetime.h"
nit: this include no longer needed

https://codereview.chromium.org/557833007/diff/60001/chrome/browser/ui/apps/c...
File chrome/browser/ui/apps/chrome_app_window_client.cc (right):

https://codereview.chromium.org/557833007/diff/60001/chrome/browser/ui/apps/c...
chrome/browser/ui/apps/chrome_app_window_client.cc:49: context, new
ChromeAppDelegate(make_scoped_ptr(new ScopedKeepAlive)),
nit: 1 arg per line

https://codereview.chromium.org/557833007/diff/60001/chrome/browser/ui/ash/la...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
(right):

https://codereview.chromium.org/557833007/diff/60001/chrome/browser/ui/ash/la...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:747:
profile, new ChromeAppDelegate(scoped_ptr<ScopedKeepAlive>()),
this should pass a KeepAlive too (also nit: 1 arg per line)

[hashimoto, 2014-09-24 09:03:46]

https://codereview.chromium.org/557833007/diff/60001/chrome/browser/apps/app_...
File chrome/browser/apps/app_browsertest_util.cc (right):

https://codereview.chromium.org/557833007/diff/60001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest_util.cc:206: new
ChromeAppDelegate(scoped_ptr<ScopedKeepAlive>()),
On 2014/09/24 07:12:41, tapted wrote:
> to maintain behaviour in tests, I think this should pass a new
ScopedKeepAlive.
> I think they typically have a browser window open the whole time, which holds
> the keepalive. However, there will be more realistic coverage with the
keepalive
> on the app window as well.

Done.

https://codereview.chromium.org/557833007/diff/60001/chrome/browser/apps/scop...
File chrome/browser/apps/scoped_keep_alive.h (right):

https://codereview.chromium.org/557833007/diff/60001/chrome/browser/apps/scop...
chrome/browser/apps/scoped_keep_alive.h:13: explicit ScopedKeepAlive();
On 2014/09/24 07:12:41, tapted wrote:
> nit: since you're here.. we can probably drop this stray `explicit`.

Good catch.
Done.

https://codereview.chromium.org/557833007/diff/60001/chrome/browser/ui/apps/c...
File chrome/browser/ui/apps/chrome_app_delegate.cc (right):

https://codereview.chromium.org/557833007/diff/60001/chrome/browser/ui/apps/c...
chrome/browser/ui/apps/chrome_app_delegate.cc:15: #include
"chrome/browser/lifetime/application_lifetime.h"
On 2014/09/24 07:12:41, tapted wrote:
> nit: this include no longer needed

Done.

https://codereview.chromium.org/557833007/diff/60001/chrome/browser/ui/apps/c...
File chrome/browser/ui/apps/chrome_app_window_client.cc (right):

https://codereview.chromium.org/557833007/diff/60001/chrome/browser/ui/apps/c...
chrome/browser/ui/apps/chrome_app_window_client.cc:49: context, new
ChromeAppDelegate(make_scoped_ptr(new ScopedKeepAlive)),
On 2014/09/24 07:12:41, tapted wrote:
> nit: 1 arg per line

Done.

https://codereview.chromium.org/557833007/diff/60001/chrome/browser/ui/ash/la...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
(right):

https://codereview.chromium.org/557833007/diff/60001/chrome/browser/ui/ash/la...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:747:
profile, new ChromeAppDelegate(scoped_ptr<ScopedKeepAlive>()),
On 2014/09/24 07:12:41, tapted wrote:
> this should pass a KeepAlive too (also nit: 1 arg per line)

Done.

[hashimoto, 2014-09-24 09:05:24]

hashimoto@chromium.org changed reviewers:
+ derat@chromium.org, oshima@chromium.org, rockot@chromium.org

[hashimoto, 2014-09-24 09:05:25]

rockot@: could you review this change as an owner of chrome/browser/extensions
and extensions?
derat@: could you review this change as an owner of chrome/browser/power?
oshima@: could you review this change as an owner of athena and
chrome/browser/ui/ash?

[Daniel Erat, 2014-09-24 14:39:06]

lgtm for chrome/browser/power

[oshima, 2014-09-24 17:02:18]

athena lgtm

[Ken Rockot(use gerrit already), 2014-09-24 17:04:50]

lgtm

[hashimoto, 2014-09-25 02:03:31]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-09-25 02:04:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/557833007/80001

[commit-bot: I haz the power, 2014-09-25 02:10:44]

Committed patchset #3 (id:80001) as a03756b1d0e0f2a46f1026c3768cb5b93f6f8b3e

[commit-bot: I haz the power, 2014-09-25 02:11:44]

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