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

content/renderer/notification_provider.cc

Index: content/renderer/notification_provider.cc
diff --git a/content/renderer/notification_provider.cc b/content/renderer/notification_provider.cc
index c4d869c2b1caea82e31f46e5997d5e64675c4808..f1716a7ab6bbb467f90e6999dc8f7430274889de 100644
--- a/content/renderer/notification_provider.cc
+++ b/content/renderer/notification_provider.cc
@@ -5,13 +5,18 @@
 #include "content/renderer/notification_provider.h"
 
 #include "base/strings/string_util.h"
+#include "content/child/image_decoder.h"
 #include "content/common/desktop_notification_messages.h"
 #include "content/common/frame_messages.h"
 #include "content/renderer/render_frame_impl.h"
+#include "third_party/WebKit/public/platform/Platform.h"
 #include "third_party/WebKit/public/platform/WebURL.h"
+#include "third_party/WebKit/public/platform/WebURLLoader.h"
+#include "third_party/WebKit/public/platform/WebURLLoaderClient.h"
 #include "third_party/WebKit/public/web/WebDocument.h"
 #include "third_party/WebKit/public/web/WebLocalFrame.h"
 #include "third_party/WebKit/public/web/WebUserGestureIndicator.h"
+#include "third_party/skia/include/core/SkBitmap.h"
 
 using blink::WebDocument;
 using blink::WebNotification;
@@ -19,38 +24,199 @@ using blink::WebNotificationPresenter;
 using blink::WebSecurityOrigin;
 using blink::WebString;
 using blink::WebURL;
