Add support for Ash to Notifications

Add support for Ash to Notifications
* Adds BalloonViewAsh and moves/renames related classes.
* Adds support for image icons to Notification class
* Adds GetRenderViewHost() method to NotificationDelegate for fetching images

BUG=124914
TEST=Tests pass. No visual changes to notifications on non-ash platforms. Web notifications show up in new tray on Ash with --ash-notify.

For CloudPrintProxyService:
TBR=scottbyer@chromium.org

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

[stevenjb, 2012-06-13 23:18:41]

I broke this up into two pieces. It's still a bit large and ugly, apologies for
that. Added myself to OWNERS since johnnyg@chromium.org is no longer working on
Chrome, and nobody else seems to want much to do with them :)

[oshima, 2012-06-13 23:58:02]

http://codereview.chromium.org/10537158/diff/1/chrome/browser/notifications/n...
File chrome/browser/notifications/notification.h (right):

http://codereview.chromium.org/10537158/diff/1/chrome/browser/notifications/n...
chrome/browser/notifications/notification.h:51: NotificationDelegate* delegate);
just q. don't we need to guard using USE_ASH?

http://codereview.chromium.org/10537158/diff/1/chrome/browser/ui/views/ash/ba...
File chrome/browser/ui/views/ash/balloon_collection_impl_ash.cc (right):

http://codereview.chromium.org/10537158/diff/1/chrome/browser/ui/views/ash/ba...
chrome/browser/ui/views/ash/balloon_collection_impl_ash.cc:24:
ash::switches::kAshNotify)) {
how about defining a utility function like IsAshNotifyEnabled() instead of
checking the switch everywhere? (I don't mind globally or locally)

http://codereview.chromium.org/10537158/diff/1/chrome/browser/ui/views/ash/ba...
chrome/browser/ui/views/ash/balloon_collection_impl_ash.cc:46: return;  // HTML
notifications are not supported in Ash.
oh, what will happen to gmail/calendar notifications?

http://codereview.chromium.org/10537158/diff/1/chrome/browser/ui/views/ash/ba...
File chrome/browser/ui/views/ash/balloon_collection_impl_ash.h (right):

http://codereview.chromium.org/10537158/diff/1/chrome/browser/ui/views/ash/ba...
chrome/browser/ui/views/ash/balloon_collection_impl_ash.h:28: //
WebNotificationTray::Delegate interface.
Overridden from XX, or change the above for consistency.

http://codereview.chromium.org/10537158/diff/1/chrome/browser/ui/views/ash/ba...
File chrome/browser/ui/views/ash/balloon_view_ash.cc (right):

http://codereview.chromium.org/10537158/diff/1/chrome/browser/ui/views/ash/ba...
chrome/browser/ui/views/ash/balloon_view_ash.cc:93:
web_notification_tray()->AddNotification(
how about defining accessor function to WebNotificationTray locally?

[stevenjb, 2012-06-14 04:27:35]

PTAL

http://codereview.chromium.org/10537158/diff/1/chrome/browser/notifications/n...
File chrome/browser/notifications/notification.h (right):

http://codereview.chromium.org/10537158/diff/1/chrome/browser/notifications/n...
chrome/browser/notifications/notification.h:51: NotificationDelegate* delegate);
On 2012/06/13 23:58:02, oshima wrote:
> just q. don't we need to guard using USE_ASH?
We don't need to, there are no ash dependencies. We could, but I thought it
would be better not to add the dependency, even though this will be unused on
other platforms.

http://codereview.chromium.org/10537158/diff/1/chrome/browser/ui/views/ash/ba...
File chrome/browser/ui/views/ash/balloon_collection_impl_ash.cc (right):

http://codereview.chromium.org/10537158/diff/1/chrome/browser/ui/views/ash/ba...
chrome/browser/ui/views/ash/balloon_collection_impl_ash.cc:24:
ash::switches::kAshNotify)) {
On 2012/06/13 23:58:02, oshima wrote:
> how about defining a utility function like IsAshNotifyEnabled() instead of
> checking the switch everywhere? (I don't mind globally or locally)
Good suggestion. Done.

http://codereview.chromium.org/10537158/diff/1/chrome/browser/ui/views/ash/ba...
chrome/browser/ui/views/ash/balloon_collection_impl_ash.cc:46: return;  // HTML
notifications are not supported in Ash.
On 2012/06/13 23:58:02, oshima wrote:
> oh, what will happen to gmail/calendar notifications?

gmail/cal/talk all use title+text+icon based notifications, not html. There was
an explicit decision to deprecate support in ChromeOS for html based
notifications. (There are two apis, see
http://www.html5rocks.com/en/tutorials/notifications/quick/)

http://codereview.chromium.org/10537158/diff/1/chrome/browser/ui/views/ash/ba...
File chrome/browser/ui/views/ash/balloon_collection_impl_ash.h (right):

http://codereview.chromium.org/10537158/diff/1/chrome/browser/ui/views/ash/ba...
chrome/browser/ui/views/ash/balloon_collection_impl_ash.h:28: //
WebNotificationTray::Delegate interface.
On 2012/06/13 23:58:02, oshima wrote:
> Overridden from XX, or change the above for consistency.

Done.

http://codereview.chromium.org/10537158/diff/1/chrome/browser/ui/views/ash/ba...
File chrome/browser/ui/views/ash/balloon_view_ash.cc (right):

http://codereview.chromium.org/10537158/diff/1/chrome/browser/ui/views/ash/ba...
chrome/browser/ui/views/ash/balloon_view_ash.cc:93:
web_notification_tray()->AddNotification(
On 2012/06/13 23:58:02, oshima wrote:
> how about defining accessor function to WebNotificationTray locally?
Also a good idea. Done.

[oshima, 2012-06-14 16:44:44]

lgtm

http://codereview.chromium.org/10537158/diff/7002/chrome/browser/notification...
File chrome/browser/notifications/OWNERS (right):

http://codereview.chromium.org/10537158/diff/7002/chrome/browser/notification...
chrome/browser/notifications/OWNERS:1: johnnyg@chromium.org
optional: I heard that he's no longer working on chrome. If that's the case, I'd
suggest to remove him from the list.

[stevenjb, 2012-06-14 17:41:31]

http://codereview.chromium.org/10537158/diff/7002/chrome/browser/notification...
File chrome/browser/notifications/OWNERS (right):

http://codereview.chromium.org/10537158/diff/7002/chrome/browser/notification...
chrome/browser/notifications/OWNERS:1: johnnyg@chromium.org
On 2012/06/14 16:44:44, oshima wrote:
> optional: I heard that he's no longer working on chrome. If that's the case,
I'd
> suggest to remove him from the list.

Done.

[stevenjb, 2012-06-14 17:42:36]

Ben, can I get a quick OWNERS review of chrome/browser/ui/views/ash? Thanks!

[Ben Goodger (Google), 2012-06-14 17:47:19]

lgtm