Request the icon of a Web Notification in the renderer process.

chrome/browser/notifications/desktop_notification_service.cc

Index: chrome/browser/notifications/desktop_notification_service.cc
diff --git a/chrome/browser/notifications/desktop_notification_service.cc b/chrome/browser/notifications/desktop_notification_service.cc
index eb6d72f6b7a30377eba03195accebb1370a8bd0e..ca8770c9e98a631726ad539c8341635cbe4fba9b 100644
--- a/chrome/browser/notifications/desktop_notification_service.cc
+++ b/chrome/browser/notifications/desktop_notification_service.cc
@@ -163,9 +163,22 @@ void DesktopNotificationService::ShowDesktopNotification(
 
   base::string16 display_source = DisplayNameForOriginInProcessId(
       origin, render_frame_host->GetProcess()->GetID());
-  Notification notification(origin, params.icon_url, params.title,
-      params.body, params.direction, display_source, params.replace_id,
-      proxy);
+
+  // TODO(peter): gfx::Image only provides an API to convert a SkBitmap to an
+  // image using 1x scale. While the Views system will resize is for the
+  // appropriate resolution, doing so immediately should result in icons of

[dewittj, 2014/09/18 17:46:49]

What do you mean immediately?

Also gfx::Image does provide a way to set multiple scale factors, it's:

gfx::ImageSkia skia(gfx::ImageSkiaRep(icon_bitmap, 1.0f));
skia.AddRepresentation(gfx::ImageSkiaRep(icon_2x_bitmap, 2.0f));
...
gfx::Image result(skia)

[Peter Beverloo, 2014/09/19 14:15:48]

Ah, that's good to know, however, I don't think that's how we should implement
this when we support multiple image resolutions for Web Notifications, as it
would imply requesting all the images. Instead, we should download an SkBitmap
of the right format from the renderer process, and pass along the used scale
factor so that we can use that ImageSkiaRep constructor.

I've updated the comment.

[dewittj, 2014/09/24 18:28:44]

I don't think the "right format" is so simple, since it can change over time;
users can change the pixel density on their Chromebooks with a key combo!  But
still fine for now.

Also, I've been looking into this for the extension API, see
https://codereview.chromium.org/354733002/

+  // higher quality.
+  Notification notification(message_center::NOTIFICATION_TYPE_SIMPLE,
+                            origin,
+                            params.title,
+                            params.body,

[Mike West, 2014/09/18 19:48:42]

This is a huge signature.

It looks like you're passing in a number of `params` attributes. Perhaps a
future refactoring could just pass in a const ref to the params object itself?

[Peter Beverloo, 2014/09/19 14:15:48]

We're trying to trim down the number of Notification constructors to make
upcoming refactorings a bit easier. There aren't many call-sites, so the noise
is fine for now.

In the longer run we could consider doing this, but the struct is going to
include other data which does not relate to the Notification class as well.

+                            gfx::Image::CreateFrom1xBitmap(params.icon),
+                            blink::WebTextDirectionDefault,
+                            message_center::NotifierId(origin),
+                            display_source,
+                            params.replace_id,
+                            message_center::RichNotificationData(),
+                            proxy);
 
   // The webkit notification doesn't timeout.
   notification.set_never_timeout(true);