Fetch notification action icons and pass them through in resources.

content/child/notifications/pending_notifications_tracker.cc

Index: content/child/notifications/pending_notifications_tracker.cc
diff --git a/content/child/notifications/pending_notifications_tracker.cc b/content/child/notifications/pending_notifications_tracker.cc
index 588af7552e3d26983581af1316e1a652cd66894d..816f48f7b52b8e81d7225329e25c86055725e873 100644
--- a/content/child/notifications/pending_notifications_tracker.cc
+++ b/content/child/notifications/pending_notifications_tracker.cc
@@ -5,57 +5,38 @@
 #include "content/child/notifications/pending_notifications_tracker.h"
 
 #include "base/bind.h"
-#include "base/location.h"
-#include "base/thread_task_runner_handle.h"
-#include "content/child/notifications/notification_image_loader.h"
+#include "base/bind_helpers.h"
+#include "base/callback.h"
+#include "content/child/notifications/pending_notification.h"
 #include "content/public/common/notification_resources.h"
-#include "third_party/WebKit/public/platform/modules/notifications/WebNotificationData.h"
-#include "third_party/skia/include/core/SkBitmap.h"
 
 namespace content {
 
-// Stores the information associated with a pending notification.
-struct PendingNotificationsTracker::PendingNotification {
-  PendingNotification(
-      const scoped_refptr<NotificationImageLoader>& image_loader,
-      const NotificationResourcesFetchedCallback& callback)
-      : image_loader(image_loader), callback(callback) {}
-
-  scoped_refptr<NotificationImageLoader> image_loader;
-  NotificationResourcesFetchedCallback callback;
-};
-
 PendingNotificationsTracker::PendingNotificationsTracker(
-    scoped_refptr<base::SingleThreadTaskRunner> main_thread_task_runner)
-    : main_thread_task_runner_(main_thread_task_runner), weak_factory_(this) {}
+    const scoped_refptr<base::SingleThreadTaskRunner>& main_thread_task_runner)
+    : next_notification_id_(0),
+      main_thread_task_runner_(main_thread_task_runner) {}
 
 PendingNotificationsTracker::~PendingNotificationsTracker() {}
 
-void PendingNotificationsTracker::FetchPageNotificationResources(
+void PendingNotificationsTracker::FetchNotificationResources(
     const blink::WebNotificationData& notification_data,
     blink::WebNotificationDelegate* delegate,
-    const NotificationResourcesFetchedCallback& callback) {
-  delegate_to_pending_id_map_[delegate] = FetchNotificationResources(
-      notification_data, callback,
-      new NotificationImageLoader(
-          base::Bind(&PendingNotificationsTracker::DidFetchPageNotification,
-                     weak_factory_.GetWeakPtr(), delegate),
-          base::ThreadTaskRunnerHandle::Get()));
-}
-
-void PendingNotificationsTracker::FetchPersistentNotificationResources(
-    const blink::WebNotificationData& notification_data,
-    const NotificationResourcesFetchedCallback& callback) {
-  FetchNotificationResources(
-      notification_data, callback,
-      new NotificationImageLoader(
-          base::Bind(
-              &PendingNotificationsTracker::DidFetchPersistentNotification,
-              weak_factory_.GetWeakPtr()),
-          base::ThreadTaskRunnerHandle::Get()));
+    const NotificationResourcesCallback& resources_callback) {
+  int32_t notification_id = next_notification_id_++;
+  if (delegate)
+    delegate_to_pending_id_map_[delegate] = notification_id;
+  base::Closure fetches_finished_callback = base::Bind(
+      &PendingNotificationsTracker::FetchesFinished,
+      base::Unretained(this) /* The pending notifications are owned by this. */,
+      delegate, notification_id, resources_callback);
+  PendingNotification* pending_notification = new PendingNotification(

[Peter Beverloo, 2016/02/05 15:43:21]

What about writing this down as follows to make ownership more explicit?

===============
  scoped_ptr<PendingNotification> pending_notification(
      new PendingNotification(main_thread_task_runner_, notification_data,
                              fetches_finished_callback));
  pending_notification->FetchResources();

  pending_notifications_.AddWithID(
      pending_notification.release(), notification_id);
===============

[Michael van Ouwerkerk, 2016/02/08 14:38:53]

Done.

+      main_thread_task_runner_, notification_data, fetches_finished_callback);
+  pending_notifications_.AddWithID(pending_notification, notification_id);
+  pending_notification->FetchResources();
 }
 
-bool PendingNotificationsTracker::CancelPageNotificationFetches(
+bool PendingNotificationsTracker::CancelResourceFetches(
     blink::WebNotificationDelegate* delegate) {
   auto iter = delegate_to_pending_id_map_.find(delegate);
   if (iter == delegate_to_pending_id_map_.end())
@@ -67,48 +48,19 @@ bool PendingNotificationsTracker::CancelPageNotificationFetches(
   return true;
 }
 
-void PendingNotificationsTracker::DidFetchPageNotification(
+void PendingNotificationsTracker::FetchesFinished(
     blink::WebNotificationDelegate* delegate,
-    int notification_id,
-    const SkBitmap& icon) {
-  PendingNotification* pending_notification =
-      pending_notifications_.Lookup(notification_id);
-  DCHECK(pending_notification);
-
-  NotificationResources notification_resources;
-  notification_resources.notification_icon = icon;
-  pending_notification->callback.Run(notification_resources);
-
-  delegate_to_pending_id_map_.erase(delegate);
-  pending_notifications_.Remove(notification_id);
-}
-
-void PendingNotificationsTracker::DidFetchPersistentNotification(
-    int notification_id, const SkBitmap& icon) {
+    int32_t notification_id,
+    const NotificationResourcesCallback& resources_callback) {
   PendingNotification* pending_notification =
       pending_notifications_.Lookup(notification_id);
   DCHECK(pending_notification);
 
-  NotificationResources notification_resources;
-  notification_resources.notification_icon = icon;
-  pending_notification->callback.Run(notification_resources);
+  resources_callback.Run(pending_notification->GetResources());

[Peter Beverloo, 2016/02/05 15:43:21]

For historical context, we once had a crash here (neé
DidFetchPersistentNotification) where the Lookup() failed in a release build,
and we'd crash after accessing memory due to calling a method on a nullptr.

We haven't seen this in a long time, but if you're into defensive programming
the following is a bit more crash-safe (since failing here isn't the end of the
world) -

===============
if (!pending_notification) {
  NOTREACHED();
  return;
}
===============

[Michael van Ouwerkerk, 2016/02/08 14:38:52]

It would be nice to then link to a bug with that crash. I'm not sure how that
would happen though, as this state is pretty much managed by this class itself
on a single thread.

 
+  if (delegate)
+    delegate_to_pending_id_map_.erase(delegate);
   pending_notifications_.Remove(notification_id);
 }
 
-int PendingNotificationsTracker::FetchNotificationResources(
-    const blink::WebNotificationData& notification_data,
-    const NotificationResourcesFetchedCallback& callback,
-    const scoped_refptr<NotificationImageLoader>& image_loader) {
-  int notification_id = pending_notifications_.Add(
-      new PendingNotification(image_loader, callback));
-
-  main_thread_task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(&NotificationImageLoader::StartOnMainThread, image_loader,
-                 notification_id, GURL(notification_data.icon)));
-
-  return notification_id;
-}
-
 }  // namespace content