Stop using shell::GetInstance()->system_tray() in system tray items

Stop using shell::GetInstance()->system_tray() in system tray items

This CL provides all SystemTrayItem objects with a pointer to the parent
SystemTray and all Tray*View objects with a pointer to the parent
SystemTrayItem. This allows the objects to walk up the chain and access
the correct system tray once multiple system trays are introduced.

UpdateNagger::~UpdateNagger() in tray_update.cc continues to go through
shell::GetInstance() as the refactoring needed to support multiple system
trays here is more complex and out of my expertise.

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

[bartfab (slow), 2012-11-15 19:11:58]

Hi Steven,

Could you review the ash/system changes?

Hi Mike,

Could you review the tray_bubble_view.* changes?

Hi Scott,

Could you review the shelf_layout_manager_unittest.cc changes?

[msw, 2012-11-15 21:08:48]

I'm not very familiar with TrayBubbleView code, but it seems to awkwardly work
around the View and Bubble conventions for sizing; Steven would be a better
reviewer for that code.

https://codereview.chromium.org/11415014/diff/1/ash/system/tray/system_tray_i...
File ash/system/tray/system_tray_item.h (right):

https://codereview.chromium.org/11415014/diff/1/ash/system/tray/system_tray_i...
ash/system/tray/system_tray_item.h:105: // fixed bubble with. If this returns
true, the bubble width is adjusted when
nit: s/with/width

https://codereview.chromium.org/11415014/diff/1/ui/views/bubble/tray_bubble_v...
File ui/views/bubble/tray_bubble_view.h (right):

https://codereview.chromium.org/11415014/diff/1/ui/views/bubble/tray_bubble_v...
ui/views/bubble/tray_bubble_view.h:117: void SetMinWidth(int width);
This should probably just be SetBubbleWidth and scrap the comparison within.

[stevenjb, 2012-11-15 21:42:04]

https://codereview.chromium.org/11415014/diff/1/ash/system/tray/system_tray_i...
File ash/system/tray/system_tray_item.h (right):

https://codereview.chromium.org/11415014/diff/1/ash/system/tray/system_tray_i...
ash/system/tray/system_tray_item.h:44: int bubble_width);
This is a lot of changes for one use case, and seems like something we would
want for CreateDetailedView() also if we did this.

We already have a bunch of code in SystemTrayItem that calls
Shell::GetInstance()->system_tray(). This will be invalid soon because there may
be more than one system tray, so we will need to set a system_tray_ member in
each SystemTrayItem eventually anyway.

I propose that instead of this change, we pass |this| to each SystemTrayItem
constructor in SystemTray::CreateItems(). Then, any SystemTrayItem code that
needs the bubble width can call:

DCHECK(system_tray()->GetBubbleView());
int width = system_tray()->GetBubbleView()->GetPreferredSize().width();

Make sense?

https://codereview.chromium.org/11415014/diff/1/ash/system/tray/system_tray_i...
ash/system/tray/system_tray_item.h:107: // least its entire preferred size.
Default is false.
This feels awkward. How about a GetMinWidth() that defaults to returning 0
(meaning no minimum)? Cases that would return 'true' here, would return
default_view_->GetPreferredSize().width(). Also, if the class needs LoginStatus
to determine this it should store it, instead of passing it here again.

[bartfab (slow), 2012-11-16 14:56:23]

https://chromiumcodereview.appspot.com/11415014/diff/1/ash/system/tray/system...
File ash/system/tray/system_tray_item.h (right):

https://chromiumcodereview.appspot.com/11415014/diff/1/ash/system/tray/system...
ash/system/tray/system_tray_item.h:44: int bubble_width);
Done. This is a much bigger refactor than I had been planning on doing :).

https://chromiumcodereview.appspot.com/11415014/diff/1/ash/system/tray/system...
ash/system/tray/system_tray_item.h:107: // least its entire preferred size.
Default is false.
Now that Tray*View can walk up the pointer chain all the way to the bubble, I
can make it explicitly modify the bubble width as needed by doing something like
this:

