Add WebNotificationTray to the status area

Add WebNotificationTray to the status area

This CL:
* Adds WebNotificationTray and its Delegate class for handling web/app notifications in the status area
* Adds hooks to SystemTray so that StatusAreaWidget can hide system notifications when web notifications are viewed


BUG=124914
TEST=WebNotificationTrayTest unittest passes, system tray functionality is unchanged, web notification tray works in ash_shell.

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

[stevenjb, 2012-06-12 18:44:16]

Ready for Review.

Mostly this just adds WebNotificationTray, with some small changes to
StatusAreaWidget to support the additional tray.

[stevenjb, 2012-06-13 16:29:21]

Ping?

[stevenjb, 2012-06-13 16:30:15]

+ben for shell/wm changes

[sadrul, 2012-06-13 16:37:56]

ash/system LGTM (with a couple of nits and questions/comments)

http://codereview.chromium.org/10546125/diff/2001/ash/system/status_area_widg...
File ash/system/status_area_widget.cc (right):

http://codereview.chromium.org/10546125/diff/2001/ash/system/status_area_widg...
ash/system/status_area_widget.cc:350: bool value,
less indentation

http://codereview.chromium.org/10546125/diff/2001/ash/system/status_area_widg...
ash/system/status_area_widget.cc:369: system_tray_->SetHideNotifications(true);
Just to clarify: with this change, both the system-tray bubble and the
web-notification bubbles can be visible at the same time?

http://codereview.chromium.org/10546125/diff/2001/ash/system/web_notification...
File ash/system/web_notification/web_notification_tray.cc (right):