+using blink::WebURLError;
+using blink::WebURLLoader;
+using blink::WebURLRequest;
 using blink::WebUserGestureIndicator;
 
 namespace content {
 
-NotificationProvider::NotificationProvider(RenderFrame* render_frame)
-    : RenderFrameObserver(render_frame) {
+// NotificationProvider::IconDownloader ----------------------------------------
+
+// Downloads the icon associated with a notification and decodes the received
+// image. This must be completed before notifications are shown to the user.
+// Icon downloaders must not be re-used for multiple notifications or icons.
+class NotificationProvider::IconDownloader : public blink::WebURLLoaderClient {
+  typedef base::Callback<void(const WebNotification&,
+                              const SkBitmap&)> DownloadCompletedCallback;
+
+ public:
+  IconDownloader(const WebNotification& notification,
+                 const DownloadCompletedCallback& callback);
+
+  virtual ~IconDownloader();
+
+  // Downloads the notification's image and invokes the callback with the
+  // decoded SkBitmap when the download has succeeded, or with an empty SkBitmap
+  // in case the download has failed.
+  void Start();
+
+  // Cancels the current image download. The callback will not be invoked.
+  void Cancel();
+
+  // blink::WebURLLoaderClient implementation.
+  virtual void didReceiveData(WebURLLoader* loader,
+                              const char* data,
+                              int data_length,
+                              int encoded_data_length) OVERRIDE;

[jamesr, 2014/09/24 04:39:54]

not lgtm.  You shouldn't use OVERRIDE on cross-repo interfaces or modifying the
interface becomes impossible

[Peter Beverloo, 2014/10/01 19:34:08]

Erps, that's an oversight indeed. Fixed.

+  virtual void didFinishLoading(WebURLLoader* loader,
+                                double finish_time,
+                                int64_t total_encoded_data_length) OVERRIDE;
+  virtual void didFail(WebURLLoader* loader, const WebURLError& error) OVERRIDE;
+
+  const WebNotification& notification() const {
+    return notification_;
+  }
+
+ private:
+  WebNotification notification_;
+  DownloadCompletedCallback callback_;
+
+  scoped_ptr<WebURLLoader> loader_;
+  bool completed_;
+
+  std::string buffer_;

[jamesr, 2014/09/24 04:39:54]

std::string should only be for utf8 string data, not arbitrary bytes.  If you
want to buffer arbitrary bytes, use a vector<uint8_t> or something like that

[Peter Beverloo, 2014/10/01 19:34:08]

There's quite some precedence for using an std::string here (e.g.
content::WebURLLoaderClientImpl which powers the ResourceFetcher), hence my
choice. I've changed it to be a vector.

Accessing the array on line 129 now uses "&buffer_[0]", as the .data() member of
a vector is a C++11 library function.

+
+  DISALLOW_COPY_AND_ASSIGN(IconDownloader);
+};
+
+NotificationProvider::IconDownloader::IconDownloader(
+    const WebNotification& notification,
+    const DownloadCompletedCallback& callback)
+    : notification_(notification),
+      callback_(callback),
+      completed_(false) {}
+
+NotificationProvider::IconDownloader::~IconDownloader() {}
+
+void NotificationProvider::IconDownloader::Start() {
+  DCHECK(!loader_);
+
+  WebURLRequest request(notification_.iconURL());
+  request.setRequestContext(WebURLRequest::RequestContextImage);
+
+  loader_.reset(blink::Platform::current()->createURLLoader());
+  loader_->loadAsynchronously(request, this);
+}
+
+void NotificationProvider::IconDownloader::Cancel() {
+  DCHECK(loader_);
+
+  completed_ = true;
+  loader_->cancel();
+}
+
+void NotificationProvider::IconDownloader::didReceiveData(
+    WebURLLoader* loader,
+    const char* data,
+    int data_length,
+    int encoded_data_length) {
+  DCHECK(!completed_);
+  DCHECK(data_length > 0);

[jamesr, 2014/09/24 04:39:54]

DCHECK_GT is slightly better

[Peter Beverloo, 2014/10/01 19:34:08]

Done.

+
+  buffer_.append(data, data_length);
+}
+
+void NotificationProvider::IconDownloader::didFinishLoading(
+    WebURLLoader* loader,
+    double finish_time,
+    int64_t total_encoded_data_length) {
+  DCHECK(!completed_);
+
+  ImageDecoder decoder;
+  SkBitmap icon = decoder.Decode(
+      reinterpret_cast<const unsigned char*>(buffer_.data()), buffer_.size());
+
+  callback_.Run(notification_, icon);
+  completed_ = true;
 }
 
-NotificationProvider::~NotificationProvider() {
+void NotificationProvider::IconDownloader::didFail(
+    WebURLLoader* loader, const WebURLError& error) {
+  if (completed_)
+    return;
+
+  callback_.Run(notification_, SkBitmap());
+  completed_ = true;
 }
 
+// NotificationProvider --------------------------------------------------------
+
+NotificationProvider::NotificationProvider(RenderFrame* render_frame)
+    : RenderFrameObserver(render_frame) {}
+
+NotificationProvider::~NotificationProvider() {}
+
 bool NotificationProvider::show(const WebNotification& notification) {
+  if (notification.iconURL().isEmpty()) {
+    DisplayNotification(notification, SkBitmap());
+    return true;
+  }
+
+  scoped_ptr<IconDownloader> downloader(
+      new IconDownloader(notification,
+          base::Bind(&NotificationProvider::DisplayNotification,
+                     base::Unretained(this))));

[jamesr, 2014/09/24 04:39:54]

what makes the base::Unretained() here safe?

[Peter Beverloo, 2014/10/01 19:34:08]

The icon downloaders are owned in the |pending_notifications_| scoped vector, so
we're sure that all instances of IconDownloader will be destroyed before |this|
goes away. They're neither shared nor used outside of this class.

+
+  downloader->Start();
+
+  pending_notifications_.push_back(downloader.release());
+  return true;
+}
+
+void NotificationProvider::DisplayNotification(
+    const WebNotification& notification, const SkBitmap& icon) {
   WebDocument document = render_frame()->GetWebFrame()->document();
   int notification_id = manager_.RegisterNotification(notification);
 
+  RemovePendingNotification(notification);
+
+  PendingNotifications::iterator iter = pending_notifications_.begin();
+  for (; iter != pending_notifications_.end(); ++iter) {
+    if ((*iter)->notification() != notification)
+      continue;
+
+    pending_notifications_.erase(iter);
+    break;
+  }
+
   ShowDesktopNotificationHostMsgParams params;
   params.origin = GURL(document.securityOrigin().toString());
-  params.icon_url = notification.iconURL();
+  params.icon = icon;
   params.title = notification.title();
   params.body = notification.body();
   params.direction = notification.direction();
   params.replace_id = notification.replaceId();
-  return Send(new DesktopNotificationHostMsg_Show(
-      routing_id(), notification_id, params));
+
+  Send(new DesktopNotificationHostMsg_Show(routing_id(),
+                                           notification_id,
+                                           params));
+}
+
+// Removes the IconDownload object from the list of pending notifications for
+// |notification|. If the download is still in process, it will be cancelled
+// by the blink::WebURLLoaderClient destructor.
+void NotificationProvider::RemovePendingNotification(
+    const WebNotification& notification) {
+  PendingNotifications::iterator iter = pending_notifications_.begin();
+  for (; iter != pending_notifications_.end(); ++iter) {
+    if ((*iter)->notification() != notification)
+      continue;
+
+    pending_notifications_.erase(iter);
+    break;
+  }
 }
 
 void NotificationProvider::cancel(const WebNotification& notification) {
   int id;
   bool id_found = manager_.GetId(notification, id);
-  // Won't be found if the notification has already been closed by the user.
+  // Won't be found if the notification has already been closed by the user,
+  // or if the notification's icon is still being requested.
   if (id_found)
     Send(new DesktopNotificationHostMsg_Cancel(routing_id(), id));
+  else
+    RemovePendingNotification(notification);
 }
 
 void NotificationProvider::objectDestroyed(
@@ -60,6 +226,8 @@ void NotificationProvider::objectDestroyed(
   // Won't be found if the notification has already been closed by the user.
   if (id_found)
     manager_.UnregisterNotification(id);
+  else
+    RemovePendingNotification(notification);
 }
 
 WebNotificationPresenter::Permission NotificationProvider::checkPermission(