Decode and resize Background Fetch icons on a background thread

This is currently done on the thread that invokes fetch(), i.e. either
the main or the worker thread. Especially now that we're going to resize
the image to the desired size too, let's move that to the background.

Per this CL we also resize the icon to the display size indicated by the
browser process.

Change-Id: Iee082c67f9c6b4903b931f95c40142cd5f77b853
Reviewed-on: https://chromium-review.googlesource.com/1166965
Commit-Queue: Peter Beverloo <peter@chromium.org>
Reviewed-by: Rayan Kanso <rayankans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582200}

third_party/blink/renderer/modules/background_fetch/background_fetch_icon_loader.cc

@@ -13,12 +13,15 @@
 #include "third_party/blink/renderer/modules/background_fetch/background_fetch_bridge.h"
 #include "third_party/blink/renderer/modules/manifest/image_resource.h"
 #include "third_party/blink/renderer/modules/manifest/image_resource_type_converters.h"
+#include "third_party/blink/renderer/platform/cross_thread_functional.h"
 #include "third_party/blink/renderer/platform/graphics/color_behavior.h"
 #include "third_party/blink/renderer/platform/heap/heap_allocator.h"
 #include "third_party/blink/renderer/platform/image-decoders/image_decoder.h"
 #include "third_party/blink/renderer/platform/image-decoders/image_frame.h"
+#include "third_party/blink/renderer/platform/image-decoders/segment_reader.h"
 #include "third_party/blink/renderer/platform/loader/fetch/resource_loader_options.h"
 #include "third_party/blink/renderer/platform/loader/fetch/resource_request.h"
+#include "third_party/blink/renderer/platform/scheduler/public/background_scheduler.h"
 #include "third_party/blink/renderer/platform/weborigin/kurl.h"
 #include "third_party/blink/renderer/platform/wtf/text/string_impl.h"
 #include "third_party/blink/renderer/platform/wtf/text/wtf_string.h"