http://codereview.chromium.org/10546125/diff/2001/ash/system/web_notification...
ash/system/web_notification/web_notification_tray.cc:398: virtual void
OnMenuButtonClicked(View* source, const gfx::Point& point) {
OVERRIDE

http://codereview.chromium.org/10546125/diff/2001/ash/system/web_notification...
ash/system/web_notification/web_notification_tray.cc:466: TrayPopupTextButton*
close_all_button_;
DISALLOW_COPY...

http://codereview.chromium.org/10546125/diff/2001/ash/system/web_notification...
ash/system/web_notification/web_notification_tray.cc:529: explicit
Bubble(WebNotificationTray* tray) : tray_(tray),
Perhaps move these in the next line to match with the other classes

http://codereview.chromium.org/10546125/diff/2001/ash/system/web_notification...
ash/system/web_notification/web_notification_tray.cc:548: bubble_widget_ =
views::BubbleDelegateView::CreateBubble(bubble_view_);
Do you need to unset bubble_widget_ when the widget closes 'by itself'
(SystemTrayBubble adds itself as its bubble_widget_'s observer so that it can
clean up when necessary)

http://codereview.chromium.org/10546125/diff/2001/ash/system/web_notification...
ash/system/web_notification/web_notification_tray.cc:583: void Update() {
Move these two functions before the overridden methods

http://codereview.chromium.org/10546125/diff/2001/ash/system/web_notification...
ash/system/web_notification/web_notification_tray.cc:595: BubbleContentsView*
contents_view_;
DISALLOW_COPY_..

http://codereview.chromium.org/10546125/diff/2001/ash/system/web_notification...
ash/system/web_notification/web_notification_tray.cc:616:
Shell::GetInstance()->AddEnvEventFilter(this);
The event-filtering stuff is very similar to the same stuff in SystemTrayBubble.
It would be good to refactor this (perhaps into Host). But that's a different
CL.

http://codereview.chromium.org/10546125/diff/2001/ash/system/web_notification...
ash/system/web_notification/web_notification_tray.cc:663: for
(WebNotificationList::Notifications::const_iterator loopiter
perhaps move 'loopiter' in the next line

http://codereview.chromium.org/10546125/diff/2001/ash/system/web_notification...
ash/system/web_notification/web_notification_tray.cc:721: aura::KeyEvent* event)
{
indent is off (and in lines 726, 733)

http://codereview.chromium.org/10546125/diff/2001/ash/system/web_notification...
File ash/system/web_notification/web_notification_tray.h (right):

http://codereview.chromium.org/10546125/diff/2001/ash/system/web_notification...
ash/system/web_notification/web_notification_tray.h:80: const std::string&
extension);
What are |source| and |extension|?

[Ben Goodger (Google), 2012-06-13 16:46:10]

two things:

- can you add a button to the window type launcher in ash_shell to add a
notification each time it's pressed?
- does the notification tray depend on the system tray? if so, why?

[stevenjb, 2012-06-13 19:05:02]

http://codereview.chromium.org/10546125/diff/2001/ash/system/status_area_widg...
File ash/system/status_area_widget.cc (right):

http://codereview.chromium.org/10546125/diff/2001/ash/system/status_area_widg...
ash/system/status_area_widget.cc:350: bool value,
On 2012/06/13 16:37:56, sadrul wrote:
> less indentation

Done.

http://codereview.chromium.org/10546125/diff/2001/ash/system/status_area_widg...
ash/system/status_area_widget.cc:369: system_tray_->SetHideNotifications(true);
On 2012/06/13 16:37:56, sadrul wrote:
> Just to clarify: with this change, both the system-tray bubble and the
> web-notification bubbles can be visible at the same time?

No, only one should be visible at a time, but that's not very clear from the
logic, and the logic actually has a flaw. As written it should be 'if
(user_action != USER_ACTION' above. A user action will always close the system
tray bubble, but we should make that explicit here. I will change the logic.

http://codereview.chromium.org/10546125/diff/2001/ash/system/web_notification...
File ash/system/web_notification/web_notification_tray.cc (right):

http://codereview.chromium.org/10546125/diff/2001/ash/system/web_notification...
ash/system/web_notification/web_notification_tray.cc:398: virtual void
OnMenuButtonClicked(View* source, const gfx::Point& point) {
On 2012/06/13 16:37:56, sadrul wrote:
> OVERRIDE

Done.

http://codereview.chromium.org/10546125/diff/2001/ash/system/web_notification...
ash/system/web_notification/web_notification_tray.cc:466: TrayPopupTextButton*
close_all_button_;
On 2012/06/13 16:37:56, sadrul wrote:
> DISALLOW_COPY...

Done.

http://codereview.chromium.org/10546125/diff/2001/ash/system/web_notification...
ash/system/web_notification/web_notification_tray.cc:529: explicit
Bubble(WebNotificationTray* tray) : tray_(tray),
On 2012/06/13 16:37:56, sadrul wrote:
> Perhaps move these in the next line to match with the other classes

Done.

http://codereview.chromium.org/10546125/diff/2001/ash/system/web_notification...
ash/system/web_notification/web_notification_tray.cc:548: bubble_widget_ =
views::BubbleDelegateView::CreateBubble(bubble_view_);
On 2012/06/13 16:37:56, sadrul wrote:
> Do you need to unset bubble_widget_ when the widget closes 'by itself'
> (SystemTrayBubble adds itself as its bubble_widget_'s observer so that it can
> clean up when necessary)

Yes... I'm not sure how that might happen, but we should probably guard against
it. done.

http://codereview.chromium.org/10546125/diff/2001/ash/system/web_notification...
ash/system/web_notification/web_notification_tray.cc:583: void Update() {
On 2012/06/13 16:37:56, sadrul wrote:
> Move these two functions before the overridden methods

Done.

http://codereview.chromium.org/10546125/diff/2001/ash/system/web_notification...
ash/system/web_notification/web_notification_tray.cc:595: BubbleContentsView*
contents_view_;
On 2012/06/13 16:37:56, sadrul wrote:
> DISALLOW_COPY_..

Done.

http://codereview.chromium.org/10546125/diff/2001/ash/system/web_notification...
ash/system/web_notification/web_notification_tray.cc:616:
Shell::GetInstance()->AddEnvEventFilter(this);
On 2012/06/13 16:37:56, sadrul wrote:
> The event-filtering stuff is very similar to the same stuff in
SystemTrayBubble.
> It would be good to refactor this (perhaps into Host). But that's a different
> CL.
Agreed.

http://codereview.chromium.org/10546125/diff/2001/ash/system/web_notification...
ash/system/web_notification/web_notification_tray.cc:663: for
(WebNotificationList::Notifications::const_iterator loopiter
On 2012/06/13 16:37:56, sadrul wrote:
> perhaps move 'loopiter' in the next line
Moved the = instead to be more consistent with multi-line assignment. Also moved
end check to its own line.

http://codereview.chromium.org/10546125/diff/2001/ash/system/web_notification...
ash/system/web_notification/web_notification_tray.cc:721: aura::KeyEvent* event)
{
On 2012/06/13 16:37:56, sadrul wrote:
> indent is off (and in lines 726, 733)

Done.

http://codereview.chromium.org/10546125/diff/2001/ash/system/web_notification...
File ash/system/web_notification/web_notification_tray.h (right):

http://codereview.chromium.org/10546125/diff/2001/ash/system/web_notification...
ash/system/web_notification/web_notification_tray.h:80: const std::string&
extension);
On 2012/06/13 16:37:56, sadrul wrote:
> What are |source| and |extension|?
Yeah, this comment is rather lacking. Much more descriptive comment added.

http://codereview.chromium.org/10546125/diff/7002/ash/system/tray/system_tray...
File ash/system/tray/system_tray_bubble.cc (right):

http://codereview.chromium.org/10546125/diff/7002/ash/system/tray/system_tray...
ash/system/tray/system_tray_bubble.cc:301: bubble_view_ = NULL;
I added this here also, just to be safe.

http://codereview.chromium.org/10546125/diff/7002/ash/system/web_notification...
File ash/system/web_notification/web_notification_tray.cc (right):

http://codereview.chromium.org/10546125/diff/7002/ash/system/web_notification...
ash/system/web_notification/web_notification_tray.cc:603: virtual void
OnWidgetClosing(views::Widget* widget) OVERRIDE {
Added this.

[stevenjb, 2012-06-13 19:06:22]

On 2012/06/13 16:46:10, Ben Goodger (Google) wrote:
> two things:
> 
> - can you add a button to the window type launcher in ash_shell to add a
> notification each time it's pressed?
There should already be one added, in ash/shell/window_type_launcher.cc.

> - does the notification tray depend on the system tray? if so, why?
It does not. Right now all of the status area code is in ash/system. We should
consider renaming the directory (but probably not in this CL).

[stevenjb, 2012-06-13 19:07:30]

On 2012/06/13 19:06:22, stevenjb (chromium) wrote:
> On 2012/06/13 16:46:10, Ben Goodger (Google) wrote:
> > two things:
> > 
> > - can you add a button to the window type launcher in ash_shell to add a
> > notification each time it's pressed?
> There should already be one added, in ash/shell/window_type_launcher.cc.
> 
> > - does the notification tray depend on the system tray? if so, why?
> It does not. Right now all of the status area code is in ash/system. We should
> consider renaming the directory (but probably not in this CL).

To clarify that, both SystemTray and WebNotificationTray are owned by
StatusAreaWidget (ash/system/status_area_widget.cc).

[Ben Goodger (Google), 2012-06-13 21:04:33]

LGTM

On Wed, Jun 13, 2012 at 12:07 PM, <stevenjb@chromium.org> wrote:

> On 2012/06/13 19:06:22, stevenjb (chromium) wrote:
>
>> On 2012/06/13 16:46:10, Ben Goodger (Google) wrote:
>> > two things:
>> >
>> > - can you add a button to the window type launcher in ash_shell to add a
>> > notification each time it's pressed?
>> There should already be one added, in ash/shell/window_type_**
>> launcher.cc.
>>
>
>  > - does the notification tray depend on the system tray? if so, why?
>> It does not. Right now all of the status area code is in ash/system. We
>> should
>> consider renaming the directory (but probably not in this CL).
>>
>
> To clarify that, both SystemTray and WebNotificationTray are owned by
> StatusAreaWidget (ash/system/status_area_**widget.cc).
>
>
>
http://codereview.chromium.**org/10546125/<http://codereview.chromium.org/105...
>