Made WebKit notifications display 80x80 icons if they have them.

Made WebKit notifications display 80x80 icons if they have them.

In the rich notification center and toasts, WebKit notifications
(those created with webkitNotifications.createNotification() or with
chrome.notifications.create({type: "simple"})) usually display their
icons in a 40x40 square, as that was the size used for notification
icons before rich notifications.

This code change causes icons that are exacly 80x80 pixels and that have
no transparency to be displayed using the full 80x80 pixel space
available with rich notifications. This allows developers to create
notifications from web pages and from apps that need to work without
rich notifications that will look as as good as they can with rich
notifications.
    
BUG=222175
R=mukai@chromium.org
TBR=pkotwicz@chromium.org

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

[dharcourt, 2013-04-12 11:03:29]

Jun, could you PTAL at the change list as a whole when you get a chance?

Peter, thanks for your pointers on image opacity. Can you take a look at just
the HasAlpha() method of the single file in this change list and let me know if
you see anything scary or if it looks reasonable to you?

Robert, thank you also for your pointers. I'm CC'ing you in case you see any
problems with my implementation. Any comment or suggestion you may have would be
greatly appreciated.

Thanks for the review!

    - Charles

[Jun Mukai, 2013-04-12 18:50:21]

lgtm

https://codereview.chromium.org/12879009/diff/6002/ui/message_center/views/no...
File ui/message_center/views/notification_view.cc (right):

https://codereview.chromium.org/12879009/diff/6002/ui/message_center/views/no...
ui/message_center/views/notification_view.cc:423: HasAlpha(icon, GetWidget())))
{
nit: align the indent

[dharcourt, 2013-04-12 19:21:06]

https://codereview.chromium.org/12879009/diff/6002/ui/message_center/views/no...
File ui/message_center/views/notification_view.cc (right):

https://codereview.chromium.org/12879009/diff/6002/ui/message_center/views/no...
ui/message_center/views/notification_view.cc:423: HasAlpha(icon, GetWidget())))
{
On 2013/04/12 18:50:21, Jun Mukai wrote:
> nit: align the indent

Should this be aligned this way (#1):

    if (notification.type() == NOTIFICATION_TYPE_SIMPLE &&
        (icon.width() != kIconSize ||
         icon.height() != kIconSize ||
         HasAlpha(icon, GetWidget()))) {

...or that way (#2):

    if (notification.type() == NOTIFICATION_TYPE_SIMPLE &&
           (icon.width() != kIconSize ||
           icon.height() != kIconSize ||
           HasAlpha(icon, GetWidget()))) {

...or another way (#3)? I haven't found a specific reference to this in the
style guide.

[Jun Mukai, 2013-04-12 19:34:38]

https://codereview.chromium.org/12879009/diff/6002/ui/message_center/views/no...
File ui/message_center/views/notification_view.cc (right):

https://codereview.chromium.org/12879009/diff/6002/ui/message_center/views/no...
ui/message_center/views/notification_view.cc:423: HasAlpha(icon, GetWidget())))
{
On 2013/04/12 19:21:07, dharcourt wrote:
> On 2013/04/12 18:50:21, Jun Mukai wrote:
> > nit: align the indent
> 
> Should this be aligned this way (#1):
> 
>     if (notification.type() == NOTIFICATION_TYPE_SIMPLE &&
>         (icon.width() != kIconSize ||
>          icon.height() != kIconSize ||
>          HasAlpha(icon, GetWidget()))) {
> 
> ...or that way (#2):
> 
>     if (notification.type() == NOTIFICATION_TYPE_SIMPLE &&
>            (icon.width() != kIconSize ||
>            icon.height() != kIconSize ||
>            HasAlpha(icon, GetWidget()))) {
> 
> ...or another way (#3)? I haven't found a specific reference to this in the
> style guide.

#1 is in my mind.

[dharcourt, 2013-04-12 20:09:56]

Jun, thanks for the review, I've corrected the nit.

Peter, since we had a 5 pm deadline to land this, I'm moving you from reviewer
to TBR. I hope that's OK. The image code involves looping over the alpha pixels
of an 80x80 images, which should not be a risk to performance (I've timed it at
0.77 ms per notification on my machine). If there is a problem Jun and I didn't
see, I can fix it in a follow-up CL.

Thanks!

    - Charles

https://codereview.chromium.org/12879009/diff/6002/ui/message_center/views/no...
File ui/message_center/views/notification_view.cc (right):

https://codereview.chromium.org/12879009/diff/6002/ui/message_center/views/no...
ui/message_center/views/notification_view.cc:423: HasAlpha(icon, GetWidget())))
{
On 2013/04/12 19:34:38, Jun Mukai wrote:
> On 2013/04/12 19:21:07, dharcourt wrote:
> > On 2013/04/12 18:50:21, Jun Mukai wrote:
> > > nit: align the indent
> > 
> > Should this be aligned this way (#1):
> > 
> >     if (notification.type() == NOTIFICATION_TYPE_SIMPLE &&
> >         (icon.width() != kIconSize ||
> >          icon.height() != kIconSize ||
> >          HasAlpha(icon, GetWidget()))) {
> > 
> > ...or that way (#2):
> > 
> >     if (notification.type() == NOTIFICATION_TYPE_SIMPLE &&
> >            (icon.width() != kIconSize ||
> >            icon.height() != kIconSize ||
> >            HasAlpha(icon, GetWidget()))) {
> > 
> > ...or another way (#3)? I haven't found a specific reference to this in the
> > style guide.
> 
> #1 is in my mind.

Lucky me, that's what I uploaded :-).

[commit-bot: I haz the power, 2013-04-12 20:10:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/12879009/11001

[commit-bot: I haz the power, 2013-04-12 22:25:53]

Change committed as 194026