Factor out a PendingNotificationTracker from the NotificationManager.

content/child/notifications/notification_manager.cc

Index: content/child/notifications/notification_manager.cc
diff --git a/content/child/notifications/notification_manager.cc b/content/child/notifications/notification_manager.cc
index 9108a3af7ee7487670b3fb5516416296fb46a68f..d8dac177bf111de09313314659f12105ff5a4f9a 100644
--- a/content/child/notifications/notification_manager.cc
+++ b/content/child/notifications/notification_manager.cc
@@ -10,14 +10,12 @@
 #include "base/threading/thread_local.h"
 #include "content/child/notifications/notification_data_conversions.h"
 #include "content/child/notifications/notification_dispatcher.h"
-#include "content/child/notifications/notification_image_loader.h"
 #include "content/child/service_worker/web_service_worker_registration_impl.h"
 #include "content/child/thread_safe_sender.h"
 #include "content/child/worker_task_runner.h"
 #include "content/common/platform_notification_messages.h"
 #include "content/public/common/platform_notification_data.h"
 #include "third_party/WebKit/public/platform/WebSerializedOrigin.h"
-#include "third_party/WebKit/public/platform/modules/notifications/WebNotificationData.h"
 #include "third_party/WebKit/public/platform/modules/notifications/WebNotificationDelegate.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 
@@ -40,9 +38,8 @@ NotificationManager::NotificationManager(
     base::SingleThreadTaskRunner* main_thread_task_runner,
     NotificationDispatcher* notification_dispatcher)
     : thread_safe_sender_(thread_safe_sender),
-      main_thread_task_runner_(main_thread_task_runner),
       notification_dispatcher_(notification_dispatcher),
