Add appIconMask to notification API

Add app icon masp to notification API

Add an app icon mask (16*16, or 32*32 for retina display) on the bottom right corner of notifications to indicate source of the notification. The app icon is displayed in a way that takes only the alpha channel of the image passed in, masks it with white foreground and grey background.

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

[Pete Williamson, 2014-06-03 22:43:06]

Looks good to me so far, a couple of notes:

Where you've used 16 below will have to change when we add high dpi bitmaps, it
will be sometimes 16 and sometimes 32 depending on the OS and environment.

Also, it might be good to work on and test in a fresh branch hooking up the API.
 I can show you how.

[not at google - send to devlin, 2014-06-03 22:53:43]

BUG?

[liyanhou, 2014-06-03 23:02:33]

On 2014/06/03 22:53:43, kalman wrote:
> BUG?

Bug number added

[Pete Williamson, 2014-06-03 23:23:22]

On 2014/06/03 22:43:06, Pete Williamson wrote:
> Looks good to me so far, a couple of notes:
> 
> Where you've used 16 below will have to change when we add high dpi bitmaps,
it
> will be sometimes 16 and sometimes 32 depending on the OS and environment.
> 
> Also, it might be good to work on and test in a fresh branch hooking up the
API.
>  I can show you how.

My bad, it looks like you  don't need to do any additional work in another
branch, this seems to fix everything.

[dewittj, 2014-06-09 21:30:58]

mostly looks fine as is, just a single comment about the code.

As we discussed we will need to do a few things before this lands:
* API update review
* Visual design updates
* Fix the location of the small icon in non-standard layouts like image format
etc.

https://codereview.chromium.org/308053004/diff/90001/chrome/browser/extension...
File chrome/browser/extensions/api/notifications/notifications_api.cc (right):