owner()->system_tray()->GetBubbleView()->SetWidth(x);

https://chromiumcodereview.appspot.com/11415014/diff/1/ui/views/bubble/tray_b...
File ui/views/bubble/tray_bubble_view.h (right):

https://chromiumcodereview.appspot.com/11415014/diff/1/ui/views/bubble/tray_b...
ui/views/bubble/tray_bubble_view.h:117: void SetMinWidth(int width);
Will do - moving this to another CL though.

[bartfab (slow), 2012-11-16 15:01:34]

https://chromiumcodereview.appspot.com/11415014/diff/7001/ash/system/bluetoot...
File ash/system/bluetooth/tray_bluetooth.cc (right):

https://chromiumcodereview.appspot.com/11415014/diff/7001/ash/system/bluetoot...
ash/system/bluetooth/tray_bluetooth.cc:34: explicit
BluetoothDefaultView(SystemTrayItem* tray)
I renamed |owner| to |tray| here and elsewhere for consistency. Both names were
being used for the same thing in different classes.

[msw, 2012-11-16 17:10:06]

https://chromiumcodereview.appspot.com/11415014/diff/7001/ash/system/tray/sys...
File ash/system/tray/system_tray_item.h (right):

https://chromiumcodereview.appspot.com/11415014/diff/7001/ash/system/tray/sys...
ash/system/tray/system_tray_item.h:20: class SystemTray;
nit: this forward decl should make a lot of the others unnecessary.
(many of the files touched include this file)

[bartfab (slow), 2012-11-16 17:16:00]

https://chromiumcodereview.appspot.com/11415014/diff/7001/ash/system/tray/sys...
File ash/system/tray/system_tray_item.h (right):

https://chromiumcodereview.appspot.com/11415014/diff/7001/ash/system/tray/sys...
ash/system/tray/system_tray_item.h:20: class SystemTray;
It does make them unnecessary in that the forward declaration happens to be
pulled in, sometimes via multiple steps of inclusion. However, as the style
guide rule is to directly include (or forward declare) all that you use, I
explicitly forward declared the class in the other headers as well.

On 2012/11/16 17:10:06, msw wrote:
> nit: this forward decl should make a lot of the others unnecessary.
> (many of the files touched include this file)

[msw, 2012-11-16 17:41:35]

https://chromiumcodereview.appspot.com/11415014/diff/7001/ash/system/tray/sys...
File ash/system/tray/system_tray_item.h (right):

https://chromiumcodereview.appspot.com/11415014/diff/7001/ash/system/tray/sys...
ash/system/tray/system_tray_item.h:20: class SystemTray;
On 2012/11/16 17:16:01, bartfab wrote:
> It does make them unnecessary in that the forward declaration happens to be
> pulled in, sometimes via multiple steps of inclusion. However, as the style
> guide rule is to directly include (or forward declare) all that you use, I
> explicitly forward declared the class in the other headers as well.
> 
> On 2012/11/16 17:10:06, msw wrote:
> > nit: this forward decl should make a lot of the others unnecessary.
> > (many of the files touched include this file)
> 

True, but for some straightforward cases, you can skip this. For example, not
every user of ui/gfx/rect.h needs to include point.h and size.h for those
fundamental underpinnings of Rect. That said, it's just a nit, and you're free
to ignore me :)

[stevenjb, 2012-11-16 17:41:42]

+jennyz
cc:oshima FYI

Thanks a ton for doing this, it will be a huge help towards the multiple monitor
effort.

https://chromiumcodereview.appspot.com/11415014/diff/7001/ash/system/audio/tr...
File ash/system/audio/tray_volume.h (right):

https://chromiumcodereview.appspot.com/11415014/diff/7001/ash/system/audio/tr...
ash/system/audio/tray_volume.h:13: class SystemTray;
nit: SystemTray is part of the base class constructor, so doesn't really need to
be forward declared here and elsewhere.

