aura: Make sure TestScreen objects are destroyed properly.

aura: Make sure TestScreen objects are destroyed properly.

BUG=none
R=sky@chromium.org
TBR=pfeldman@ for content/shell/ changes

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=279155

[sadrul, 2014-06-19 20:04:21]

This is on top of https://codereview.chromium.org/345603003/. I can change the
order of the CLs, and land this one first instead.

[sky, 2014-06-19 20:33:21]

https://codereview.chromium.org/344833004/diff/70001/chrome/browser/signin/si...
File chrome/browser/signin/signin_error_notifier_ash_unittest.cc (left):

https://codereview.chromium.org/344833004/diff/70001/chrome/browser/signin/si...
chrome/browser/signin/signin_error_notifier_ash_unittest.cc:27: #if
!defined(OS_CHROMEOS)
Don't we want to run this on linux non-chromeos? Same comment for all of them.

[sadrul, 2014-06-19 21:31:51]

https://codereview.chromium.org/344833004/diff/70001/chrome/browser/signin/si...
File chrome/browser/signin/signin_error_notifier_ash_unittest.cc (left):

https://codereview.chromium.org/344833004/diff/70001/chrome/browser/signin/si...
chrome/browser/signin/signin_error_notifier_ash_unittest.cc:27: #if
!defined(OS_CHROMEOS)
On 2014/06/19 20:33:20, sky wrote:
> Don't we want to run this on linux non-chromeos? Same comment for all of them.

It doesn't look like this is actually used/needed on non-Win (see below in line
81).

[sky, 2014-06-19 23:14:11]

ic, LGTM

[sadrul, 2014-06-20 00:59:29]

+oshima@: James mentioned the TestScreen's leaking may have been deliberate
(https://codereview.chromium.org/345603003/#msg6)? Can you take a look?

[sadrul, 2014-06-20 01:00:28]

+pfeldman@ for content/shell owner

[oshima, 2014-06-20 18:37:59]

On 2014/06/20 00:59:29, sadrul wrote:
> +oshima@: James mentioned the TestScreen's leaking may have been deliberate
> (https://codereview.chromium.org/345603003/#msg6)? Can you take a look?

We used to leak it because Screen object may be accessed after ash shutdown.
I fixed it by creating a dedicated screen instance during shutdown, so there
should be
no leak in ash now.

I *think* this is not an issue in unittest. If this does not cause any issue,
then you can go ahead.

[sadrul, 2014-06-23 18:53:00]

On 2014/06/20 18:37:59, oshima wrote:
> On 2014/06/20 00:59:29, sadrul wrote:
> > +oshima@: James mentioned the TestScreen's leaking may have been deliberate
> > (https://codereview.chromium.org/345603003/#msg6)? Can you take a look?
> 
> We used to leak it because Screen object may be accessed after ash shutdown.
> I fixed it by creating a dedicated screen instance during shutdown, so there
> should be
> no leak in ash now.
> 
> I *think* this is not an issue in unittest. If this does not cause any issue,
> then you can go ahead.

Cool. The tests seem happy with this change. Thanks.

TBR'ing pfeldman@ for content/shell changes

[sadrul, 2014-06-23 18:55:32]

Committed patchset #5 manually as r279155 (presubmit successful).