Add WebNotificationTray

Add WebNotificationTray.
This class will support notifications for web pages and apps.
It is designed to be part of the status area, located next to the system tray.
Design doc is here: http://code.google.com/p/chromium/issues/detail?id=124914

This CL includes unit tests and ash_shell integration.
Chrome integration will follow.

BUG=124914
TEST=aura_shell_unittests --gtest_filter=WebNotificationTrayTest*

REPLACED BY http://codereview.chromium.org/10546125/

[stevenjb, 2012-06-04 22:53:13]

This adds WebNotificationTray to ash_shell and includes some unittests.
Chrome integration will follow.

[oshima, 2012-06-05 22:59:46]

http://codereview.chromium.org/10514008/diff/8001/ui/resources/ui_resources_s...
File ui/resources/ui_resources_standard.grd (right):

http://codereview.chromium.org/10514008/diff/8001/ui/resources/ui_resources_s...
ui/resources/ui_resources_standard.grd:160: <include
name="IDR_AURA_UBER_TRAY_WEB_NOTIFICATON"
file="default_100_percent/aura/statusbar_notifications.png" type="BINDATA" />
please add this to ui_resources_2x.grd, ui_resources_touch_2x.rg as well. (Sorry
for mess. We're working on fixing this)

[stevenjb, 2012-06-06 17:28:12]

Please review at your earliest convenience, we would like to enable the new
notifications by default by the end of the week (there will be one more (small)
integration CL after this one).

[oshima, 2012-06-06 18:10:51]

looks like this no longer contains resource change. removing myself.

[sadrul, 2012-06-06 19:19:18]

Reviewed some of the changes (please note that a few comments are in patchset3,
but they apply to patchset4 too).

This CL does a lot more than add WebNotificationTray. Can you please explain the
changes in the CL description (e.g. X moves out of Y, Z becomes W and A goes
through B to C etc.). That would make the review easier too.

http://codereview.chromium.org/10514008/diff/8001/ash/system/status_area_widg...
File ash/system/status_area_widget_delegate.h (right):

http://codereview.chromium.org/10514008/diff/8001/ash/system/status_area_widg...
ash/system/status_area_widget_delegate.h:44: void set_alignment(ShelfAlignment
alignment) { alignment_ = alignment; }
Move these above the overridden functions

http://codereview.chromium.org/10514008/diff/8001/ash/system/status_area_widg...
ash/system/status_area_widget_delegate.h:48: virtual void
ChildPreferredSizeChanged(View* child);
OVERRIDE

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

http://codereview.chromium.org/10514008/diff/8001/ash/system/tray/system_tray...
ash/system/tray/system_tray.h:58: virtual void SetShelfAlignment(ShelfAlignment
alignment) OVERRIDE;
Move this after the non-overridden methods

http://codereview.chromium.org/10514008/diff/13001/ash/system/web_notificatio...
File ash/system/web_notification/web_notification_tray.cc (right):

http://codereview.chromium.org/10514008/diff/13001/ash/system/web_notificatio...
ash/system/web_notification/web_notification_tray.cc:66: const std::string& e) :
the ':' should be in the next line

http://codereview.chromium.org/10514008/diff/13001/ash/system/web_notificatio...
ash/system/web_notification/web_notification_tray.cc:73: std::string id;
newline between 72/73