https://codereview.chromium.org/308053004/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/notifications/notifications_api.cc:292:
NotificationBitmapToGfxImage(image_scale,
We should handle the case where this returns false by providing an error message
just like the one above:

if (!NotificationBitmapToGfxImage(...))
  SetError(...)

[liyanhou, 2014-06-16 16:54:53]

On 2014/06/09 21:30:58, dewittj wrote:
> mostly looks fine as is, just a single comment about the code.
> 
> As we discussed we will need to do a few things before this lands:
> * API update review

As we discussed, this would be at the same time as the first round of reviews.

> * Visual design updates

Done.

> * Fix the location of the small icon in non-standard layouts like image format
> etc.

This will be in a separate CL.

> 
>
https://codereview.chromium.org/308053004/diff/90001/chrome/browser/extension...
> File chrome/browser/extensions/api/notifications/notifications_api.cc (right):
> 
>
https://codereview.chromium.org/308053004/diff/90001/chrome/browser/extension...
> chrome/browser/extensions/api/notifications/notifications_api.cc:292:
> NotificationBitmapToGfxImage(image_scale,
> We should handle the case where this returns false by providing an error
message
> just like the one above:
> 
> if (!NotificationBitmapToGfxImage(...))
>   SetError(...)

[liyanhou, 2014-06-16 16:57:00]

PTAL. Thank you!

https://codereview.chromium.org/308053004/diff/90001/chrome/browser/extension...
File chrome/browser/extensions/api/notifications/notifications_api.cc (right):

https://codereview.chromium.org/308053004/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/notifications/notifications_api.cc:292:
NotificationBitmapToGfxImage(image_scale,
On 2014/06/09 21:30:58, dewittj wrote:
> We should handle the case where this returns false by providing an error
message
> just like the one above:
> 
> if (!NotificationBitmapToGfxImage(...))
>   SetError(...)
> 

Done.

[dewittj, 2014-06-16 18:09:55]

https://codereview.chromium.org/308053004/diff/150001/chrome/common/extension...
File chrome/common/extensions/api/notifications.idl (right):

https://codereview.chromium.org/308053004/diff/150001/chrome/common/extension...
chrome/common/extensions/api/notifications.idl:61: // Small icon
Can you explain about the alpha mask here and where the icon will be rendered

https://codereview.chromium.org/308053004/diff/150001/ui/message_center/views...
File ui/message_center/views/small_image_mask.cc (right):

https://codereview.chromium.org/308053004/diff/150001/ui/message_center/views...
ui/message_center/views/small_image_mask.cc:11: const gfx::ImageSkia
GetMaskedSmallImage(const gfx::ImageSkia& small_image) {
I would just move this into notification_view.cc inside an anonymous namespace,
since there is only one consumer.  Not worth a new file here.

https://codereview.chromium.org/308053004/diff/150001/ui/message_center/views...
ui/message_center/views/small_image_mask.cc:16: const gfx::ImageSkia background
= CreateImage(width, height, 163, 163, 163);
I would suggest you use an SkColor in these functions instead.  See
SkBitmap::eraseColor (in fact eraseRGB is deprecated anyway).

Also, any constants you use should be defined in message_center_style.h so that
they can be used in the Mac version as well.  Define them something like this:

const SkColor kSmallImageMaskForegraound = SkColor...
const SkColor kSmallImageMaskBackground = SkColorWHITE;

You'll see other examples in message_center_style.

[dewittj, 2014-06-26 16:25:01]

please update the API based on comments in API review.

[liyanhou, 2014-06-26 22:44:54]

On 2014/06/16 18:09:55, dewittj wrote:
>
https://codereview.chromium.org/308053004/diff/150001/chrome/common/extension...
> File chrome/common/extensions/api/notifications.idl (right):
> 
>
https://codereview.chromium.org/308053004/diff/150001/chrome/common/extension...
> chrome/common/extensions/api/notifications.idl:61: // Small icon
> Can you explain about the alpha mask here and where the icon will be rendered
> 

Done.

>
https://codereview.chromium.org/308053004/diff/150001/ui/message_center/views...
> File ui/message_center/views/small_image_mask.cc (right):
> 
>
https://codereview.chromium.org/308053004/diff/150001/ui/message_center/views...
> ui/message_center/views/small_image_mask.cc:11: const gfx::ImageSkia
> GetMaskedSmallImage(const gfx::ImageSkia& small_image) {
> I would just move this into notification_view.cc inside an anonymous
namespace,
> since there is only one consumer.  Not worth a new file here.
> 

Done.

>
https://codereview.chromium.org/308053004/diff/150001/ui/message_center/views...
> ui/message_center/views/small_image_mask.cc:16: const gfx::ImageSkia
background
> = CreateImage(width, height, 163, 163, 163);
> I would suggest you use an SkColor in these functions instead.  See
> SkBitmap::eraseColor (in fact eraseRGB is deprecated anyway).
> 
> Also, any constants you use should be defined in message_center_style.h so
that
> they can be used in the Mac version as well.  Define them something like this:
> 
> const SkColor kSmallImageMaskForegraound = SkColor...
> const SkColor kSmallImageMaskBackground = SkColorWHITE;
> 
> You'll see other examples in message_center_style.

Done.

[liyanhou, 2014-06-26 22:45:09]

On 2014/06/26 16:25:01, dewittj wrote:
> please update the API based on comments in API review.

Done.

[Pete Williamson, 2014-06-26 22:50:04]

Adding Dmitry to the CC list

[dewittj, 2014-06-27 02:43:56]

lgtm, one nit.  Please add kalman@ as a reviewer, he will review and lgtm once
he's happy and the apps-dev list thread concerns have been resolved.

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

https://codereview.chromium.org/308053004/diff/200001/ui/message_center/views...
ui/message_center/views/message_view.cc:37: namespace {
Just combine the two unnamed namespaces

[liyanhou, 2014-06-27 03:18:40]

PTAL. Thank you!

https://codereview.chromium.org/308053004/diff/150001/chrome/common/extension...
File chrome/common/extensions/api/notifications.idl (right):

https://codereview.chromium.org/308053004/diff/150001/chrome/common/extension...
chrome/common/extensions/api/notifications.idl:61: // Small icon
On 2014/06/16 18:09:55, dewittj wrote:
> Can you explain about the alpha mask here and where the icon will be rendered

Done.

https://codereview.chromium.org/308053004/diff/150001/ui/message_center/views...
File ui/message_center/views/small_image_mask.cc (right):

https://codereview.chromium.org/308053004/diff/150001/ui/message_center/views...
ui/message_center/views/small_image_mask.cc:11: const gfx::ImageSkia
GetMaskedSmallImage(const gfx::ImageSkia& small_image) {
On 2014/06/16 18:09:55, dewittj wrote:
> I would just move this into notification_view.cc inside an anonymous
namespace,
> since there is only one consumer.  Not worth a new file here.

Done.

https://codereview.chromium.org/308053004/diff/150001/ui/message_center/views...
ui/message_center/views/small_image_mask.cc:16: const gfx::ImageSkia background
= CreateImage(width, height, 163, 163, 163);
On 2014/06/16 18:09:55, dewittj wrote:
> I would suggest you use an SkColor in these functions instead.  See
> SkBitmap::eraseColor (in fact eraseRGB is deprecated anyway).
> 
> Also, any constants you use should be defined in message_center_style.h so
that
> they can be used in the Mac version as well.  Define them something like this:
> 
> const SkColor kSmallImageMaskForegraound = SkColor...
> const SkColor kSmallImageMaskBackground = SkColorWHITE;
> 
> You'll see other examples in message_center_style.

Done.

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

https://codereview.chromium.org/308053004/diff/200001/ui/message_center/views...
ui/message_center/views/message_view.cc:37: namespace {
On 2014/06/27 02:43:56, dewittj wrote:
> Just combine the two unnamed namespaces

Done.

[not at google - send to devlin, 2014-06-27 20:45:18]

ping this review when the apps-dev thread is resolved in case I forget.

[liyanhou, 2014-07-11 23:51:43]

On 2014/06/27 20:45:18, kalman (OOO July 11-14) wrote:
> ping this review when the apps-dev thread is resolved in case I forget.

PTAL. From I'm understanding, the name is decided to be appIconMask. Feel free
to let me know if there's any concern. Thank you!

[not at google - send to devlin, 2014-07-15 15:17:55]

sure.

https://codereview.chromium.org/308053004/diff/220001/chrome/common/extension...
File chrome/common/extensions/api/notifications.idl (right):

https://codereview.chromium.org/308053004/diff/220001/chrome/common/extension...
chrome/common/extensions/api/notifications.idl:63: // of a notification
please phrase this comment in terms of the property, which is a noun. see
comments on all of these other properties as examples.

i'd also like to see what this URL actually is. a remote URL? a local URL? data
URL? same goes for all of the other comments on URL images (the iconUrls,
imageUrl). the comments of those could be better really; rather than "Sender's
avatar..." it should be "A URL to the sender's avatar...", with a second
paragraph "URLs can be ...".

and the comment should end with a period.

[liyanhou, 2014-07-21 16:51:34]

PTAL. Thank you so much!

https://codereview.chromium.org/308053004/diff/220001/chrome/common/extension...
File chrome/common/extensions/api/notifications.idl (right):

https://codereview.chromium.org/308053004/diff/220001/chrome/common/extension...
chrome/common/extensions/api/notifications.idl:63: // of a notification
On 2014/07/15 15:17:54, kalman wrote:
> please phrase this comment in terms of the property, which is a noun. see
> comments on all of these other properties as examples.
> 
> i'd also like to see what this URL actually is. a remote URL? a local URL?
data
> URL? same goes for all of the other comments on URL images (the iconUrls,
> imageUrl). the comments of those could be better really; rather than "Sender's
> avatar..." it should be "A URL to the sender's avatar...", with a second
> paragraph "URLs can be ...".
> 
> and the comment should end with a period.

Done.

[not at google - send to devlin, 2014-07-21 20:50:15]

Thanks for clarifying. One last question.

https://codereview.chromium.org/308053004/diff/300001/chrome/common/extension...
File chrome/common/extensions/api/notifications.idl (right):

https://codereview.chromium.org/308053004/diff/300001/chrome/common/extension...
chrome/common/extensions/api/notifications.idl:57: // notifications.
nit: separate these with a blank line so that they are detected as separate
paragraphs. That will look better in the generated documentation.

https://codereview.chromium.org/308053004/diff/300001/chrome/common/extension...
chrome/common/extensions/api/notifications.idl:58: // URLs can be data URL,
local URL, or blob URL.
Let me rephrase this sentence to make sure I understand:

This can be a data: URL, blob: URL, or a URL relative to a resource within this
extension's .crx file. It cannot be a URL to a remote resource.

(my main concern is the use of the word "local").

https://codereview.chromium.org/308053004/diff/300001/chrome/common/extension...
chrome/common/extensions/api/notifications.idl:66: // of the image will be
considered.
you could say here,

A URL to the app icon mask. URLs have the same restrictions as
$(ref:notifications.NotificationOptions.iconUrl iconUrl).

https://codereview.chromium.org/308053004/diff/300001/chrome/common/extension...
chrome/common/extensions/api/notifications.idl:96: // URLs can be data URL,
local URL, or blob URL.
likewise

[liyanhou, 2014-07-22 00:05:23]

PTAL. Thank you so much!

https://codereview.chromium.org/308053004/diff/300001/chrome/common/extension...
File chrome/common/extensions/api/notifications.idl (right):

https://codereview.chromium.org/308053004/diff/300001/chrome/common/extension...
chrome/common/extensions/api/notifications.idl:57: // notifications.
On 2014/07/21 20:50:15, kalman wrote:
> nit: separate these with a blank line so that they are detected as separate
> paragraphs. That will look better in the generated documentation.

Done.

https://codereview.chromium.org/308053004/diff/300001/chrome/common/extension...
chrome/common/extensions/api/notifications.idl:58: // URLs can be data URL,
local URL, or blob URL.
On 2014/07/21 20:50:15, kalman wrote:
> Let me rephrase this sentence to make sure I understand:
> 
> This can be a data: URL, blob: URL, or a URL relative to a resource within
this
> extension's .crx file. It cannot be a URL to a remote resource.
> 
> (my main concern is the use of the word "local").

Done.

https://codereview.chromium.org/308053004/diff/300001/chrome/common/extension...
chrome/common/extensions/api/notifications.idl:66: // of the image will be
considered.
On 2014/07/21 20:50:15, kalman wrote:
> you could say here,
> 
> A URL to the app icon mask. URLs have the same restrictions as
> $(ref:notifications.NotificationOptions.iconUrl iconUrl).

Done.

https://codereview.chromium.org/308053004/diff/300001/chrome/common/extension...
chrome/common/extensions/api/notifications.idl:96: // URLs can be data URL,
local URL, or blob URL.
On 2014/07/21 20:50:15, kalman wrote:
> likewise

Done.

[not at google - send to devlin, 2014-07-22 00:06:16]

lgtm

[liyanhou, 2014-07-22 00:08:37]

The CQ bit was checked by liyanhou@chromium.org

[commit-bot: I haz the power, 2014-07-22 00:11:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/liyanhou@chromium.org/308053004/340001

[commit-bot: I haz the power, 2014-07-22 01:38:42]

Change committed as 284586