Fetch notification action icons and pass them through in resources.

content/child/notifications/pending_notification.cc

+// Copyright 2016 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/pending_notification.h"
+
+#include "base/barrier_closure.h"
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/location.h"
+#include "base/thread_task_runner_handle.h"
+#include "content/public/common/notification_resources.h"
+#include "url/gurl.h"
+
+namespace content {
+
+PendingNotification::PendingNotification(
+    scoped_refptr<base::SingleThreadTaskRunner> main_thread_task_runner,
+    const blink::WebNotificationData& notification_data,
+    const base::Closure& fetches_finished_callback)
+    : main_thread_task_runner_(main_thread_task_runner),
+      notification_data_(notification_data),
+      action_icons_(std::vector<SkBitmap>(notification_data_.actions.size())),

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

Initialization list declarations will construct the member with the given
arguments. No need for the additional move here, you can just write:

  action_icons(notification_data_.actions.size())

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

Done.

+      fetches_finished_barrier_closure_(BarrierClosure(

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

Why don't you have to specify base:: here?

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

Dunno, maybe because it has BASE_EXPORT? Certainly it has precedence, but it
seems only right to specify the namespace. Done.

+          1 /* notification icon */ + notification_data_.actions.size(),
+          fetches_finished_callback)),
+      weak_factory_(this) {}
+
+PendingNotification::~PendingNotification() {}
+
+void PendingNotification::FetchResources() {
+  // TODO(mvanouwerkerk): Add a timeout mechanism: crbug.com/579137
+  FetchImageResource(notification_data_.icon,
+                     base::Bind(&PendingNotification::DidFetchNotificationIcon,
+                                weak_factory_.GetWeakPtr()));
+  for (size_t i = 0; i < notification_data_.actions.size(); i++) {
+    FetchImageResource(notification_data_.actions[i].icon,
+                       base::Bind(&PendingNotification::DidFetchActionIcon,
+                                  weak_factory_.GetWeakPtr(), i));
+  }
+}
+
+NotificationResources PendingNotification::GetResources() {

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

This will make a copy of all the resources. Since this isn't marked as const,
can we perhaps ensure that it'll only be called once and swap memory instead?

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

It's not that bad, the bulky pixel data is thread-safe ref counted, and a deep
copy of that data is only made in the various copyTo methods of SkBitmap. It's
easy to add a call to std::move but then the contract of this method changes
significantly, without much gain. Do you think that's worthwhile?

+  NotificationResources resources;
+  resources.notification_icon = notification_icon_;
+  resources.action_icons = action_icons_;
+  return resources;
+}
+
+void PendingNotification::FetchImageResource(
+    const blink::WebURL& image_web_url,
+    const ImageLoadCompletedCallback& image_callback) {
+  if (image_web_url.isEmpty()) {
+    image_callback.Run(SkBitmap());
+    return;
+  }
+
+  // Convert to GURL before sending to the main thread (crbug.com/458640).

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

Per your request, a suggestion for an updated comment with more details:

  // Explicitly convert the WebURL to a GURL before passing it to a different
  // thread. This is important because WebURLs must not be passed between
  // threads, and per base::Bind() semantics conversion would otherwise be done
  // on the receiving thread.

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

Nice. Done.

+  GURL image_gurl(image_web_url);
+
+  scoped_refptr<NotificationImageLoader> image_loader(
+      new NotificationImageLoader(image_callback,
+                                  base::ThreadTaskRunnerHandle::Get()));

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

Something to consider: since we know both the target thread and the main thread
here, rather than using ThreadTaskRunnerHandle::Get() in the
NotificationImageLoader we could simplify that code a bit by passing it in the
constructor, re-adding the main thread DCHECK in StartOnMainThread() and remove
the if(has_main_thread) statements in the destructors, since we know we'll have
it.

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

Cool. Done.

+  image_loaders_.insert(image_loader);
+  main_thread_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&NotificationImageLoader::StartOnMainThread,
+                            image_loader, image_web_url));
+}
+
+void PendingNotification::DidFetchNotificationIcon(
+    const SkBitmap& notification_icon) {
+  notification_icon_ = notification_icon;
+  fetches_finished_barrier_closure_.Run();
+}
+
+void PendingNotification::DidFetchActionIcon(size_t action_index,
+                                             const SkBitmap& action_icon) {
+  action_icons_[action_index] = action_icon;

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

DCHECK_LT(action_index, action_icons_.size()); ?

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

Done.

+  fetches_finished_barrier_closure_.Run();
+}
+
+}  // namespace content