Prepare SystemTray to support notifications

Prepare SystemTray to support notifications

The following cleanup was done in preparation for notifications which may require multiple SystemTrayBubble instances:

* Separate out SystemTrayBubbleView from SystemTrayBubble
* Move SystemTray.popup_ -> SystemTrayBubble.bubble_widget_
* Remove scoped_ptr for views (views are owned by the view hierarchy)
* Changed behavior to not close when focus is lost so that the default view can be reused.

BUG=124269
TEST=All tests should pass; system tray behavior should be unchanged.

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

[sadrul, 2012-04-26 20:12:24]

Are there docs for the system-tray notifications somewhere?

I was thinking there will be a new TrayItem (like TrayNotification), which will
have a detailed-view that contains the different notifications (essentially the
same way the caps-lock/accessibility changed notifications show up). Is that how
you are planning on doing this?

> * Move SystemTray.popup_ -> SystemTrayBubble.bubble_widget_ 

+1

> * Remove scoped_ptr for views (views are owned by the view hierarchy) 

This was done mostly deliberately so there's no issue with
cleanup/initialization/recreating a view etc. Unless this is causing other
issues, I would rather keep the scoped_ptr's (the views are removed from the
hierarchy at the right time, so this does not cause double-free or things like
that).

> * Separate out SystemTrayBubbleView from SystemTrayBubble 
> * Changed behavior to not close when focus is lost so that the default view
can be reused. 

It is slightly difficult to notice the change of behaviour of the last entry.
Can you point out where this change happening?

[stevenjb (google-dont-use), 2012-04-26 20:34:55]

On Thu, Apr 26, 2012 at 1:12 PM, <sadrul@chromium.org> wrote:

> Are there docs for the system-tray notifications somewhere?
>
> I was thinking there will be a new TrayItem (like TrayNotification), which
> will
> have a detailed-view that contains the different notifications
> (essentially the
> same way the caps-lock/accessibility changed notifications show up). Is
> that how
> you are planning on doing this?
>
> What docs exist are here:
https://docs.google.com/a/google.com/document/d/1VnO3cL6e5m4bcUDiVuWDQ-6tlaVM...

There are two parts to notifications. One is a separate tray, which I
haven't thought much about yet.
The other are notifications associated with tray items like power and
network, which is what I am working on now. Mostly these will behave like
volume / brightness, but there are use cases where we will need to display
both the notification and a detailed view. This is what I am looking to
support.