https://chromiumcodereview.appspot.com/11415014/diff/7001/ash/system/bluetoot...
File ash/system/bluetooth/tray_bluetooth.cc (right):

https://chromiumcodereview.appspot.com/11415014/diff/7001/ash/system/bluetoot...
ash/system/bluetooth/tray_bluetooth.cc:34: explicit
BluetoothDefaultView(SystemTrayItem* tray)
On 2012/11/16 15:01:35, bartfab wrote:
> I renamed |owner| to |tray| here and elsewhere for consistency. Both names
were
> being used for the same thing in different classes.
Actually, sorry to push back on this, but "tray" implies "system tray" to me.
This should be "tray_item" or just "item" (or owner). tray()->system_tray()
below looks particularly confusing.

https://chromiumcodereview.appspot.com/11415014/diff/7001/ash/system/tray/sys...
File ash/system/tray/system_tray_item.h (right):

https://chromiumcodereview.appspot.com/11415014/diff/7001/ash/system/tray/sys...
ash/system/tray/system_tray_item.h:20: class SystemTray;
On 2012/11/16 17:16:01, bartfab wrote:
> It does make them unnecessary in that the forward declaration happens to be
> pulled in, sometimes via multiple steps of inclusion. However, as the style
> guide rule is to directly include (or forward declare) all that you use, I
> explicitly forward declared the class in the other headers as well.
> 
> On 2012/11/16 17:10:06, msw wrote:
> > nit: this forward decl should make a lot of the others unnecessary.
> > (many of the files touched include this file)
> 

I mentioned this earlier also. Forward declarations work like #incldues. If A
includes B which includes C, A does not always need to include C (e.g. if A
derives from B derives from C, generally A only includes B). Use good judgement
here. In this case, SystemTray is part of the constructor for SystemTrayItem, so
anything that includes system_tray_item.h can safely assume that SystemTray will
remain declared; the declaration can not be removed without changing the
constructor and thus all dependent files.

[stevenjb, 2012-11-16 17:46:55]

https://chromiumcodereview.appspot.com/11415014/diff/1/ash/system/tray/system...
File ash/system/tray/system_tray_item.h (right):

https://chromiumcodereview.appspot.com/11415014/diff/1/ash/system/tray/system...
ash/system/tray/system_tray_item.h:44: int bubble_width);
On 2012/11/16 14:56:24, bartfab wrote:
> Done. This is a much bigger refactor than I had been planning on doing :).
Yeah, sorry about that, but really appreciate it.

https://chromiumcodereview.appspot.com/11415014/diff/1/ash/system/tray/system...
ash/system/tray/system_tray_item.h:107: // least its entire preferred size.
Default is false.
On 2012/11/16 14:56:24, bartfab wrote:
> Now that Tray*View can walk up the pointer chain all the way to the bubble, I
> can make it explicitly modify the bubble width as needed by doing something
like
> this:
> 
> owner()->system_tray()->GetBubbleView()->SetWidth(x);

That sounds reasonable also.

[bartfab (slow), 2012-11-16 18:45:34]

https://chromiumcodereview.appspot.com/11415014/diff/7001/ash/system/audio/tr...
File ash/system/audio/tray_volume.h (right):

https://chromiumcodereview.appspot.com/11415014/diff/7001/ash/system/audio/tr...
ash/system/audio/tray_volume.h:13: class SystemTray;
On 2012/11/16 17:41:43, stevenjb (chromium) wrote:
> nit: SystemTray is part of the base class constructor, so doesn't really need
to
> be forward declared here and elsewhere.

Done.

https://chromiumcodereview.appspot.com/11415014/diff/7001/ash/system/bluetoot...
File ash/system/bluetooth/tray_bluetooth.cc (right):