-      weak_factory_(this) {
+      pending_notifications_(this, main_thread_task_runner) {
   g_notification_manager_tls.Pointer()->Set(this);
 }
 
@@ -64,6 +61,41 @@ NotificationManager* NotificationManager::ThreadSpecificInstance(
   return manager;
 }
 
+void NotificationManager::DisplayPageNotification(
+    const blink::WebSerializedOrigin& origin,
+    const blink::WebNotificationData& notification_data,
+    blink::WebNotificationDelegate* delegate,
+    const SkBitmap& icon) {
+  int notification_id =
+      notification_dispatcher_->GenerateNotificationId(CurrentWorkerId());
+
+  active_page_notifications_[notification_id] = delegate;
+  thread_safe_sender_->Send(
+      new PlatformNotificationHostMsg_Show(
+          notification_id,
+          GURL(origin.string()),
+          icon,
+          ToPlatformNotificationData(notification_data)));
+}
+
+void NotificationManager::DisplayPersistentNotification(
+    const blink::WebSerializedOrigin& origin,
+    const blink::WebNotificationData& notification_data,
+    int64 service_worker_registration_id,
+    scoped_ptr<blink::WebNotificationShowCallbacks> callbacks,
+    const SkBitmap& icon) {
+  thread_safe_sender_->Send(
+      new PlatformNotificationHostMsg_ShowPersistent(
+          service_worker_registration_id,
+          GURL(origin.string()),
+          icon,
+          ToPlatformNotificationData(notification_data)));
+
+  // There currently isn't a case in which the promise would be rejected per
+  // our implementation, so always resolve it here.
+  callbacks->onSuccess();
+}
+
 void NotificationManager::OnWorkerRunLoopStopped() {
   delete this;
 }
@@ -73,18 +105,13 @@ void NotificationManager::show(
     const blink::WebNotificationData& notification_data,
     blink::WebNotificationDelegate* delegate) {
   if (notification_data.icon.isEmpty()) {
-    DisplayNotification(origin, notification_data, delegate,
-                        nullptr /* image_loader */);
+    DisplayPageNotification(origin, notification_data, delegate,
+                            SkBitmap() /* icon */);

[mlamouri (slow - plz ping), 2015/02/18 14:53:58]

nit: you probably don't need that /* icon */ comment.

[Peter Beverloo, 2015/02/18 15:11:50]

Done.

     return;
   }
 
-  pending_page_notifications_[delegate] = CreateImageLoader(
-      notification_data.icon,
-      base::Bind(&NotificationManager::DisplayNotification,
-                 weak_factory_.GetWeakPtr(),
-                 origin,
-                 notification_data,
-                 delegate));
+  pending_notifications_.FetchPageNotification(
+      origin, notification_data, delegate);
 }
 
 void NotificationManager::showPersistent(
@@ -93,48 +120,42 @@ void NotificationManager::showPersistent(
     blink::WebServiceWorkerRegistration* service_worker_registration,
     blink::WebNotificationShowCallbacks* callbacks) {
   DCHECK(service_worker_registration);
-
   int64 service_worker_registration_id =
       static_cast<WebServiceWorkerRegistrationImpl*>(
           service_worker_registration)->registration_id();
 
-  int request_id = persistent_notification_requests_.Add(callbacks);
+  scoped_ptr<blink::WebNotificationShowCallbacks> owned_callbacks(callbacks);
   if (notification_data.icon.isEmpty()) {
     DisplayPersistentNotification(origin,
                                   notification_data,
                                   service_worker_registration_id,
-                                  request_id,
-                                  nullptr /* image_loader */);
+                                  owned_callbacks.Pass(),
+                                  SkBitmap() /* icon */);

[mlamouri (slow - plz ping), 2015/02/18 14:53:58]

nit: /* icon */ is probably not needed.

[Peter Beverloo, 2015/02/18 15:11:50]

Done.

     return;
   }
 
-  pending_persistent_notifications_.insert(CreateImageLoader(
-      notification_data.icon,
-      base::Bind(&NotificationManager::DisplayPersistentNotification,
-                 weak_factory_.GetWeakPtr(),
-                 origin,
-                 notification_data,
-                 service_worker_registration_id,
-                 request_id)));
+  pending_notifications_.FetchPersistentNotification(
+      origin, notification_data, service_worker_registration_id,
+      owned_callbacks.Pass());
 }
 
 void NotificationManager::close(blink::WebNotificationDelegate* delegate) {
-  if (RemovePendingPageNotification(delegate))
+  if (pending_notifications_.CancelPageNotificationFetches(delegate))
     return;
 
-  auto iter = active_notifications_.begin();
-  for (; iter != active_notifications_.end(); ++iter) {
+  auto iter = active_page_notifications_.begin();
+  for (; iter != active_page_notifications_.end(); ++iter) {

[mlamouri (slow - plz ping), 2015/02/18 14:53:58]

Here and below, could that be:
  for (auto iter : active_page_notifications)

?

[Peter Beverloo, 2015/02/18 15:11:50]

Done.

     if (iter->second != delegate)
       continue;
 
     thread_safe_sender_->Send(
         new PlatformNotificationHostMsg_Close(iter->first));
-    active_notifications_.erase(iter);
+    active_page_notifications_.erase(iter);
     return;
   }
 
   // It should not be possible for Blink to call close() on a Notification which
-  // does not exist anymore in the manager.
+  // does not exist in either the pending or active notification lists.
   NOTREACHED();
 }
 
@@ -146,15 +167,15 @@ void NotificationManager::closePersistent(
 
 void NotificationManager::notifyDelegateDestroyed(
     blink::WebNotificationDelegate* delegate) {
-  if (RemovePendingPageNotification(delegate))
+  if (pending_notifications_.CancelPageNotificationFetches(delegate))
     return;
 
-  auto iter = active_notifications_.begin();
-  for (; iter != active_notifications_.end(); ++iter) {
+  auto iter = active_page_notifications_.begin();
+  for (; iter != active_page_notifications_.end(); ++iter) {
     if (iter->second != delegate)
       continue;
 
-    active_notifications_.erase(iter);
+    active_page_notifications_.erase(iter);
     return;
   }
 }
@@ -182,107 +203,28 @@ bool NotificationManager::OnMessageReceived(const IPC::Message& message) {
 }
 
 void NotificationManager::OnDidShow(int notification_id) {
-  const auto& iter = active_notifications_.find(notification_id);
-  if (iter == active_notifications_.end())
+  const auto& iter = active_page_notifications_.find(notification_id);
+  if (iter == active_page_notifications_.end())
     return;
 
   iter->second->dispatchShowEvent();
 }
 
 void NotificationManager::OnDidClose(int notification_id) {
-  const auto& iter = active_notifications_.find(notification_id);
-  if (iter == active_notifications_.end())
+  const auto& iter = active_page_notifications_.find(notification_id);
+  if (iter == active_page_notifications_.end())
     return;
 
   iter->second->dispatchCloseEvent();
-  active_notifications_.erase(iter);
+  active_page_notifications_.erase(iter);
 }
 
 void NotificationManager::OnDidClick(int notification_id) {
-  const auto& iter = active_notifications_.find(notification_id);
-  if (iter == active_notifications_.end())
+  const auto& iter = active_page_notifications_.find(notification_id);
+  if (iter == active_page_notifications_.end())
     return;
 
   iter->second->dispatchClickEvent();
 }
 
-scoped_refptr<NotificationImageLoader> NotificationManager::CreateImageLoader(
-    const blink::WebURL& image_url,
-    const NotificationImageLoadedCallback& callback) const {
-  scoped_refptr<NotificationImageLoader> pending_notification(
-      new NotificationImageLoader(callback));
-
-  main_thread_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&NotificationImageLoader::StartOnMainThread,
-                            pending_notification, image_url,
-                            base::ThreadTaskRunnerHandle::Get()));
-
-  return pending_notification;
-}
-
-void NotificationManager::DisplayNotification(
-    const blink::WebSerializedOrigin& origin,
-    const blink::WebNotificationData& notification_data,
-    blink::WebNotificationDelegate* delegate,
-    scoped_refptr<NotificationImageLoader> image_loader) {
-  int notification_id =
-      notification_dispatcher_->GenerateNotificationId(CurrentWorkerId());
-
-  active_notifications_[notification_id] = delegate;
-
-  SkBitmap icon;
-  if (image_loader)
-    icon = image_loader->GetDecodedImage();
-
-  thread_safe_sender_->Send(
-      new PlatformNotificationHostMsg_Show(
-          notification_id,
-          GURL(origin.string()),
-          icon,
-          ToPlatformNotificationData(notification_data)));
-
-  // If this Notification contained an icon, it can be safely deleted now.
-  RemovePendingPageNotification(delegate);
-}
-
-void NotificationManager::DisplayPersistentNotification(
-    const blink::WebSerializedOrigin& origin,
-    const blink::WebNotificationData& notification_data,
-    int64 service_worker_registration_id,
-    int request_id,
-    scoped_refptr<NotificationImageLoader> image_loader) {
-  blink::WebNotificationShowCallbacks* callbacks =
-      persistent_notification_requests_.Lookup(request_id);
-  DCHECK(callbacks);
-
-  SkBitmap icon;
-  if (image_loader) {
-    pending_persistent_notifications_.erase(image_loader);
-    icon = image_loader->GetDecodedImage();
-  }
-
-  thread_safe_sender_->Send(
-      new PlatformNotificationHostMsg_ShowPersistent(
-          service_worker_registration_id,
-          GURL(origin.string()),
-          icon,
-          ToPlatformNotificationData(notification_data)));
-
-  // There currently isn't a case in which the promise would be rejected per
-  // our implementation, so always resolve it here.
-  callbacks->onSuccess();
-
-  persistent_notification_requests_.Remove(request_id);
-}
-
-bool NotificationManager::RemovePendingPageNotification(
-    blink::WebNotificationDelegate* delegate) {
-  const auto& iter = pending_page_notifications_.find(delegate);
-  if (iter == pending_page_notifications_.end())
-    return false;
-
-  pending_page_notifications_.erase(iter);
-  return true;
-}
-
 }  // namespace content