>  * Move SystemTray.popup_ -> SystemTrayBubble.bubble_**widget_
>>
>
> +1
>
>
>  * Remove scoped_ptr for views (views are owned by the view hierarchy)
>>
>
> This was done mostly deliberately so there's no issue with
> cleanup/initialization/**recreating a view etc. Unless this is causing
> other
> issues, I would rather keep the scoped_ptr's (the views are removed from
> the
> hierarchy at the right time, so this does not cause double-free or things
> like
> that).
>

Mostly it's just confusing. scoped_ptr implies ownership, but the views are
actually owned by the hierarchy, and deleting them early (i.e. when the
model is destroyed) is a bit odd. This confused me for a while, and I
couldn't think of a situation where the scoped ptr would actually help.


>  * Separate out SystemTrayBubbleView from SystemTrayBubble
>> * Changed behavior to not close when focus is lost so that the default
>> view
>>
> can be reused.
>
> It is slightly difficult to notice the change of behaviour of the last
> entry.
> Can you point out where this change happening?
>

Yeah, it's the addition of bubble_view_->set_close_on_deactivate(false)
in SystemTrayBubble::InitView(), and the change
to SystemTray::ShowDefaultView().

Basically I assumed that popup_ (now bobble_widget_) ->Hide() was in fact
hiding the widget and not closing it. It took me a bit to track that down
to BubbleDelegateView::OnWidgetActivationChanged(). I wanted to either call
Close() explicitly or make Hide() actually hide, and chose the latter.



>
>
http://codereview.chromium.**org/10235010/<http://codereview.chromium.org/102...
>

[stevenjb, 2012-04-30 15:55:36]

Ping? I have another CL just about ready to go based on this one.

[Ben Goodger (Google), 2012-04-30 16:02:54]

How are the notifications going to work? My understanding looking at the mocks
is that they are only going to share the presentation and the menu behavior. I
would be surprised to see much change to the systray beyond this.

[Ben Goodger (Google), 2012-04-30 16:04:50]

Also, I would suggest that the notifications UI be implemented in a separate
component that ash consumes.

src/ui/notifications seems reasonable.

The reason is that we will probably want to run this UI outside ash on Windows
and Linux. You don't need to do that work now, but if you design it as a
separate component now it'll make it easier to support this use case.

[stevenjb, 2012-04-30 16:15:07]

The notifications for system items (power, networking, locale change) are
integrated with the system tray; that's what this CL is for. Details here:
https://docs.google.com/a/google.com/document/d/1VnO3cL6e5m4bcUDiVuWDQ-6tlaVM...

Other notifications (web, app) will be implemented separately. Putting that code
in ui/notifications sounds reasonable.

[Ben Goodger (Google), 2012-04-30 17:07:48]

That makes sense - thanks for the explaination!

On Mon, Apr 30, 2012 at 9:15 AM, <stevenjb@chromium.org> wrote:

> The notifications for system items (power, networking, locale change) are
> integrated with the system tray; that's what this CL is for. Details here:
> https://docs.google.com/a/**google.com/document/d/**
>
1VnO3cL6e5m4bcUDiVuWDQ-**6tlaVMPeo0J0lPhQLdG-E/edit?**disco=AAAAAElE5Cw<https://docs.google.com/a/google.com/document/d/1VnO3cL6e5m4bcUDiVuWDQ-6tlaVMPeo0J0lPhQLdG-E/edit?disco=AAAAAElE5Cw>
>
> Other notifications (web, app) will be implemented separately. Putting
> that code
> in ui/notifications sounds reasonable.
>
>
>
http://codereview.chromium.**org/10235010/<http://codereview.chromium.org/102...
>

[sadrul, 2012-04-30 17:46:19]

Please move the removal of scoped_ptr use in a separate cleanup CL

http://codereview.chromium.org/10235010/diff/6002/ash/system/tray/system_tray.cc
File ash/system/tray/system_tray.cc (right):

http://codereview.chromium.org/10235010/diff/6002/ash/system/tray/system_tray...
ash/system/tray/system_tray.cc:440:
bubble_view_->set_close_on_deactivate(false);
Is there a reason for this change?

http://codereview.chromium.org/10235010/diff/6002/ash/system/tray/system_tray...
ash/system/tray/system_tray.cc:532: bubble_widget_->Hide();
This should, in fact, close the bubble.

[stevenjb, 2012-04-30 19:09:53]

Will move scoped_ptr changes to separate CL.

http://codereview.chromium.org/10235010/diff/6002/ash/system/tray/system_tray.cc
File ash/system/tray/system_tray.cc (right):

http://codereview.chromium.org/10235010/diff/6002/ash/system/tray/system_tray...
ash/system/tray/system_tray.cc:440:
bubble_view_->set_close_on_deactivate(false);
On 2012/04/30 17:46:19, sadrul wrote:
> Is there a reason for this change?
Yes, as described, this makes the window hide on deactivate instead of close. If
we prefer close, I can change the API to close instead of hide whenever focus is
lost. That might simplify a couple of issues with the introduction of popup
notifications, so I'll go ahead and do that.
Done.

http://codereview.chromium.org/10235010/diff/6002/ash/system/tray/system_tray...
ash/system/tray/system_tray.cc:532: bubble_widget_->Hide();
On 2012/04/30 17:46:19, sadrul wrote:
> This should, in fact, close the bubble.
Done. See above.

[sadrul, 2012-04-30 20:01:51]

http://codereview.chromium.org/10235010/diff/25003/ash/system/tray/system_tra...
File ash/system/tray/system_tray.cc (right):

http://codereview.chromium.org/10235010/diff/25003/ash/system/tray/system_tra...
ash/system/tray/system_tray.cc:742: bubble_ = NULL;
Perhaps it makes sense to have bubble_ as a scoped_ptr now.

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

http://codereview.chromium.org/10235010/diff/25003/ash/system/tray/system_tra...
ash/system/tray/system_tray.h:79: void RemoveBubble(internal::SystemTrayBubble*
bubble);
This should be private/protected with friend'ing Bubble? It doesn't make sense
to have this in the public API.

[stevenjb, 2012-04-30 20:25:59]

http://codereview.chromium.org/10235010/diff/25003/ash/system/tray/system_tra...
File ash/system/tray/system_tray.cc (right):

http://codereview.chromium.org/10235010/diff/25003/ash/system/tray/system_tra...
ash/system/tray/system_tray.cc:742: bubble_ = NULL;
On 2012/04/30 20:01:52, sadrul wrote:
> Perhaps it makes sense to have bubble_ as a scoped_ptr now.
Yes, good call, that makes a lot of sense. Done.

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

http://codereview.chromium.org/10235010/diff/25003/ash/system/tray/system_tra...
ash/system/tray/system_tray.h:79: void RemoveBubble(internal::SystemTrayBubble*
bubble);
On 2012/04/30 20:01:52, sadrul wrote:
> This should be private/protected with friend'ing Bubble? It doesn't make sense
> to have this in the public API.
Sure. Done.

[sadrul, 2012-04-30 21:01:41]

LGTM

http://codereview.chromium.org/10235010/diff/28003/ash/system/tray/system_tra...
File ash/system/tray/system_tray.cc (right):

http://codereview.chromium.org/10235010/diff/28003/ash/system/tray/system_tra...
ash/system/tray/system_tray.cc:362: SystemTrayBubbleView* bubble_view() const {
return bubble_view_; }
This doesn't seem to be used.

[stevenjb, 2012-04-30 21:45:01]

http://codereview.chromium.org/10235010/diff/28003/ash/system/tray/system_tra...
File ash/system/tray/system_tray.cc (right):

http://codereview.chromium.org/10235010/diff/28003/ash/system/tray/system_tra...
ash/system/tray/system_tray.cc:362: SystemTrayBubbleView* bubble_view() const {
return bubble_view_; }
On 2012/04/30 21:01:41, sadrul wrote:
> This doesn't seem to be used.
Good catch. Removed.