http://codereview.chromium.org/10514008/diff/13001/ash/system/web_notificatio...
ash/system/web_notification/web_notification_tray.cc:120: loopiter !=
notifications_.end(); ) {
Can you just ++loopiter here instead of below?

.. oh, because of the erase call below?

http://codereview.chromium.org/10514008/diff/13001/ash/system/web_notificatio...
ash/system/web_notification/web_notification_tray.cc:192: const int
kShowSettingsCommand = 2;
These could be in the anonymous namespace

http://codereview.chromium.org/10514008/diff/13001/ash/system/web_notificatio...
ash/system/web_notification/web_notification_tray.cc:386: ==
views::MenuRunner::MENU_DELETED) {
== should be in the line above

http://codereview.chromium.org/10514008/diff/13001/ash/system/web_notificatio...
ash/system/web_notification/web_notification_tray.cc:458: explicit
Bubble(WebNotificationTray* tray) :
: should be in the next line

http://codereview.chromium.org/10514008/diff/13001/ash/system/web_notificatio...
ash/system/web_notification/web_notification_tray.cc:498: ~Bubble() {
virtual?

http://codereview.chromium.org/10514008/diff/13001/ash/system/web_notificatio...
ash/system/web_notification/web_notification_tray.cc:619: bubble_.reset(new
Bubble(this));
This seems unfortunate. It would be nice to say 'show this bubble', and then
something else can coordinate between the two bubbles. So for example, if the
'main' bubble is visible, and a notification comes in, this will just create the
bubble here. So the 'BubbleVisibilityController' [sic] will notice that the
'main' bubble is up, so it won't display this notification bubble until the
other bubble closes.

http://codereview.chromium.org/10514008/diff/13001/ash/system/web_notificatio...
ash/system/web_notification/web_notification_tray.cc:634: if (bubble())
if (notification_list_ ... && bubble())
  bubble_->Update()
?

[jennyz, 2012-06-06 21:04:38]

Some nits.

http://codereview.chromium.org/10514008/diff/20002/ash/system/tray/system_tra...
File ash/system/tray/system_tray_bubble.h (right):

http://codereview.chromium.org/10514008/diff/20002/ash/system/tray/system_tra...
ash/system/tray/system_tray_bubble.h:73: virtual void OnMoiseExitedView()
OVERRIDE;
Typo: OnMoiseExitedView->OnMouseExitedView

http://codereview.chromium.org/10514008/diff/20002/ash/system/tray/system_tra...
ash/system/tray/system_tray_bubble.h:84: bool IsVisible() { return
bubble_widget_ && bubble_widget_->IsVisible(); }
Should this be moved to system_tray_bubble.cc file?

http://codereview.chromium.org/10514008/diff/20002/ash/system/tray/system_tra...
File ash/system/tray/system_tray_bubble_view.h (right):

http://codereview.chromium.org/10514008/diff/20002/ash/system/tray/system_tra...
ash/system/tray/system_tray_bubble_view.h:26: virtual void OnMoiseExitedView() =
0;
Typo: Moise->Mouse

[stevenjb, 2012-06-06 23:12:14]

Moving notification visibility control to StatusAreaWidget exposed some issues
in tests where we were accessing the ststem tray or web notification tray
created in Shell rather than the one created in the test. This led to some non
trivial changes moving control of the tray views to StatusAreaWidget, but I
think this approach is cleaner and more maintainable. Also it moves a bunch of
code out of shell.cc which seems a good thing.

http://codereview.chromium.org/10514008/diff/8001/ash/system/status_area_widg...
File ash/system/status_area_widget_delegate.h (right):

http://codereview.chromium.org/10514008/diff/8001/ash/system/status_area_widg...
ash/system/status_area_widget_delegate.h:44: void set_alignment(ShelfAlignment
alignment) { alignment_ = alignment; }
On 2012/06/06 19:19:18, sadrul wrote:
> Move these above the overridden functions

Done.

http://codereview.chromium.org/10514008/diff/8001/ash/system/status_area_widg...
ash/system/status_area_widget_delegate.h:48: virtual void
ChildPreferredSizeChanged(View* child);
On 2012/06/06 19:19:18, sadrul wrote:
> OVERRIDE

Done.

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

http://codereview.chromium.org/10514008/diff/8001/ash/system/tray/system_tray...
ash/system/tray/system_tray.h:58: virtual void SetShelfAlignment(ShelfAlignment
alignment) OVERRIDE;
On 2012/06/06 19:19:18, sadrul wrote:
> Move this after the non-overridden methods

Done.

http://codereview.chromium.org/10514008/diff/8001/ui/resources/ui_resources_s...
File ui/resources/ui_resources_standard.grd (right):

http://codereview.chromium.org/10514008/diff/8001/ui/resources/ui_resources_s...
ui/resources/ui_resources_standard.grd:160: <include
name="IDR_AURA_UBER_TRAY_WEB_NOTIFICATON"
file="default_100_percent/aura/statusbar_notifications.png" type="BINDATA" />
On 2012/06/05 22:59:46, oshima wrote:
> please add this to ui_resources_2x.grd, ui_resources_touch_2x.rg as well.
(Sorry
> for mess. We're working on fixing this)

Done.

http://codereview.chromium.org/10514008/diff/13001/ash/system/web_notificatio...
File ash/system/web_notification/web_notification_tray.cc (right):

http://codereview.chromium.org/10514008/diff/13001/ash/system/web_notificatio...
ash/system/web_notification/web_notification_tray.cc:66: const std::string& e) :
On 2012/06/06 19:19:19, sadrul wrote:
> the ':' should be in the next line

Done.

http://codereview.chromium.org/10514008/diff/13001/ash/system/web_notificatio...
ash/system/web_notification/web_notification_tray.cc:73: std::string id;
On 2012/06/06 19:19:19, sadrul wrote:
> newline between 72/73

Done.

http://codereview.chromium.org/10514008/diff/13001/ash/system/web_notificatio...
ash/system/web_notification/web_notification_tray.cc:120: loopiter !=
notifications_.end(); ) {
On 2012/06/06 19:19:19, sadrul wrote:
> Can you just ++loopiter here instead of below?
> 
> .. oh, because of the erase call below?
Correct.

http://codereview.chromium.org/10514008/diff/13001/ash/system/web_notificatio...
ash/system/web_notification/web_notification_tray.cc:192: const int
kShowSettingsCommand = 2;
On 2012/06/06 19:19:19, sadrul wrote:
> These could be in the anonymous namespace

Done.

http://codereview.chromium.org/10514008/diff/13001/ash/system/web_notificatio...
ash/system/web_notification/web_notification_tray.cc:386: ==
views::MenuRunner::MENU_DELETED) {
On 2012/06/06 19:19:19, sadrul wrote:
> == should be in the line above
Used ignore_result instead.

http://codereview.chromium.org/10514008/diff/13001/ash/system/web_notificatio...
ash/system/web_notification/web_notification_tray.cc:458: explicit
Bubble(WebNotificationTray* tray) :
On 2012/06/06 19:19:19, sadrul wrote:
> : should be in the next line

Done.

http://codereview.chromium.org/10514008/diff/13001/ash/system/web_notificatio...
ash/system/web_notification/web_notification_tray.cc:498: ~Bubble() {
On 2012/06/06 19:19:19, sadrul wrote:
> virtual?
Oops! Done.

http://codereview.chromium.org/10514008/diff/13001/ash/system/web_notificatio...
ash/system/web_notification/web_notification_tray.cc:619: bubble_.reset(new
Bubble(this));
On 2012/06/06 19:19:19, sadrul wrote:
> This seems unfortunate. It would be nice to say 'show this bubble', and then
> something else can coordinate between the two bubbles. So for example, if the
> 'main' bubble is visible, and a notification comes in, this will just create
the
> bubble here. So the 'BubbleVisibilityController' [sic] will notice that the
> 'main' bubble is up, so it won't display this notification bubble until the
> other bubble closes.
I can put this in the status area. I was going to do that, but I thought with
only two we didn't need the added complexity, but I agree that the logic would
be better centralized.

http://codereview.chromium.org/10514008/diff/13001/ash/system/web_notificatio...
ash/system/web_notification/web_notification_tray.cc:634: if (bubble())
On 2012/06/06 19:19:19, sadrul wrote:
> if (notification_list_ ... && bubble())
>   bubble_->Update()
> ?
I personally dislike combining functions that do something with simple tests,
but changed to use an early exit instead.

http://codereview.chromium.org/10514008/diff/20002/ash/system/tray/system_tra...
File ash/system/tray/system_tray_bubble.h (right):

http://codereview.chromium.org/10514008/diff/20002/ash/system/tray/system_tra...
ash/system/tray/system_tray_bubble.h:73: virtual void OnMoiseExitedView()
OVERRIDE;
On 2012/06/06 21:04:38, jennyz wrote:
> Typo: OnMoiseExitedView->OnMouseExitedView
Yikes! Nice catch, thanks!

http://codereview.chromium.org/10514008/diff/20002/ash/system/tray/system_tra...
ash/system/tray/system_tray_bubble.h:84: bool IsVisible() { return
bubble_widget_ && bubble_widget_->IsVisible(); }
On 2012/06/06 21:04:38, jennyz wrote:
> Should this be moved to system_tray_bubble.cc file?

Done.

http://codereview.chromium.org/10514008/diff/20002/ash/system/tray/system_tra...
File ash/system/tray/system_tray_bubble_view.h (right):

http://codereview.chromium.org/10514008/diff/20002/ash/system/tray/system_tra...
ash/system/tray/system_tray_bubble_view.h:26: virtual void OnMoiseExitedView() =
0;
On 2012/06/06 21:04:38, jennyz wrote:
> Typo: Moise->Mouse

Done.

[stevenjb, 2012-06-06 23:12:32]

PTAL

[jennyz, 2012-06-07 01:12:20]

lgtm

[jennyz, 2012-06-07 01:12:21]

lgtm

[sadrul, 2012-06-07 14:10:06]

Hi! Please update the CL description a bit more? That would help with the
review.

[stevenjb (google-dont-use), 2012-06-07 16:14:56]

Added some more info, including a link to the design doc.