Close notification windows while message loop is still active

Close notification windows while message loop is still active
Reset tooltip_controller explicitly so that timer stops at shutdown.
Use tray_ reference in SystemTrayDelegate instead of going through ash instance.
Make sure delegates doesn't access tray after the tray has been deleted.

BUG=104998
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=131902

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=132320

[oshima, 2012-04-09 20:21:41]

Sadrul, can you take a look at this first, especially the changes to system tray
delegate?

[oshima, 2012-04-10 16:32:32]

On 2012/04/09 20:21:41, oshima wrote:
> Sadrul, can you take a look at this first, especially the changes to system
tray
> delegate?

friendly ping.

[sadrul, 2012-04-10 17:01:24]

The tray related changes LGTM.

Regarding the other changes: I don't know the chrome shutdown path that well.
Having said that ... it does LGTM

[oshima, 2012-04-10 18:17:53]

+ben for OWNERS review.

[Ben Goodger (Google), 2012-04-10 22:35:02]

http://codereview.chromium.org/9968101/diff/23003/ash/shell.h
File ash/shell.h (right):

http://codereview.chromium.org/9968101/diff/23003/ash/shell.h#newcode350
ash/shell.h:350: // If the shell has been shutdown.
shut down

http://codereview.chromium.org/9968101/diff/23003/chrome/browser/ui/browser_l...
File chrome/browser/ui/browser_list_aura.cc (right):

http://codereview.chromium.org/9968101/diff/23003/chrome/browser/ui/browser_l...
chrome/browser/ui/browser_list_aura.cc:26:
ash::Shell::GetInstance()->Shutdown();
can you delete the instance and then set shutdown_ statically so it knows not to
recreate, rather than leaving a non-NULL Shell instance with a bunch of NULL
ptrs hanging off it?

[oshima, 2012-04-10 23:06:55]

http://codereview.chromium.org/9968101/diff/23003/chrome/browser/ui/browser_l...
File chrome/browser/ui/browser_list_aura.cc (right):

http://codereview.chromium.org/9968101/diff/23003/chrome/browser/ui/browser_l...
chrome/browser/ui/browser_list_aura.cc:26:
ash::Shell::GetInstance()->Shutdown();
On 2012/04/10 22:35:02, Ben Goodger (Google) wrote:
> can you delete the instance and then set shutdown_ statically so it knows not
to
> recreate, rather than leaving a non-NULL Shell instance with a bunch of NULL
> ptrs hanging off it? 

Looks like GetInstance no longer creates instance. Changed to DeleteInstance()

[Ben Goodger (Google), 2012-04-11 21:07:11]

lgtm

[commit-bot: I haz the power, 2012-04-11 21:58:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/9968101/23005

[commit-bot: I haz the power, 2012-04-11 21:58:20]

Can't apply patch for file tools/valgrind/memcheck/suppressions.txt.
While running patch -p1 --forward --force;
patching file tools/valgrind/memcheck/suppressions.txt
Hunk #1 FAILED at 4442.
1 out of 1 hunk FAILED -- saving rejects to file
tools/valgrind/memcheck/suppressions.txt.rej

[commit-bot: I haz the power, 2012-04-11 22:24:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/9968101/31001

[commit-bot: I haz the power, 2012-04-11 22:26:22]

Try job failure for 9968101-31001 on android for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-04-11 22:45:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/9968101/31001

[commit-bot: I haz the power, 2012-04-12 02:24:52]

Change committed as 131902

[oshima, 2012-04-12 23:32:17]

On 2012/04/12 02:24:52, I haz the power (commit-bot) wrote:
> Change committed as 131902

Ben, can you take another look?

I'll work on CloseAllSecondaryWidgets separately and I removed
Shell::DeleteInstance for now.
I also moved OnTrayDestroyed to the SystemTray's dtor because I believe that's
more appropriate.

[Ben Goodger (Google), 2012-04-12 23:33:56]

http://codereview.chromium.org/9968101/diff/29006/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

http://codereview.chromium.org/9968101/diff/29006/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:608: tray_ = NULL;
rather than NULL checking everything in this file, can we just remove this
object as an observer from the various things it observes?

[oshima, 2012-04-12 23:50:27]

http://codereview.chromium.org/9968101/diff/29006/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

http://codereview.chromium.org/9968101/diff/29006/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:608: tray_ = NULL;
On 2012/04/12 23:33:56, Ben Goodger (Google) wrote:
> rather than NULL checking everything in this file, can we just remove this
> object as an observer from the various things it observes?

Yes, I thought about it. I still need NULL check for
Shell::GetInstance()->tray_delegate() (but not all places),
and I felt it's cleaner to do NULL check in one place instead of
doing this in multiple places.

This NULL check is necessary only if we close tray/widgets before
message loop is destroyed, so another option is to exclude it for now.

Let me know which way you prefer.

[oshima, 2012-04-13 08:34:32]

Uploaded new patch. PTAL.

Since NULL check isn't necessary now, I removed them and deleted delegate. I'll
add NULL check when I implement CloseAllSecondaryWidgets().

[Ben Goodger (Google), 2012-04-13 15:25:10]

This seems much nicer.
LGTM.

[commit-bot: I haz the power, 2012-04-13 15:43:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/9968101/41002

[commit-bot: I haz the power, 2012-04-14 00:33:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/9968101/41002

[commit-bot: I haz the power, 2012-04-14 00:50:42]

Try job failure for 9968101-41002 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-04-14 02:08:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/9968101/61002

[commit-bot: I haz the power, 2012-04-14 03:24:55]

Change committed as 132320