@@ -28,8 +31,13 @@ namespace blink {
 
 namespace {
 
-const unsigned long kIconFetchTimeoutInMs = 30000;
-const int kMinimumIconSizeInPx = 0;
+constexpr unsigned long kIconFetchTimeoutInMs = 30000;
+constexpr int kMinimumIconSizeInPx = 0;
+
+// Because including base::ClampToRange would be a dependency violation.
+int ClampToRange(const int value, const int min, const int max) {
+  return std::min(std::max(value, min), max);
+}
 
 }  // namespace
 
@@ -58,13 +66,16 @@ void BackgroundFetchIconLoader::DidGetIconDisplaySizeIfSoLoadIcon(
     ExecutionContext* execution_context,
     IconCallback icon_callback,
     const WebSize& icon_display_size_pixels) {
-  if (icon_display_size_pixels.IsEmpty()) {
+  icon_display_size_pixels_ = icon_display_size_pixels;
+
+  // If |icon_display_size_pixels_| is empty then no image will be displayed by
+  // the UI powering Background Fetch. Bail out immediately.
+  if (icon_display_size_pixels_.IsEmpty()) {
     std::move(icon_callback).Run(SkBitmap());
     return;
   }
 
-  KURL best_icon_url =
-      PickBestIconForDisplay(execution_context, icon_display_size_pixels);
+  KURL best_icon_url = PickBestIconForDisplay(execution_context);
   if (best_icon_url.IsEmpty()) {
     // None of the icons provided was suitable.
     std::move(icon_callback).Run(SkBitmap());
@@ -90,8 +101,7 @@ void BackgroundFetchIconLoader::DidGetIconDisplaySizeIfSoLoadIcon(
 }
 
 KURL BackgroundFetchIconLoader::PickBestIconForDisplay(
-    ExecutionContext* execution_context,
-    const WebSize& icon_display_size_pixels) {
+    ExecutionContext* execution_context) {
   std::vector<Manifest::ImageResource> icons;
   for (auto& icon : icons_) {
     // Update the src of |icon| to include the base URL in case relative paths
@@ -102,7 +112,7 @@ KURL BackgroundFetchIconLoader::PickBestIconForDisplay(
 
   // TODO(crbug.com/868875): Handle cases where `sizes` or `purpose` is empty.
   return KURL(ManifestIconSelector::FindBestMatchingIcon(
-      std::move(icons), icon_display_size_pixels.height, kMinimumIconSizeInPx,
+      std::move(icons), icon_display_size_pixels_.height, kMinimumIconSizeInPx,
       Manifest::ImageResource::Purpose::ANY));
 }
 
@@ -128,22 +138,69 @@ void BackgroundFetchIconLoader::DidFinishLoading(
     unsigned long resource_identifier) {
   if (stopped_)
     return;
-  if (data_) {
-    // Decode data.
-    const bool data_complete = true;
-    std::unique_ptr<ImageDecoder> decoder = ImageDecoder::Create(
-        data_, data_complete, ImageDecoder::kAlphaPremultiplied,
-        ImageDecoder::kDefaultBitDepth, ColorBehavior::TransformToSRGB());
-    if (decoder) {
-      // the |ImageFrame*| is owned by the decoder.
-      ImageFrame* image_frame = decoder->DecodeFrameBufferAtIndex(0);
-      if (image_frame) {
-        std::move(icon_callback_).Run(image_frame->Bitmap());
-        return;
+
+  if (!data_) {
+    RunCallbackWithEmptyBitmap();
+    return;
+  }
+
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner =
+      Platform::Current()->CurrentThread()->GetTaskRunner();
+
+  BackgroundScheduler::PostOnBackgroundThread(
+      FROM_HERE,
+      CrossThreadBind(
+          &BackgroundFetchIconLoader::DecodeAndResizeImageOnBackgroundThread,
+          WrapCrossThreadPersistent(this), std::move(task_runner),
+          SegmentReader::CreateFromSharedBuffer(std::move(data_))));
+}
+
+void BackgroundFetchIconLoader::DecodeAndResizeImageOnBackgroundThread(
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+    scoped_refptr<SegmentReader> data) {
+  DCHECK(task_runner);
+  DCHECK(data);
+
+  // Explicitly pass in the |icon_display_size_pixels_| to benefit from decoders
+  // that have optimizations for partial decoding.
+  std::unique_ptr<ImageDecoder> decoder = ImageDecoder::Create(
+      std::move(data), /* data_complete= */ true,
+      ImageDecoder::kAlphaPremultiplied, ImageDecoder::kDefaultBitDepth,
+      ColorBehavior::TransformToSRGB(),
+      {icon_display_size_pixels_.width, icon_display_size_pixels_.height});
+
+  if (decoder) {
+    ImageFrame* image_frame = decoder->DecodeFrameBufferAtIndex(0);
+    if (image_frame) {
+      decoded_icon_ = image_frame->Bitmap();
+
+      int width = decoded_icon_.width();
+      int height = decoded_icon_.height();
+
+      // If the |decoded_icon_| is larger than |icon_display_size_pixels_|
+      // permits, we need to resize it as well. This can be done synchronously
+      // given that we're on a background thread already.
+      double scale = std::min(
+          static_cast<double>(icon_display_size_pixels_.width) / width,
+          static_cast<double>(icon_display_size_pixels_.height) / height);
+
+      if (scale < 1) {
+        width = ClampToRange(scale * width, 1, icon_display_size_pixels_.width);
+        height =
+            ClampToRange(scale * height, 1, icon_display_size_pixels_.height);
+
+        // Use the RESIZE_GOOD quality allowing the implementation to pick an
+        // appropriate method for the resize. Can be increased to RESIZE_BETTER
+        // or RESIZE_BEST if the quality looks poor.
+        decoded_icon_ = skia::ImageOperations::Resize(
+            decoded_icon_, skia::ImageOperations::RESIZE_GOOD, width, height);
       }
     }
   }
-  RunCallbackWithEmptyBitmap();
+
+  PostCrossThreadTask(*task_runner, FROM_HERE,
+                      CrossThreadBind(&BackgroundFetchIconLoader::RunCallback,
+                                      WrapCrossThreadPersistent(this)));
 }
 
 void BackgroundFetchIconLoader::DidFail(const ResourceError& error) {
@@ -154,13 +211,18 @@ void BackgroundFetchIconLoader::DidFailRedirectCheck() {
   RunCallbackWithEmptyBitmap();
 }
 
-void BackgroundFetchIconLoader::RunCallbackWithEmptyBitmap() {
+void BackgroundFetchIconLoader::RunCallback() {
   // If this has been stopped it is not desirable to trigger further work,
   // there is a shutdown of some sort in progress.
   if (stopped_)
     return;
 
-  std::move(icon_callback_).Run(SkBitmap());
+  std::move(icon_callback_).Run(decoded_icon_);
+}
+
+void BackgroundFetchIconLoader::RunCallbackWithEmptyBitmap() {
+  DCHECK(decoded_icon_.isNull());
+  RunCallback();
 }
 
 }  // namespace blink

third_party/blink/renderer/modules/background_fetch/background_fetch_icon_loader.h

@@ -17,6 +17,7 @@
 namespace blink {
 
 class BackgroundFetchBridge;
+class SegmentReader;
 struct WebSize;
 
 class MODULES_EXPORT BackgroundFetchIconLoader final
@@ -30,16 +31,12 @@ class MODULES_EXPORT BackgroundFetchIconLoader final
   BackgroundFetchIconLoader();
   ~BackgroundFetchIconLoader() override;
 
-  // Scales down |icon| and returns result. If it is already small enough,
-  // |icon| is returned unchanged.
-  static SkBitmap ScaleDownIfNeeded(const SkBitmap& icon);
-
   // Asynchronously download an icon from the given url, decodes the loaded
   // data, and passes the bitmap to the given callback.
   void Start(BackgroundFetchBridge* bridge,
              ExecutionContext* execution_context,
              HeapVector<ManifestImageResource> icons,
-             IconCallback callback);
+             IconCallback icon_callback);
 
   // Cancels the pending load, if there is one. The |icon_callback_| will not
   // be run.
@@ -58,6 +55,8 @@ class MODULES_EXPORT BackgroundFetchIconLoader final
 
  private:
   friend class BackgroundFetchIconLoaderTest;
+
+  void RunCallback();
   void RunCallbackWithEmptyBitmap();
 
   // Callback for BackgroundFetchBridge::GetIconDisplaySize()
@@ -67,16 +66,37 @@ class MODULES_EXPORT BackgroundFetchIconLoader final
       const WebSize& icon_display_size_pixels);
 
   // Picks the best icon from the list of developer provided icons, for current
-  // display, given the ideal |icon_display_size_pixels|, and returns its KURL.
+  // display, given the ideal |icon_display_size_pixels_|, and returns its KURL.
   // If none of the icons is appropriate, this returns an empty URL.
-  KURL PickBestIconForDisplay(ExecutionContext* execution_context,
-                              const WebSize& icon_display_size_pixels);
+  KURL PickBestIconForDisplay(ExecutionContext* execution_context);
+
+  // Decodes the |data| to a SkBitmap using the image decoders and resizes it to
+  // be no larger than |icon_display_size_pixels_|.
+  void DecodeAndResizeImageOnBackgroundThread(
+      scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+      scoped_refptr<SegmentReader> data);
+
+  // Called when the image has been decoded and resized on a background thread.
+  void DidFinishDecoding();
 
-  bool stopped_ = false;
-  scoped_refptr<SharedBuffer> data_;
-  IconCallback icon_callback_;
   HeapVector<ManifestImageResource> icons_;
+  IconCallback icon_callback_;
+
   Member<ThreadableLoader> threadable_loader_;
+
+  // Size at which the icon will be presented to the user.
+  WebSize icon_display_size_pixels_;
+
+  // Data received from the ThreadableLoader. Will be invalidated when decoding
+  // of the image data starts.
+  scoped_refptr<SharedBuffer> data_;
+
+  // Result of decoding the icon on the background thread.
+  SkBitmap decoded_icon_;
+
+  // Whether the icon loading operation has been stopped. The process should
+  // be aborted then, and |icon_callback_| must not be invoked anymore.
+  bool stopped_ = false;
 };
 
 }  // namespace blink

third_party/blink/renderer/modules/background_fetch/background_fetch_icon_loader_test.cc

@@ -64,11 +64,15 @@ class BackgroundFetchIconLoaderTest : public PageTestBase {
 
   // Callback for BackgroundFetchIconLoader. This will set up the state of the
   // load as either success or failed based on whether the bitmap is empty.
-  void IconLoaded(const SkBitmap& bitmap) {
-    if (!bitmap.empty())
+  void IconLoaded(base::OnceClosure quit_closure, const SkBitmap& bitmap) {
+    bitmap_ = bitmap;
+
+    if (!bitmap_.isNull())
       loaded_ = BackgroundFetchLoadState::kLoadSuccessful;
     else
       loaded_ = BackgroundFetchLoadState::kLoadFailed;
+
+    std::move(quit_closure).Run();
   }
 
   ManifestImageResource CreateTestIcon(const String& url_str,
@@ -84,10 +88,14 @@ class BackgroundFetchIconLoaderTest : public PageTestBase {
   KURL PickRightIcon(HeapVector<ManifestImageResource> icons,
                      const WebSize& ideal_display_size) {
     loader_->icons_ = std::move(icons);
-    return loader_->PickBestIconForDisplay(GetContext(), ideal_display_size);
+    loader_->icon_display_size_pixels_ = ideal_display_size;
+
+    return loader_->PickBestIconForDisplay(GetContext());
   }
 
-  void LoadIcon(const KURL& url) {
+  void LoadIcon(const KURL& url,
+                const WebSize& maximum_size,
+                base::OnceClosure quit_closure) {
     ManifestImageResource icon;
     icon.setSrc(url.GetString());
     icon.setType("image/png");
@@ -97,8 +105,9 @@ class BackgroundFetchIconLoaderTest : public PageTestBase {
     loader_->icons_ = std::move(icons);
     loader_->DidGetIconDisplaySizeIfSoLoadIcon(
         GetContext(),
-        Bind(&BackgroundFetchIconLoaderTest::IconLoaded, WTF::Unretained(this)),
-        WebSize(192, 192));
+        WTF::Bind(&BackgroundFetchIconLoaderTest::IconLoaded,
+                  WTF::Unretained(this), WTF::Passed(std::move(quit_closure))),
+        maximum_size);
   }
 
   ExecutionContext* GetContext() const { return &GetDocument(); }
@@ -106,15 +115,30 @@ class BackgroundFetchIconLoaderTest : public PageTestBase {
  protected:
   ScopedTestingPlatformSupport<TestingPlatformSupport> platform_;
   BackgroundFetchLoadState loaded_ = BackgroundFetchLoadState::kNotLoaded;
+  SkBitmap bitmap_;
 
  private:
   Persistent<BackgroundFetchIconLoader> loader_;
 };
 
 TEST_F(BackgroundFetchIconLoaderTest, SuccessTest) {
-  LoadIcon(KURL(kBackgroundFetchImageLoaderIcon500x500FullPath));
+  base::RunLoop run_loop;
+
+  WebSize maximum_size{192, 168};
+  LoadIcon(KURL(kBackgroundFetchImageLoaderIcon500x500FullPath), maximum_size,
+           run_loop.QuitClosure());
+
   platform_->GetURLLoaderMockFactory()->ServeAsynchronousRequests();
-  EXPECT_EQ(BackgroundFetchLoadState::kLoadSuccessful, loaded_);
+
+  run_loop.Run();
+
+  ASSERT_EQ(BackgroundFetchLoadState::kLoadSuccessful, loaded_);
+  ASSERT_FALSE(bitmap_.drawsNothing());
+
+  // Resizing a 500x500 image to fit on a canvas of 192x168 pixels should yield
+  // a decoded image size of 168x168, avoiding image data to get lost.
+  EXPECT_EQ(bitmap_.width(), 168);
+  EXPECT_EQ(bitmap_.height(), 168);
 }
 
 TEST_F(BackgroundFetchIconLoaderTest, PickIconRelativePath) {