https://chromiumcodereview.appspot.com/11415014/diff/7001/ash/system/bluetoot...
ash/system/bluetooth/tray_bluetooth.cc:34: explicit
BluetoothDefaultView(SystemTrayItem* tray)
I picked |tray| over |owner| because it seems to have been chosen more often in
the past. But I agree that |owner| is cleaner. Done.

On 2012/11/16 17:41:43, stevenjb (chromium) wrote:
> On 2012/11/16 15:01:35, bartfab wrote:
> > I renamed |owner| to |tray| here and elsewhere for consistency. Both names
> were
> > being used for the same thing in different classes.
> Actually, sorry to push back on this, but "tray" implies "system tray" to me.
> This should be "tray_item" or just "item" (or owner). tray()->system_tray()
> below looks particularly confusing.
>

https://chromiumcodereview.appspot.com/11415014/diff/7001/ash/system/tray/sys...
File ash/system/tray/system_tray_item.h (right):

https://chromiumcodereview.appspot.com/11415014/diff/7001/ash/system/tray/sys...
ash/system/tray/system_tray_item.h:20: class SystemTray;
On 2012/11/16 17:41:43, stevenjb (chromium) wrote:
> On 2012/11/16 17:16:01, bartfab wrote:
> > It does make them unnecessary in that the forward declaration happens to be
> > pulled in, sometimes via multiple steps of inclusion. However, as the style
> > guide rule is to directly include (or forward declare) all that you use, I
> > explicitly forward declared the class in the other headers as well.
> > 
> > On 2012/11/16 17:10:06, msw wrote:
> > > nit: this forward decl should make a lot of the others unnecessary.
> > > (many of the files touched include this file)
> > 
> 
> I mentioned this earlier also. Forward declarations work like #incldues. If A
> includes B which includes C, A does not always need to include C (e.g. if A
> derives from B derives from C, generally A only includes B). Use good
judgement
> here. In this case, SystemTray is part of the constructor for SystemTrayItem,
so
> anything that includes system_tray_item.h can safely assume that SystemTray
will
> remain declared; the declaration can not be removed without changing the
> constructor and thus all dependent files.

Done.

[jennyz, 2012-11-16 19:16:14]

lgtm

[stevenjb, 2012-11-16 19:23:52]

Thanks a ton for doing this.
LGTM

[msw, 2012-11-16 20:09:04]

LGTM with a couple nits; thanks!

https://chromiumcodereview.appspot.com/11415014/diff/2002/ash/system/chromeos...
File ash/system/chromeos/network/tray_network.cc (right):

