Enable users of NotificationUIManager to specify binary images.

Enable users of NotificationUIManager to specify binary images.

This refactors message_center::Notification to hold a class
containing the optional data associated with the
notification.  It also alters chrome/browser/notification so
that you can manually set this.

Ash tests only updated to use new API.

TBR=sky@chromium.org,stevenjb@chromium.org
BUG=227093
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=204181

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

[dewittj, 2013-05-31 00:20:24]

There are a few compile errors porting tests on ChromeOS but I thought that I'd
give you a chance to take a look at this change since it's pretty large.

petewil: synced_notifications*
mukai: all

I would start by looking at the change to message_center/notification.h - this
is the root of all the other changes.  The changes to
message_center_notification_manager are around forcing downloads to happen
before adding the notification to message center, and the changes to
chrome/browser/notification are so that we can support both raw bitmaps and urls
depending on whether we are using balloons or rich notifications.

[Pete Williamson, 2013-05-31 01:19:08]

Synced Notification changes LGTM with a nit.

https://codereview.chromium.org/14631005/diff/36002/chrome/browser/notificati...
File chrome/browser/notifications/sync_notifier/synced_notification.cc (right):

https://codereview.chromium.org/14631005/diff/36002/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/synced_notification.cc:143:
rich_notification_data.icon = icon;
I'd prefer not to check in lines 141-143 and the headers that it needs.

[dewittj, 2013-05-31 16:31:34]

petewil: Took out ResourceBundle.  Additionally, I modified the unit tests to
stop checking icon_url() since it's no longer being set.

https://codereview.chromium.org/14631005/diff/36002/chrome/browser/notificati...
File chrome/browser/notifications/sync_notifier/synced_notification.cc (right):

https://codereview.chromium.org/14631005/diff/36002/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/synced_notification.cc:143:
rich_notification_data.icon = icon;
On 2013/05/31 01:19:08, Pete Williamson wrote:
> I'd prefer not to check in lines 141-143 and the headers that it needs.

Done.

[Jun Mukai, 2013-05-31 17:08:10]

