Hidden windows should not keep Chrome alive.

Hidden windows should not keep Chrome alive.

For each app window, there is a ChromeAppDelegate which
holds a ScopedKeepAlive. This CL resets the ScopedKeepAlive
while the app window is hidden.

BUG=450710
Committed: https://crrev.com/39d75d5b5f11525e384745fd313d1d77ed7d0a88
Cr-Commit-Position: refs/heads/master@{#316413}

[jackhou1, 2015-01-27 23:50:11]

jackhou@chromium.org changed reviewers:
+ benwells@chromium.org

[jackhou1, 2015-01-27 23:50:12]

benwells, what do you think of this?

Are there any cases where a hidden window should keep Chrome alive?

[benwells, 2015-01-28 02:42:10]

Can we have a test for this?

https://codereview.chromium.org/875273003/diff/1/chrome/browser/ui/apps/chrom...
File chrome/browser/ui/apps/chrome_app_delegate.cc (right):

https://codereview.chromium.org/875273003/diff/1/chrome/browser/ui/apps/chrom...
chrome/browser/ui/apps/chrome_app_delegate.cc:308: keep_alive_.reset();
If an app window is created hidden does it get OnHide?

https://codereview.chromium.org/875273003/diff/1/chrome/browser/ui/apps/chrom...
chrome/browser/ui/apps/chrome_app_delegate.cc:310: void
ChromeAppDelegate::OnShow() {
Nit: Blank line above.

[jackhou1, 2015-02-03 06:05:13]

Added test.

I realized with this change, an app that starts out hidden and shows after some
delay might not get a chance to show because Chrome has quit. This is likely to
happen if the app is launched from a shortcut while Chrome is not running. 

Is that acceptable behavior?

If not, how can we accommodate the above case? Maybe a timeout after hiding
before releasing the keep-alive?

https://codereview.chromium.org/875273003/diff/1/chrome/browser/ui/apps/chrom...
File chrome/browser/ui/apps/chrome_app_delegate.cc (right):

https://codereview.chromium.org/875273003/diff/1/chrome/browser/ui/apps/chrom...
chrome/browser/ui/apps/chrome_app_delegate.cc:308: keep_alive_.reset();
On 2015/01/28 02:42:10, benwells wrote:
> If an app window is created hidden does it get OnHide?

Yup, AppWindow::Init calls Hide() if the window is initially hidden.

https://codereview.chromium.org/875273003/diff/1/chrome/browser/ui/apps/chrom...
chrome/browser/ui/apps/chrome_app_delegate.cc:310: void
ChromeAppDelegate::OnShow() {
On 2015/01/28 02:42:10, benwells wrote:
> Nit: Blank line above.

Done.

[benwells, 2015-02-03 06:48:38]

On 2015/02/03 06:05:13, jackhou1 wrote:
> Added test.
> 
> I realized with this change, an app that starts out hidden and shows after
some
> delay might not get a chance to show because Chrome has quit. This is likely
to
> happen if the app is launched from a shortcut while Chrome is not running. 
> 
> Is that acceptable behavior?

Erg. Yeah I think that will probably break apps that are started that way. Have
you verified it is a problem? 

> 
> If not, how can we accommodate the above case? Maybe a timeout after hiding
> before releasing the keep-alive?

I can't think of anything else :\

> 
>
https://codereview.chromium.org/875273003/diff/1/chrome/browser/ui/apps/chrom...
> File chrome/browser/ui/apps/chrome_app_delegate.cc (right):
> 
>
https://codereview.chromium.org/875273003/diff/1/chrome/browser/ui/apps/chrom...
> chrome/browser/ui/apps/chrome_app_delegate.cc:308: keep_alive_.reset();
> On 2015/01/28 02:42:10, benwells wrote:
> > If an app window is created hidden does it get OnHide?
> 
> Yup, AppWindow::Init calls Hide() if the window is initially hidden.
> 
>
https://codereview.chromium.org/875273003/diff/1/chrome/browser/ui/apps/chrom...
> chrome/browser/ui/apps/chrome_app_delegate.cc:310: void
> ChromeAppDelegate::OnShow() {
> On 2015/01/28 02:42:10, benwells wrote:
> > Nit: Blank line above.
> 
> Done.

[jackhou1, 2015-02-09 04:52:11]

Okay, I think Patch Set 3 is a reasonably neat way to implement the timeout
behavior.

It will only keep alive with a timeout if the window is hidden but has never
been shown. Once the window is shown, hiding immediately releases the
keep-alive.

[benwells, 2015-02-10 07:25:10]

lgtm

https://codereview.chromium.org/875273003/diff/40001/chrome/browser/apps/app_...
File chrome/browser/apps/app_window_interactive_uitest.cc (right):

https://codereview.chromium.org/875273003/diff/40001/chrome/browser/apps/app_...
chrome/browser/apps/app_window_interactive_uitest.cc:485: // An window that is
created hidden and stays hidden should not keep Chrome
Nit: A window...

https://codereview.chromium.org/875273003/diff/40001/chrome/browser/apps/app_...
chrome/browser/apps/app_window_interactive_uitest.cc:500:
IN_PROC_BROWSER_TEST_F(AppWindowHiddenKeepAliveTest, HiddenThenShown) {
Have you verified this test fails if the keepalive isn't added after a window
goes from hidden to shown?

[jackhou1, 2015-02-10 23:32:40]

New patchsets have been uploaded after l-g-t-m from benwells@chromium.org

[jackhou1, 2015-02-10 23:33:21]

PTAL

Updated some tests to use chrome::WillKeepAlive.

https://codereview.chromium.org/875273003/diff/40001/chrome/browser/apps/app_...
File chrome/browser/apps/app_window_interactive_uitest.cc (right):

https://codereview.chromium.org/875273003/diff/40001/chrome/browser/apps/app_...
chrome/browser/apps/app_window_interactive_uitest.cc:485: // An window that is
created hidden and stays hidden should not keep Chrome
On 2015/02/10 07:25:10, benwells wrote:
> Nit: A window...

Done.

https://codereview.chromium.org/875273003/diff/40001/chrome/browser/apps/app_...
chrome/browser/apps/app_window_interactive_uitest.cc:500:
IN_PROC_BROWSER_TEST_F(AppWindowHiddenKeepAliveTest, HiddenThenShown) {
On 2015/02/10 07:25:10, benwells wrote:
> Have you verified this test fails if the keepalive isn't added after a window
> goes from hidden to shown?

In this case, the ChromeAppDelegate just avoids removing the keep-alive if the
window has never been shown. Added a new test to cover the case you suggested.

[benwells, 2015-02-11 04:37:10]

On 2015/02/10 23:33:21, jackhou1 wrote:
> PTAL
> 
> Updated some tests to use chrome::WillKeepAlive.
> 
>
https://codereview.chromium.org/875273003/diff/40001/chrome/browser/apps/app_...
> File chrome/browser/apps/app_window_interactive_uitest.cc (right):
> 
>
https://codereview.chromium.org/875273003/diff/40001/chrome/browser/apps/app_...
> chrome/browser/apps/app_window_interactive_uitest.cc:485: // An window that is
> created hidden and stays hidden should not keep Chrome
> On 2015/02/10 07:25:10, benwells wrote:
> > Nit: A window...
> 
> Done.
> 
>
https://codereview.chromium.org/875273003/diff/40001/chrome/browser/apps/app_...
> chrome/browser/apps/app_window_interactive_uitest.cc:500:
> IN_PROC_BROWSER_TEST_F(AppWindowHiddenKeepAliveTest, HiddenThenShown) {
> On 2015/02/10 07:25:10, benwells wrote:
> > Have you verified this test fails if the keepalive isn't added after a
window
> > goes from hidden to shown?
> 
> In this case, the ChromeAppDelegate just avoids removing the keep-alive if the
> window has never been shown. Added a new test to cover the case you suggested.

Latest patch set has issues in the new tests...

[jackhou1, 2015-02-11 22:34:16]

PTAL

[benwells, 2015-02-12 22:34:44]

On 2015/02/11 22:34:16, jackhou1 wrote:
> PTAL

lgtm once the asan issue is fixed.

[benwells, 2015-02-12 22:55:08]

(forgot this nit)

https://codereview.chromium.org/875273003/diff/40001/chrome/browser/apps/app_...
File chrome/browser/apps/app_window_interactive_uitest.cc (right):

https://codereview.chromium.org/875273003/diff/40001/chrome/browser/apps/app_...
chrome/browser/apps/app_window_interactive_uitest.cc:485: // An window that is
created hidden and stays hidden should not keep Chrome
On 2015/02/10 23:33:20, jackhou1 wrote:
> On 2015/02/10 07:25:10, benwells wrote:
> > Nit: A window...
> 
> Done.

Nit: no it's not...

[jackhou1, 2015-02-13 05:46:31]

New patchsets have been uploaded after l-g-t-m from benwells@chromium.org

[jackhou1, 2015-02-15 23:00:27]

Okay, asan actually fixed now.

https://codereview.chromium.org/875273003/diff/40001/chrome/browser/apps/app_...
File chrome/browser/apps/app_window_interactive_uitest.cc (right):

https://codereview.chromium.org/875273003/diff/40001/chrome/browser/apps/app_...
chrome/browser/apps/app_window_interactive_uitest.cc:485: // An window that is
created hidden and stays hidden should not keep Chrome
On 2015/02/12 22:55:08, benwells wrote:
> On 2015/02/10 23:33:20, jackhou1 wrote:
> > On 2015/02/10 07:25:10, benwells wrote:
> > > Nit: A window...
> > 
> > Done.
> 
> Nit: no it's not...

Oops, I fixed the one below and not this one. Done now.

[jackhou1, 2015-02-15 23:00:33]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2015-02-15 23:01:35]

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

[commit-bot: I haz the power, 2015-02-16 00:39:02]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-02-16 00:40:14]

Patchset 8 (id:??) landed as
https://crrev.com/39d75d5b5f11525e384745fd313d1d77ed7d0a88
Cr-Commit-Position: refs/heads/master@{#316413}