Adds a small icon to notifications, and connects it to synced notifications.

Adds a small icon to notifications, and connects it to synced notifications.

The small icon is a 16x16 image in the bottom right of all notification
templates.  Currently it is not exposed to the JS API, but instead is used
only by internal API systems (starting with synced notifications.)

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

[dewittj, 2014-01-29 22:34:09]

petewil: chrome/browser/*
rsesek: ui/message_center/cocoa/*
dimich ui/message_center/*

PTAL, thanks.

[Robert Sesek, 2014-01-29 22:37:48]

cocoa/ LGTM

https://codereview.chromium.org/149433005/diff/40001/ui/message_center/cocoa/...
File ui/message_center/cocoa/notification_controller.mm (right):

https://codereview.chromium.org/149433005/diff/40001/ui/message_center/cocoa/...
ui/message_center/cocoa/notification_controller.mm:203: -
(NSBox*)createImageBox:(gfx::Image)notificationImage;
While it's cheap to copy gfx::Image, you can use a const& here.

[Dmitry Titov, 2014-01-29 23:07:05]

message_center, few nits:

https://codereview.chromium.org/149433005/diff/40001/ui/message_center/messag...
File ui/message_center/message_center.h (right):

https://codereview.chromium.org/149433005/diff/40001/ui/message_center/messag...
ui/message_center/message_center.h:107: virtual void
SetNotificationSmallImage(const std::string& notification_id,
If we don't call this and don't plan to in the near future, it's better not to
add the dead code. It is easy to add it when and if it'll be needed. Consider
removing this method.
Also, in FakeMessageCenter this method goes above SetNotificationButtonIcon()...

https://codereview.chromium.org/149433005/diff/40001/ui/message_center/views/...
File ui/message_center/views/message_view.cc (right):

https://codereview.chromium.org/149433005/diff/40001/ui/message_center/views/...
ui/message_center/views/message_view.cc:58: views::ImageView* small_image = new
views::ImageView();
this one could be small_image_view... if the small_image_skia->small_image

https://codereview.chromium.org/149433005/diff/40001/ui/message_center/views/...
File ui/message_center/views/message_view.h (right):

https://codereview.chromium.org/149433005/diff/40001/ui/message_center/views/...
ui/message_center/views/message_view.h:52: const gfx::ImageSkia&
small_image_skia,
no need to repeat "skia" in the var name...

[Pete Williamson, 2014-01-30 00:00:21]

LGTM with nit

https://codereview.chromium.org/149433005/diff/40001/chrome/browser/notificat...
File chrome/browser/notifications/sync_notifier/synced_notification.cc (right):

https://codereview.chromium.org/149433005/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification.cc:337: if
(!app_icon_bitmap_.IsEmpty()) {
Nit: It would be nice to move this change to line 314 to be more closely
correlated with the surrounding code.

[dewittj, 2014-01-30 00:41:56]

https://codereview.chromium.org/149433005/diff/40001/chrome/browser/notificat...
File chrome/browser/notifications/sync_notifier/synced_notification.cc (right):

https://codereview.chromium.org/149433005/diff/40001/chrome/browser/notificat...
chrome/browser/notifications/sync_notifier/synced_notification.cc:337: if
(!app_icon_bitmap_.IsEmpty()) {
On 2014/01/30 00:00:21, Pete Williamson wrote:
> Nit: It would be nice to move this change to line 314 to be more closely
> correlated with the surrounding code.

Done.

https://codereview.chromium.org/149433005/diff/40001/ui/message_center/cocoa/...
File ui/message_center/cocoa/notification_controller.mm (right):

https://codereview.chromium.org/149433005/diff/40001/ui/message_center/cocoa/...
ui/message_center/cocoa/notification_controller.mm:203: -
(NSBox*)createImageBox:(gfx::Image)notificationImage;
On 2014/01/29 22:37:48, rsesek wrote:
> While it's cheap to copy gfx::Image, you can use a const& here.

Done.

https://codereview.chromium.org/149433005/diff/40001/ui/message_center/messag...
File ui/message_center/message_center.h (right):

https://codereview.chromium.org/149433005/diff/40001/ui/message_center/messag...
ui/message_center/message_center.h:107: virtual void
SetNotificationSmallImage(const std::string& notification_id,
On 2014/01/29 23:07:05, Dmitry Titov wrote:
> If we don't call this and don't plan to in the near future, it's better not to
> add the dead code. It is easy to add it when and if it'll be needed. Consider
> removing this method.
> Also, in FakeMessageCenter this method goes above
SetNotificationButtonIcon()...
> 

Done.

https://codereview.chromium.org/149433005/diff/40001/ui/message_center/views/...
File ui/message_center/views/message_view.cc (right):

https://codereview.chromium.org/149433005/diff/40001/ui/message_center/views/...
ui/message_center/views/message_view.cc:58: views::ImageView* small_image = new
views::ImageView();
On 2014/01/29 23:07:05, Dmitry Titov wrote:
> this one could be small_image_view... if the small_image_skia->small_image

Done.

https://codereview.chromium.org/149433005/diff/40001/ui/message_center/views/...
File ui/message_center/views/message_view.h (right):

https://codereview.chromium.org/149433005/diff/40001/ui/message_center/views/...
ui/message_center/views/message_view.h:52: const gfx::ImageSkia&
small_image_skia,
On 2014/01/29 23:07:05, Dmitry Titov wrote:
> no need to repeat "skia" in the var name...

Done.

[Dmitry Titov, 2014-01-30 00:46:23]

LGTM

[commit-bot: I haz the power, 2014-01-30 00:50:55]

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

[commit-bot: I haz the power, 2014-01-30 02:20:48]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2014-01-30 02:26:03]

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

[commit-bot: I haz the power, 2014-01-30 03:04:41]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-30 03:13:18]

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

[commit-bot: I haz the power, 2014-01-30 07:37:28]

Change committed as 247875

[robwu, 2014-05-22 21:04:16]

While looking for the recommended size of the icon (http://crbug.com/376456), I
found an unused private member.

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

https://codereview.chromium.org/149433005/diff/60001/chrome/browser/notificat...
chrome/browser/notifications/notification.h:129: GURL small_image_url_;
This is not used anywhere, shouldn't it be removed?