Fetch notification action icons and pass them through in resources.

content/child/notifications/notification_manager.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/child/notifications/notification_manager.h"
 
 #include <cmath>
 #include <utility>
 
 #include "base/lazy_instance.h"
@@ skipping 14 matching lines @@
 
 using blink::WebNotificationPermission;
 
 namespace content {
 namespace {
 
 int CurrentWorkerId() {
   return WorkerThread::GetCurrentId();
 }
 
+// Checks whether |notification_data| specifies any non-empty resources that
+// need to be fetched.
+bool hasResourcesToFetch(const blink::WebNotificationData& notification_data) {

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

nit: HasResourcesToFetch (Chromium style)

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

Done.

+  if (!notification_data.icon.isEmpty())
+    return true;
+  for (const auto& action : notification_data.actions) {
+    if (!action.icon.isEmpty())
+      return true;
+  }
+  return false;
+}
+
 }  // namespace
 
 static base::LazyInstance<base::ThreadLocalPointer<NotificationManager>>::Leaky
     g_notification_manager_tls = LAZY_INSTANCE_INITIALIZER;
 
 NotificationManager::NotificationManager(
     ThreadSafeSender* thread_safe_sender,
     base::SingleThreadTaskRunner* main_thread_task_runner,
     NotificationDispatcher* notification_dispatcher)
     : thread_safe_sender_(thread_safe_sender),
@@ skipping 21 matching lines @@
 }
 
 void NotificationManager::WillStopCurrentWorkerThread() {
   delete this;
 }
 
 void NotificationManager::show(
     const blink::WebSecurityOrigin& origin,
     const blink::WebNotificationData& notification_data,
     blink::WebNotificationDelegate* delegate) {
-  if (notification_data.icon.isEmpty()) {
+  if (!hasResourcesToFetch(notification_data)) {
     DisplayPageNotification(origin, notification_data, delegate,
                             NotificationResources());
     return;
   }
 
-  notifications_tracker_.FetchPageNotificationResources(
+  notifications_tracker_.FetchNotificationResources(

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

DCHECK_EQ(0u, notification_data.actions.size());

No harm in keeping the post-fetch DCHECK as well, but I'd rather we check
*before* downloading resources since HasResourcesToFetch() would happily return
true.

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

Sure. Let's put that at the start of the function then.

       notification_data, delegate,
       base::Bind(&NotificationManager::DisplayPageNotification,
                  base::Unretained(this),  // this owns |notifications_tracker_|
                  origin, notification_data, delegate));
 }
 
 void NotificationManager::showPersistent(
     const blink::WebSecurityOrigin& origin,
     const blink::WebNotificationData& notification_data,
     blink::WebServiceWorkerRegistration* service_worker_registration,
@@ skipping 16 matching lines @@
   // an indication that something has gone wrong.
   size_t author_data_size = notification_data.data.size();
   UMA_HISTOGRAM_MEMORY_KB("Notifications.AuthorDataSizeKB",
                           static_cast<int>(ceil(author_data_size / 1024.0)));
 
   if (author_data_size > PlatformNotificationData::kMaximumDeveloperDataSize) {
     owned_callbacks->onError();
     return;
   }
 
-  if (notification_data.icon.isEmpty()) {
+  if (!hasResourcesToFetch(notification_data)) {
+    NotificationResources notification_resources;
+    if (!notification_data.actions.isEmpty()) {

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

It would be great to document why this is significant. What about:

    // Action button indices are expected to have an corresponding icon bitmap,
    // which may be empty when the developer provided no (or an invalid) icon.

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

Yes, it's a subtle one.

+      notification_resources.action_icons.resize(
+          notification_data.actions.size());
+    }
     DisplayPersistentNotification(
         origin, notification_data, service_worker_registration_id,
-        std::move(owned_callbacks), NotificationResources());
+        std::move(owned_callbacks), notification_resources);
     return;
   }
 
-  notifications_tracker_.FetchPersistentNotificationResources(
-      notification_data,
+  notifications_tracker_.FetchNotificationResources(
+      notification_data, nullptr /* delegate */,
       base::Bind(&NotificationManager::DisplayPersistentNotification,
                  base::Unretained(this),  // this owns |notifications_tracker_|
                  origin, notification_data, service_worker_registration_id,
                  base::Passed(&owned_callbacks)));
 }
 
 void NotificationManager::getNotifications(
     const blink::WebString& filter_tag,
     blink::WebServiceWorkerRegistration* service_worker_registration,
     blink::WebNotificationGetCallbacks* callbacks) {
@@ skipping 14 matching lines @@
       notification_dispatcher_->GenerateNotificationId(CurrentWorkerId());
 
   pending_get_notification_requests_.AddWithID(callbacks, request_id);
 
   thread_safe_sender_->Send(new PlatformNotificationHostMsg_GetNotifications(
       request_id, service_worker_registration_id, origin,
       base::UTF16ToUTF8(base::StringPiece16(filter_tag))));
 }
 
 void NotificationManager::close(blink::WebNotificationDelegate* delegate) {
-  if (notifications_tracker_.CancelPageNotificationFetches(delegate))
+  if (notifications_tracker_.CancelResourceFetches(delegate))
     return;
 
   for (auto& iter : active_page_notifications_) {
     if (iter.second != delegate)
       continue;
 
     thread_safe_sender_->Send(
         new PlatformNotificationHostMsg_Close(iter.first));
     active_page_notifications_.erase(iter.first);
     return;
   }
 
   // It should not be possible for Blink to call close() on a Notification which
   // does not exist in either the pending or active notification lists.
   NOTREACHED();
 }
 
 void NotificationManager::closePersistent(
     const blink::WebSecurityOrigin& origin,
     int64_t persistent_notification_id) {
   thread_safe_sender_->Send(new PlatformNotificationHostMsg_ClosePersistent(
       // TODO(mkwst): This is potentially doing the wrong thing with unique
       // origins. Perhaps also 'file:', 'blob:' and 'filesystem:'. See
       // https://crbug.com/490074 for detail.
       blink::WebStringToGURL(origin.toString()), persistent_notification_id));
 }
 
 void NotificationManager::notifyDelegateDestroyed(
     blink::WebNotificationDelegate* delegate) {
-  if (notifications_tracker_.CancelPageNotificationFetches(delegate))
+  if (notifications_tracker_.CancelResourceFetches(delegate))
     return;
 
   for (auto& iter : active_page_notifications_) {
     if (iter.second != delegate)
       continue;
 
     active_page_notifications_.erase(iter.first);
     return;
   }
 }
@@ skipping 96 matching lines @@
   callbacks->onSuccess(notifications);
 
   pending_get_notification_requests_.Remove(request_id);
 }
 
 void NotificationManager::DisplayPageNotification(
     const blink::WebSecurityOrigin& origin,
     const blink::WebNotificationData& notification_data,
     blink::WebNotificationDelegate* delegate,
     const NotificationResources& notification_resources) {
+  DCHECK_EQ(notification_data.actions.size(), 0u);
+  DCHECK_EQ(notification_resources.action_icons.size(), 0u);
+
   int notification_id =
       notification_dispatcher_->GenerateNotificationId(CurrentWorkerId());
 
   active_page_notifications_[notification_id] = delegate;
   // TODO(mkwst): This is potentially doing the wrong thing with unique
   // origins. Perhaps also 'file:', 'blob:' and 'filesystem:'. See
   // https://crbug.com/490074 for detail.
   thread_safe_sender_->Send(new PlatformNotificationHostMsg_Show(
       notification_id, blink::WebStringToGURL(origin.toString()),
       ToPlatformNotificationData(notification_data), notification_resources));
 }
 
 void NotificationManager::DisplayPersistentNotification(
     const blink::WebSecurityOrigin& origin,
     const blink::WebNotificationData& notification_data,
     int64_t service_worker_registration_id,
     scoped_ptr<blink::WebNotificationShowCallbacks> callbacks,
     const NotificationResources& notification_resources) {
+  DCHECK_EQ(notification_data.actions.size(),
+            notification_resources.action_icons.size());
+
   // TODO(peter): GenerateNotificationId is more of a request id. Consider
   // renaming the method in the NotificationDispatcher if this makes sense.
   int request_id =
       notification_dispatcher_->GenerateNotificationId(CurrentWorkerId());
 
   pending_show_notification_requests_.AddWithID(callbacks.release(),
                                                 request_id);
 
   // TODO(mkwst): This is potentially doing the wrong thing with unique
   // origins. Perhaps also 'file:', 'blob:' and 'filesystem:'. See
   // https://crbug.com/490074 for detail.
   thread_safe_sender_->Send(new PlatformNotificationHostMsg_ShowPersistent(
       request_id, service_worker_registration_id,
       blink::WebStringToGURL(origin.toString()),
       ToPlatformNotificationData(notification_data), notification_resources));
 }
 
 }  // namespace content