https://codereview.chromium.org/14631005/diff/44002/chrome/browser/notificati...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/14631005/diff/44002/chrome/browser/notificati...
chrome/browser/notifications/message_center_notification_manager.cc:501:
message_center_->UpdateNotification(updated_id_,
It seems that UpdateNotification will still be called even though it's still not
yet added, if the app invokes update before the images are fully downloaded.

https://codereview.chromium.org/14631005/diff/44002/chrome/browser/notificati...
File chrome/browser/notifications/notification.h (right):

https://codereview.chromium.org/14631005/diff/44002/chrome/browser/notificati...
chrome/browser/notifications/notification.h:25: class Notification {
Can we make this class a subclass of message_center::Notification instead of
adding RichNotificationData?  as you did NotificationDelegate?

https://codereview.chromium.org/14631005/diff/44002/ui/message_center/notific...
File ui/message_center/notification.h (right):

https://codereview.chromium.org/14631005/diff/44002/ui/message_center/notific...
ui/message_center/notification.h:45: gfx::Image icon;
Almost all notifications have icons, especially webkit notification will be
created with icon URL.
Probably better to extract image resources here and name it as
NotificationImages or something like that.

[dewittj, 2013-05-31 22:13:55]

I removed the downloading changes to make this patch a little smaller and more
focused.  They are TBD in a future patch.

I am investigating making the notifications related to each other, but there is
an issue with this: message_center takes ownership of Notifications (using
scoped_ptr) while c/b/notification is generally passed around as a const
reference.  This prevents me from making any changes to the object after
creation.  Should I define a copy constructor for message_center notifications
so that I can create new ones when they're added to the center?

https://codereview.chromium.org/14631005/diff/44002/chrome/browser/notificati...
File chrome/browser/notifications/notification.h (right):

https://codereview.chromium.org/14631005/diff/44002/chrome/browser/notificati...
chrome/browser/notifications/notification.h:25: class Notification {
On 2013/05/31 17:08:10, Jun Mukai wrote:
> Can we make this class a subclass of message_center::Notification instead of
> adding RichNotificationData?  as you did NotificationDelegate?

Not easily, since the prevailing style is to pass around const references of
chrome/browser/notification but to pass around scoped_ptr of
ui/message_center/notification.

https://codereview.chromium.org/14631005/diff/44002/ui/message_center/notific...
File ui/message_center/notification.h (right):

https://codereview.chromium.org/14631005/diff/44002/ui/message_center/notific...
ui/message_center/notification.h:45: gfx::Image icon;
On 2013/05/31 17:08:10, Jun Mukai wrote:
> Almost all notifications have icons, especially webkit notification will be
> created with icon URL.
> Probably better to extract image resources here and name it as
> NotificationImages or something like that.

Not sure what you mean.  Do you mean take out icon from optional_fields_? 
Perhaps better to just rename optional_fields_?

[dewittj, 2013-06-01 01:05:01]

PTAL - I made c/b/notification a subclass of ui/mc/notification, leaving it with
mostly just extra URL parameters hanging around.

I did leave the RichNotificationData struct since it's useful for setting all
the notifications data, but I moved the icon out into the class proper (and into
the constructor) to emphasize that icons are required.

[Jun Mukai, 2013-06-02 21:21:23]

you may also want to edit
chrome/browser/extensions/api/notification/notification_api.cc to use
RichNotificationData rather than optional fields.

https://codereview.chromium.org/14631005/diff/105001/chrome/browser/notificat...
File chrome/browser/notifications/notification.h (right):

https://codereview.chromium.org/14631005/diff/105001/chrome/browser/notificat...
chrome/browser/notifications/notification.h:133: // Contains URLs for optional
images.
This comment doesn't sound correct, it still contains the rich notification data
like priority or button text.
If you want to make it correct, probably better to change the field name to
'image_urls_'

https://codereview.chromium.org/14631005/diff/105001/ui/message_center/notifi...
File ui/message_center/notification.h (left):

https://codereview.chromium.org/14631005/diff/105001/ui/message_center/notifi...
ui/message_center/notification.h:124: DISALLOW_COPY_AND_ASSIGN(Notification);
why this needs to be removed? IIRC most of our code handles the pointer to
Notification and doesn't copy it.

[dewittj, 2013-06-03 17:32:26]

I'm not ready to update the API file since it is still using URLs.  That will
come in my next patchset.  This is necessary to unblock synced notifications so
that was the first target for using RichNotificationData.

https://codereview.chromium.org/14631005/diff/105001/chrome/browser/notificat...
File chrome/browser/notifications/notification.h (right):

https://codereview.chromium.org/14631005/diff/105001/chrome/browser/notificat...
chrome/browser/notifications/notification.h:133: // Contains URLs for optional
images.
On 2013/06/02 21:21:23, Jun Mukai wrote:
> This comment doesn't sound correct, it still contains the rich notification
data
> like priority or button text.
> If you want to make it correct, probably better to change the field name to
> 'image_urls_'

Did one better and just saved all the items separately s image_url_ and friends.

https://codereview.chromium.org/14631005/diff/105001/ui/message_center/notifi...
File ui/message_center/notification.h (left):

https://codereview.chromium.org/14631005/diff/105001/ui/message_center/notifi...
ui/message_center/notification.h:124: DISALLOW_COPY_AND_ASSIGN(Notification);
On 2013/06/02 21:21:23, Jun Mukai wrote:
> why this needs to be removed? IIRC most of our code handles the pointer to
> Notification and doesn't copy it.

We need copy and assignment because chrome/browser/Notification (now a subclass
of ui/message_center/Notification) is passed around as const ref and copied as
needed.

[Jun Mukai, 2013-06-03 20:01:27]

lgtm

[Pete Williamson, 2013-06-04 22:13:11]

Changes to the SyncedNotificationTests all LGTM

[commit-bot: I haz the power, 2013-06-04 22:54:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/14631005/134001

[commit-bot: I haz the power, 2013-06-05 06:42:45]

Change committed as 204181

[commit-bot: I haz the power, 2013-06-05 18:08:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/14631005/155001

[commit-bot: I haz the power, 2013-06-06 04:48:59]

Change committed as 204419