Add SystemTrayObservers

Add SystemTrayObservers

Once there is more than one SystemTray instance, we will need to support observer lists so that multiple tray items can be notified.

Hosting the observer lists in a separate class owned by ass::SystemTrayDelegate seemed like the most forward approach.

BUG=159543
For ash.gyp:
TBR=sky@chromium.org


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

[stevenjb, 2012-11-08 02:32:59]

Sadrul, can you take a look at this one? Does this seem like a reasonable
approach?

[oshima, 2012-11-09 19:21:50]

http://codereview.chromium.org/11312139/diff/1/ash/system/tray/system_tray_ob...
File ash/system/tray/system_tray_observers.h (right):

http://codereview.chromium.org/11312139/diff/1/ash/system/tray/system_tray_ob...
ash/system/tray/system_tray_observers.h:41: }
new line between methods.

http://codereview.chromium.org/11312139/diff/1/chrome/browser/chromeos/locale...
File chrome/browser/chromeos/locale_change_guard.cc (right):

http://codereview.chromium.org/11312139/diff/1/chrome/browser/chromeos/locale...
chrome/browser/chromeos/locale_change_guard.cc:177:
ash::Shell::GetInstance()->tray_delegate()->GetSystemTrayObservers();
TrayDelegate has NotifyXXX methods for several other 
notifications, so it may make sense to define such methods
for these notifications as well. Yon you don't have to expose
observers and eliminate NULL check.

[sadrul, 2012-11-09 23:26:32]

Apologies for the really long delay in review.

http://codereview.chromium.org/11312139/diff/1/chrome/browser/chromeos/locale...
File chrome/browser/chromeos/locale_change_guard.cc (right):

http://codereview.chromium.org/11312139/diff/1/chrome/browser/chromeos/locale...
chrome/browser/chromeos/locale_change_guard.cc:177:
ash::Shell::GetInstance()->tray_delegate()->GetSystemTrayObservers();
I agree that it would be nice if these places all didn't have to loop over the
observer-list.

There could be TrayDelegate::NotifyXXX. Another approach would be to create a
Shell::SystemTrayProxy (or SystemTrayObserver), which is notified of these
changes. The Proxy/Observer can then iterate over the system-trays and update
their observers accordingly.

[stevenjb, 2012-11-09 23:40:44]

http://codereview.chromium.org/11312139/diff/1/chrome/browser/chromeos/locale...
File chrome/browser/chromeos/locale_change_guard.cc (right):

http://codereview.chromium.org/11312139/diff/1/chrome/browser/chromeos/locale...
chrome/browser/chromeos/locale_change_guard.cc:177:
ash::Shell::GetInstance()->tray_delegate()->GetSystemTrayObservers();
On 2012/11/09 23:26:32, sadrul wrote:
> I agree that it would be nice if these places all didn't have to loop over the
> observer-list.
> 
> There could be TrayDelegate::NotifyXXX. Another approach would be to create a
> Shell::SystemTrayProxy (or SystemTrayObserver), which is notified of these
> changes. The Proxy/Observer can then iterate over the system-trays and update
> their observers accordingly.

Yeah, I started to create NotifyXXX, but realized that most of these
could/should be moved to Ash at some point and removed from the
SystemTrayDelegate entirely.

That said, I'm OK with moving these to NotifyXXX in the meanwhile, and moving
all of NotifyXXX to SystemTrayObservers such that all of the calls look like:

ash::Shell::GetInstance()->tray_delegate()->GetSystemTrayObservers()->NotifyXXX(...)

A bit of a mouthful, but it would tidy up SystemTrayDelegate a bit. Sound
reasonable?

We could also put GetSystemTrayObservers() directly into Shell to make this less
verbose?

[stevenjb, 2012-11-12 18:14:42]

I couldn't eliminate testing the result of GetSystemTrayObservers() because we
could be be using a stub implementation that doesn't have a SystemTrayObservers
implementation. However, this does seem much cleaner and doesn't require the
observer list iteration outside of SystemTrayObservers.

PTAL

[stevenjb, 2012-11-13 18:38:06]

Ping

[oshima, 2012-11-13 19:30:04]

On 2012/11/12 18:14:42, stevenjb (chromium) wrote:
> I couldn't eliminate testing the result of GetSystemTrayObservers() because we
> could be be using a stub implementation that doesn't have a
SystemTrayObservers
> implementation. However, this does seem much cleaner and doesn't require the
> observer list iteration outside of SystemTrayObservers.
> 
> PTAL

Can't stub implementation returns the same object? I'm ok to have separate
object if
that's better option, but I'd call it XxxNotifier instead of XxxObserver because
that's what it will do. (Observers implies implementation)

[stevenjb, 2012-11-13 20:33:26]

I renamed SystemTrayObservers to SystemTrayNotifier and moved ownership to
ash:Shell so that we don't need to test it everywhere.

[stevenjb, 2012-11-13 20:36:12]

PTAL

[sadrul, 2012-11-13 20:54:50]

LGTM

The notifier looks a bit tedious, but I can't think of a way to make this better
:(

http://codereview.chromium.org/11312139/diff/6003/ash/system/tray/system_tray.h
File ash/system/tray/system_tray.h (right):

http://codereview.chromium.org/11312139/diff/6003/ash/system/tray/system_tray...
ash/system/tray/system_tray.h:65: // Calls TrayBackgroundView::Initialize()
" .. and creates the default set of items ..."

[stevenjb (google-dont-use), 2012-11-13 21:01:47]

Agreed on the tediousness and lack of straightforward better options.
Hopefully a bunch of this will go away over time as code gets moved from
src/chrome to src/chromeos and src/ash.



On Tue, Nov 13, 2012 at 12:54 PM, <sadrul@chromium.org> wrote:

> LGTM
>
> The notifier looks a bit tedious, but I can't think of a way to make this
> better
> :(
>
>
> http://codereview.chromium.**org/11312139/diff/6003/ash/**
>
system/tray/system_tray.h<http://codereview.chromium.org/11312139/diff/6003/ash/system/tray/system_tray.h>
> File ash/system/tray/system_tray.h (right):
>
> http://codereview.chromium.**org/11312139/diff/6003/ash/**
>
system/tray/system_tray.h#**newcode65<http://codereview.chromium.org/11312139/diff/6003/ash/system/tray/system_tray.h#newcode65>
> ash/system/tray/system_tray.h:**65: // Calls
> TrayBackgroundView::**Initialize()
> " .. and creates the default set of items ..."
>
>
http://codereview.chromium.**org/11312139/<http://codereview.chromium.org/113...
>

[oshima, 2012-11-13 21:15:00]

lgtm

thanks, looks much better than before (this CL).

[stevenjb, 2012-11-13 21:19:53]

http://codereview.chromium.org/11312139/diff/6003/ash/system/tray/system_tray.h
File ash/system/tray/system_tray.h (right):

http://codereview.chromium.org/11312139/diff/6003/ash/system/tray/system_tray...
ash/system/tray/system_tray.h:65: // Calls TrayBackgroundView::Initialize()
On 2012/11/13 20:54:50, sadrul wrote:
> " .. and creates the default set of items ..."

Done.

[commit-bot: I haz the power, 2012-11-13 22:17:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/11312139/1024

[commit-bot: I haz the power, 2012-11-13 22:17:53]

Presubmit check for 11312139-1024 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    ash

Presubmit checks took 4.4s to calculate.

[commit-bot: I haz the power, 2012-11-13 22:28:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/11312139/1024

[commit-bot: I haz the power, 2012-11-14 01:11:03]

Change committed as 167548