https://chromiumcodereview.appspot.com/11415014/diff/2002/ash/system/chromeos...
ash/system/chromeos/network/tray_network.cc:367: : NetworkDetailedView(owner) {
nit: indent one more space.

https://chromiumcodereview.appspot.com/11415014/diff/2002/ash/system/user/tra...
File ash/system/user/tray_user.cc (right):

https://chromiumcodereview.appspot.com/11415014/diff/2002/ash/system/user/tra...
ash/system/user/tray_user.cc:8: #include "ash/system/tray/system_tray.h"
nit: potentially not necessary; tray_user.h include system_tray_item.h

[bartfab (slow), 2012-11-19 17:15:30]

https://chromiumcodereview.appspot.com/11415014/diff/2002/ash/system/chromeos...
File ash/system/chromeos/network/tray_network.cc (right):

https://chromiumcodereview.appspot.com/11415014/diff/2002/ash/system/chromeos...
ash/system/chromeos/network/tray_network.cc:367: : NetworkDetailedView(owner) {
On 2012/11/16 20:09:04, msw wrote:
> nit: indent one more space.

Done.

https://chromiumcodereview.appspot.com/11415014/diff/2002/ash/system/user/tra...
File ash/system/user/tray_user.cc (right):

https://chromiumcodereview.appspot.com/11415014/diff/2002/ash/system/user/tra...
ash/system/user/tray_user.cc:8: #include "ash/system/tray/system_tray.h"
system_tray_item.h has a forward declaration of the SystemTray class only. This
file needs the actual definition.

[oshima, 2012-11-19 17:43:42]

drive-by:

https://chromiumcodereview.appspot.com/11415014/diff/2002/ash/system/user/tra...
File ash/system/user/tray_user.cc (right):

https://chromiumcodereview.appspot.com/11415014/diff/2002/ash/system/user/tra...
ash/system/user/tray_user.cc:8: #include "ash/system/tray/system_tray.h"
On 2012/11/19 17:15:31, bartfab wrote:
> system_tray_item.h has a forward declaration of the SystemTray class only.
This
> file needs the actual definition.

looks like it's not used in a way need definition?

[bartfab (slow), 2012-11-19 18:00:30]

https://chromiumcodereview.appspot.com/11415014/diff/2002/ash/system/user/tra...
File ash/system/user/tray_user.cc (right):

https://chromiumcodereview.appspot.com/11415014/diff/2002/ash/system/user/tra...
ash/system/user/tray_user.cc:8: #include "ash/system/tray/system_tray.h"
Actually, you are right. I can remove the include - it is a somewhat moot
exercise though as my next CL will add it straight back because it will be using
the system tray object.

On 2012/11/19 17:43:43, oshima wrote:
> On 2012/11/19 17:15:31, bartfab wrote:
> > system_tray_item.h has a forward declaration of the SystemTray class only.
> This
> > file needs the actual definition.
> 
> looks like it's not used in a way need definition?
>

[oshima, 2012-11-19 21:14:17]

http://codereview.chromium.org/11415014/diff/2002/ash/system/user/tray_user.cc
File ash/system/user/tray_user.cc (right):

http://codereview.chromium.org/11415014/diff/2002/ash/system/user/tray_user.c...
ash/system/user/tray_user.cc:8: #include "ash/system/tray/system_tray.h"
On 2012/11/19 18:00:31, bartfab wrote:
> Actually, you are right. I can remove the include - it is a somewhat moot
> exercise though as my next CL will add it straight back because it will be
using
> the system tray object.
> 
> On 2012/11/19 17:43:43, oshima wrote:
> > On 2012/11/19 17:15:31, bartfab wrote:
> > > system_tray_item.h has a forward declaration of the SystemTray class only.
> > This
> > > file needs the actual definition.
> > 
> > looks like it's not used in a way need definition?
> > 
> 

If this will be necessary in next CL, please add it in that CL.
This way, we can keep the code in clean state.

[bartfab (slow), 2012-11-19 22:32:25]

http://codereview.chromium.org/11415014/diff/2002/ash/system/user/tray_user.cc
File ash/system/user/tray_user.cc (right):

http://codereview.chromium.org/11415014/diff/2002/ash/system/user/tray_user.c...
ash/system/user/tray_user.cc:8: #include "ash/system/tray/system_tray.h"
Yes, of course. I addressed your comment and removed the include. I just
mentioned that it will be used soon so that the unnecessary include would have
become useful soon anyway, even if it had not been spotted now.

On 2012/11/19 21:14:17, oshima wrote:
> On 2012/11/19 18:00:31, bartfab wrote:
> > Actually, you are right. I can remove the include - it is a somewhat moot
> > exercise though as my next CL will add it straight back because it will be
> using
> > the system tray object.
> > 
> > On 2012/11/19 17:43:43, oshima wrote:
> > > On 2012/11/19 17:15:31, bartfab wrote:
> > > > system_tray_item.h has a forward declaration of the SystemTray class
only.
> > > This
> > > > file needs the actual definition.
> > > 
> > > looks like it's not used in a way need definition?
> > > 
> > 
> 
> If this will be necessary in next CL, please add it in that CL.
> This way, we can keep the code in clean state.

[oshima, 2012-11-19 22:53:12]

lgtm

http://codereview.chromium.org/11415014/diff/2002/ash/system/user/tray_user.cc
File ash/system/user/tray_user.cc (right):

http://codereview.chromium.org/11415014/diff/2002/ash/system/user/tray_user.c...
ash/system/user/tray_user.cc:8: #include "ash/system/tray/system_tray.h"
On 2012/11/19 22:32:25, bartfab wrote:
> Yes, of course. I addressed your comment and removed the include. I just
> mentioned that it will be used soon so that the unnecessary include would have
> become useful soon anyway, even if it had not been spotted now.

I understand, but we generally advise to add it when you need it, not when
you will need it. This is because many people are working on the same files and
not everyone would know why it's there, and such line may be deleted by
other people.

> 
> On 2012/11/19 21:14:17, oshima wrote:
> > On 2012/11/19 18:00:31, bartfab wrote:
> > > Actually, you are right. I can remove the include - it is a somewhat moot
> > > exercise though as my next CL will add it straight back because it will be
> > using
> > > the system tray object.
> > > 
> > > On 2012/11/19 17:43:43, oshima wrote:
> > > > On 2012/11/19 17:15:31, bartfab wrote:
> > > > > system_tray_item.h has a forward declaration of the SystemTray class
> only.
> > > > This
> > > > > file needs the actual definition.
> > > > 
> > > > looks like it's not used in a way need definition?
> > > > 
> > > 
> > 
> > If this will be necessary in next CL, please add it in that CL.
> > This way, we can keep the code in clean state.
>

[bartfab (slow), 2012-11-19 22:57:54]

http://codereview.chromium.org/11415014/diff/2002/ash/system/user/tray_user.cc
File ash/system/user/tray_user.cc (right):

http://codereview.chromium.org/11415014/diff/2002/ash/system/user/tray_user.c...
ash/system/user/tray_user.cc:8: #include "ash/system/tray/system_tray.h"
Absolutely. It was a mistake.

On 2012/11/19 22:53:12, oshima wrote:
> On 2012/11/19 22:32:25, bartfab wrote:
> > Yes, of course. I addressed your comment and removed the include. I just
> > mentioned that it will be used soon so that the unnecessary include would
have
> > become useful soon anyway, even if it had not been spotted now.
> 
> I understand, but we generally advise to add it when you need it, not when
> you will need it. This is because many people are working on the same files
and
> not everyone would know why it's there, and such line may be deleted by
> other people.
> 
> > 
> > On 2012/11/19 21:14:17, oshima wrote:
> > > On 2012/11/19 18:00:31, bartfab wrote:
> > > > Actually, you are right. I can remove the include - it is a somewhat
moot
> > > > exercise though as my next CL will add it straight back because it will
be
> > > using
> > > > the system tray object.
> > > > 
> > > > On 2012/11/19 17:43:43, oshima wrote:
> > > > > On 2012/11/19 17:15:31, bartfab wrote:
> > > > > > system_tray_item.h has a forward declaration of the SystemTray class
> > only.
> > > > > This
> > > > > > file needs the actual definition.
> > > > > 
> > > > > looks like it's not used in a way need definition?
> > > > > 
> > > > 
> > > 
> > > If this will be necessary in next CL, please add it in that CL.
> > > This way, we can keep the code in clean state.
> > 
>

[commit-bot: I haz the power, 2012-11-20 10:36:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/11415014/107

[commit-bot: I haz the power, 2012-11-20 10:37:22]

Presubmit check for 11415014-107 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.0s to calculate.

[bartfab (slow), 2012-11-20 10:45:26]

Hi Scott,

Review ping for shelf_layout_manager_unittest.cc.

[sky, 2012-11-21 15:02:00]

LGTM

[commit-bot: I haz the power, 2012-11-21 15:02:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/11415014/14001

[commit-bot: I haz the power, 2012-11-21 15:40:41]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-21 19:36:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/11415014/1055

[commit-bot: I haz the power, 2012-11-21 21:42:01]

Change committed as 169141