[Background Fetch] Update icon selection logic to use ManifestIconSelector code.

Background Fetch introduced a new way to select icons (Bug: 813564)

However, now that we have udpated our spec and code to use ImageResource
instead of IconDefinition to represent icons, we should reuse the Web Manifest's
icon selection code here.

This is the second step towards that.

Bug: 860507
Change-Id: I1e8795f8768610e22002a9a87b0c7bf8c33f9d0b
Reviewed-on: https://chromium-review.googlesource.com/1128086
Commit-Queue: Mugdha Lakhani <nator@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577148}

content/common/background_fetch/background_fetch_struct_traits_unittest.cc

@@ -20,7 +20,7 @@ namespace content {
 
 namespace {
 
-// Creates a new IconDefinition object for the given arguments.
+// Creates a new ImageResource object for the given arguments.
 blink::Manifest::ImageResource CreateIcon(const std::string& src,
                                           std::vector<gfx::Size> sizes,
                                           const std::string& type) {
@@ -71,7 +71,7 @@ TEST(BackgroundFetchStructTraitsTest, BackgroundFetchRegistrationRoundTrip) {
   EXPECT_EQ(roundtrip_registration.download_total, registration.download_total);
 }
 
-TEST(BackgroundFetchStructTraitsTest, IconDefinitionRoundtrip) {
+TEST(BackgroundFetchStructTraitsTest, ImageResourceRoundtrip) {
   blink::Manifest::ImageResource icon =
       CreateIcon("my_icon.png", {{256, 256}}, "image/png");
 

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

@@ -5,12 +5,14 @@
 #include "third_party/blink/renderer/modules/background_fetch/background_fetch_icon_loader.h"
 
 #include "skia/ext/image_operations.h"
+#include "third_party/blink/public/common/manifest/manifest_icon_selector.h"
 #include "third_party/blink/public/platform/web_size.h"
 #include "third_party/blink/public/platform/web_url_request.h"
 #include "third_party/blink/renderer/core/execution_context/execution_context.h"
 #include "third_party/blink/renderer/core/loader/threadable_loader.h"
 #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/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"
@@ -19,6 +21,7 @@
 #include "third_party/blink/renderer/platform/loader/fetch/resource_request.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"
 #include "third_party/blink/renderer/platform/wtf/threading.h"
 
 namespace blink {
@@ -26,9 +29,7 @@ namespace blink {
 namespace {
 
 const unsigned long kIconFetchTimeoutInMs = 30000;
-const int kMinimumIconSizeInPx = 16;
-const double kAnySizeScore = 0.8;
-const double kUnspecifiedSizeScore = 0.4;
+const int kMinimumIconSizeInPx = 0;
 
 }  // namespace
 
@@ -62,15 +63,14 @@ void BackgroundFetchIconLoader::DidGetIconDisplaySizeIfSoLoadIcon(
     return;
   }
 
-  int best_icon_index =
+  KURL best_icon_url =
       PickBestIconForDisplay(execution_context, icon_display_size_pixels);
-  if (best_icon_index < 0) {
+  if (best_icon_url.IsEmpty()) {
     // None of the icons provided was suitable.
     std::move(icon_callback).Run(SkBitmap());
     return;
   }
-  KURL best_icon_url =
-      execution_context->CompleteURL(icons_[best_icon_index].src());
+
   icon_callback_ = std::move(icon_callback);
 
   ThreadableLoaderOptions threadable_loader_options;
@@ -92,94 +92,16 @@ void BackgroundFetchIconLoader::DidGetIconDisplaySizeIfSoLoadIcon(
   threadable_loader_->Start(resource_request);
 }
 
-int BackgroundFetchIconLoader::PickBestIconForDisplay(
+KURL BackgroundFetchIconLoader::PickBestIconForDisplay(
     ExecutionContext* execution_context,
     const WebSize& icon_display_size_pixels) {
-  int best_index = -1;
-  double best_score = 0.0;
-  for (size_t i = 0; i < icons_.size(); ++i) {
-    // If the icon has no or invalid src, move on.
-    if (!icons_[i].hasSrc())
-      continue;
-    KURL icon_url = execution_context->CompleteURL(icons_[i].src());
-    if (!icon_url.IsValid() || icon_url.IsEmpty())
-      continue;
-
-    double score = GetIconScore(icons_[i], icon_display_size_pixels.width);
-    if (!score)
-      continue;
-    // According to the spec, if two icons get the same score, we must use the
-    // one that's declared last. (https://w3c.github.io/manifest/#icons-member).
-    if (score >= best_score) {
-      best_score = score;
-      best_index = i;
-    }
-  }
-  return best_index;
-}
+  std::vector<Manifest::ImageResource> icons;
+  for (const auto& icon : icons_)
+    icons.emplace_back(blink::ConvertManifestImageResource(icon));
 
-// The scoring works as follows:
-// When the size is "any", the icon size score is kAnySizeScore.
-// If unspecified, use the unspecified size score, kUnspecifiedSizeScore as
-// icon score.
-
-// For other sizes, the icon score lies in [0,1] and is computed by multiplying
-// the dominant size score and aspect ratio score.
-//
-// The dominant size score lies in [0, 1] and is computed using
-// dominant size and and |ideal_size|:
-//   - If dominant_size < kMinimumIconSizeInPx, the size score is 0.0.
-//   - For all other sizes, the score is calculated as
-//     1/(1 + abs(dominant_size-ideal_size))
-//   - If dominant_size < ideal_size, there is an upscaling penalty, which is
-//     dominant_size/ideal_size.
-//
-// The aspect ratio score lies in [0, 1] and is computed by dividing the short
-// edge length by the long edge. (Bias towards square icons assumed).
-//
-// Note: If this is an ico file containing multiple sizes, return the best
-// score.
-double BackgroundFetchIconLoader::GetIconScore(ManifestImageResource icon,
-                                               const int ideal_size) {
-  // Extract sizes from the icon definition, expressed as "<width>x<height>
-  // <width>x<height> ...."
-  if (!icon.hasSizes() || icon.sizes().IsEmpty())
-    return kUnspecifiedSizeScore;
-
-  String sizes = icon.sizes();
-  // if any size is set to "any" return kAnySizeScore;
-  if (sizes.LowerASCII() == "any")
-    return kAnySizeScore;
-
-  Vector<String> sizes_str;
-  sizes.Split(" ", false /* allow_empty_entries*/, sizes_str);
-
-  // Pick the first size.
-  // TODO(nator): Add support for multiple sizes (.ico files).
-  Vector<String> width_and_height_str;
-  sizes_str[0].Split("x", false /* allow_empty_entries */,
-                     width_and_height_str);
-  // If sizes isn't in this format, consider it as 'unspecified'.
-  if (width_and_height_str.size() != 2)
-    return kUnspecifiedSizeScore;
-  double width = width_and_height_str[0].ToDouble();
-  double height = width_and_height_str[1].ToDouble();
-
-  // Compute dominant size score
-  int dominant_size = std::max(width, height);
-  int short_size = std::min(width, height);
-  if (dominant_size < kMinimumIconSizeInPx)
-    return 0.0;
-
-  double dominant_size_score = 1.0 / (1.0 + abs(dominant_size - ideal_size));
-  if (dominant_size < ideal_size)
-    dominant_size_score = dominant_size_score * dominant_size / ideal_size;
-  // Compute aspect ratio score. If dominant_size is zero, we'd have returned
-  // by now.
-  double aspect_ratio_score = short_size / dominant_size;
-
-  // Compute icon score.
-  return aspect_ratio_score * dominant_size_score;
+  return KURL(ManifestIconSelector::FindBestMatchingIcon(
+      std::move(icons), icon_display_size_pixels.height, kMinimumIconSizeInPx,
+      Manifest::ImageResource::Purpose::ANY));
 }
 
 void BackgroundFetchIconLoader::Stop() {

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

@@ -10,7 +10,6 @@
 #include "third_party/blink/renderer/core/loader/threadable_loader.h"
 #include "third_party/blink/renderer/core/loader/threadable_loader_client.h"
 #include "third_party/blink/renderer/modules/background_fetch/background_fetch_type_converters.h"
-#include "third_party/blink/renderer/modules/manifest/image_resource.h"
 #include "third_party/blink/renderer/modules/modules_export.h"
 #include "third_party/blink/renderer/platform/shared_buffer.h"
 #include "third_party/skia/include/core/SkBitmap.h"
@@ -68,14 +67,10 @@ 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 index
-  // in the icons_ array.
-  int PickBestIconForDisplay(ExecutionContext* execution_context,
-                             const WebSize& icon_display_size_pixels);
-
-  // Get a score for the given icon, based on ideal_size. The icon with the
-  // highest score is chosen.
-  double GetIconScore(ManifestImageResource icon, const int ideal_size);
+  // 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);
 
   bool stopped_ = false;
   scoped_refptr<SharedBuffer> data_;

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

@@ -74,8 +74,8 @@ class BackgroundFetchIconLoaderTest : public PageTestBase {
     return icon;
   }
 
-  int PickRightIcon(HeapVector<ManifestImageResource> icons,
-                    const WebSize& ideal_display_size) {
+  KURL PickRightIcon(HeapVector<ManifestImageResource> icons,
+                     const WebSize& ideal_display_size) {
     loader_->icons_ = std::move(icons);
     return loader_->PickBestIconForDisplay(GetContext(), ideal_display_size);
   }
@@ -85,6 +85,7 @@ class BackgroundFetchIconLoaderTest : public PageTestBase {
     icon.setSrc(url.GetString());
     icon.setType("image/png");
     icon.setSizes("500x500");
+    icon.setPurpose("ANY");
     HeapVector<ManifestImageResource> icons(1, icon);
     loader_->icons_ = std::move(icons);
     loader_->DidGetIconDisplaySizeIfSoLoadIcon(
@@ -123,8 +124,8 @@ TEST_F(BackgroundFetchIconLoaderTest, PickRightIconTest) {
   icons.push_back(icon1);
   icons.push_back(icon2);
 
-  int index = PickRightIcon(std::move(icons), WebSize(50, 50));
-  EXPECT_EQ(index, 1);
+  KURL best_icon = PickRightIcon(std::move(icons), WebSize(50, 50));
+  EXPECT_EQ(best_icon, KURL(kBackgroundFetchImageLoaderIcon48x48));
 }
 
 TEST_F(BackgroundFetchIconLoaderTest, PickRightIconGivenAnyTest) {
@@ -140,8 +141,8 @@ TEST_F(BackgroundFetchIconLoaderTest, PickRightIconGivenAnyTest) {
   icons.push_back(icon1);
   icons.push_back(icon2);
 
-  int index = PickRightIcon(std::move(icons), WebSize(50, 50));
-  EXPECT_EQ(index, 2);
+  KURL best_icon = PickRightIcon(std::move(icons), WebSize(50, 50));
+  EXPECT_EQ(best_icon, KURL(kBackgroundFetchImageLoaderIcon));
 }
 
 TEST_F(BackgroundFetchIconLoaderTest, PickRightIconWithTieBreakTest) {
@@ -157,8 +158,8 @@ TEST_F(BackgroundFetchIconLoaderTest, PickRightIconWithTieBreakTest) {
   icons.push_back(icon0);
   icons.push_back(icon1);
   icons.push_back(icon2);
-  int index = PickRightIcon(std::move(icons), WebSize(50, 50));
-  EXPECT_EQ(index, 2);
+  KURL best_icon = PickRightIcon(std::move(icons), WebSize(50, 50));
+  EXPECT_EQ(best_icon, KURL(kBackgroundFetchImageLoaderIcon3000x2000));
 }
 
 }  // namespace blink

third_party/blink/renderer/modules/manifest/image_resource_type_converters.cc

@@ -9,6 +9,7 @@
 #include "third_party/blink/public/platform/web_size.h"
 #include "third_party/blink/public/platform/web_string.h"
 #include "third_party/blink/public/platform/web_vector.h"
+#include "third_party/blink/renderer/modules/manifest/image_resource.h"
 #include "third_party/blink/renderer/platform/weborigin/kurl.h"
 #include "third_party/blink/renderer/platform/wtf/hash_set.h"
 #include "third_party/blink/renderer/platform/wtf/text/wtf_string.h"
@@ -105,3 +106,31 @@ blink::mojom::blink::ManifestImageResourcePtr TypeConverter<
 }
 
 }  // namespace mojo
+
+namespace blink {
+
+Manifest::ImageResource ConvertManifestImageResource(
+    const ManifestImageResource& icon) {
+  Manifest::ImageResource manifest_icon;
+  manifest_icon.src = blink::KURL(icon.src());
+  manifest_icon.type = WebString(mojo::ParseType(icon.type())).Utf16();
+
+  // Parse 'purpose'
+  const auto purposes = mojo::ParsePurpose(icon.purpose());
+  // ParsePurpose() would've weeded out any purposes that're not ANY or BADGE.
+  for (auto purpose : purposes) {
+    manifest_icon.purpose.emplace_back(
+        purpose == mojo::Purpose::ANY
+            ? Manifest::ImageResource::Purpose::ANY
+            : Manifest::ImageResource::Purpose::BADGE);
+  }
+  // Parse 'sizes'.
+  WTF::Vector<WebSize> sizes = mojo::ParseSizes(icon.sizes());
+  for (const auto& size : sizes) {
+    manifest_icon.sizes.emplace_back(gfx::Size(size.height, size.width));
+  }
+
+  return manifest_icon;
+}
+
+}  // namespace blink

third_party/blink/renderer/modules/manifest/image_resource_type_converters.h

@@ -5,10 +5,18 @@
 #ifndef THIRD_PARTY_BLINK_RENDERER_MODULES_MANIFEST_IMAGE_RESOURCE_TYPE_CONVERTERS_H_
 #define THIRD_PARTY_BLINK_RENDERER_MODULES_MANIFEST_IMAGE_RESOURCE_TYPE_CONVERTERS_H_
 
+#include "third_party/blink/public/common/manifest/manifest.h"
 #include "third_party/blink/public/mojom/manifest/manifest.mojom-blink.h"
-#include "third_party/blink/renderer/modules/manifest/image_resource.h"
 #include "third_party/blink/renderer/modules/modules_export.h"
 
+namespace blink {
+
+class ManifestImageResource;
+MODULES_EXPORT Manifest::ImageResource ConvertManifestImageResource(
+    const ManifestImageResource& image_resource);
+
+}  // namespace blink
+
 namespace mojo {
 
 template <>

third_party/blink/renderer/modules/manifest/image_resource_type_converters_test.cc

@@ -7,6 +7,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/blink/public/mojom/manifest/manifest.mojom-blink.h"
 #include "third_party/blink/public/platform/web_size.h"
+#include "third_party/blink/public/platform/web_string.h"
 #include "third_party/blink/renderer/modules/manifest/image_resource.h"
 #include "third_party/blink/renderer/platform/weborigin/kurl.h"
 #include "third_party/blink/renderer/platform/wtf/text/wtf_string.h"
@@ -184,6 +185,22 @@ TEST(ImageResourceConverter, ExampleValueTest) {
   EXPECT_EQ(expected_resource, ManifestImageResource::From(resource));
 }
 
+TEST(ImageResourceConverter, BlinkToMojoTypeTest) {
+  blink::ManifestImageResource icon;
+  icon.setSrc("http://example.com/lolcat.jpg");
+  icon.setPurpose("BADGE");
+  icon.setSizes("32x32 64x64 128x128");
+  icon.setType("image/jpeg");
+
+  blink::Manifest::ImageResource mojo_icon =
+      blink::ConvertManifestImageResource(icon);
+  EXPECT_EQ(mojo_icon.src.spec(), "http://example.com/lolcat.jpg");
+  EXPECT_EQ(mojo_icon.type, blink::WebString("image/jpeg").Utf16());
+  EXPECT_EQ(mojo_icon.sizes[1], gfx::Size(64, 64));
+  EXPECT_EQ(mojo_icon.purpose[0],
+            blink::Manifest::ImageResource::Purpose::BADGE);
+}
+
 }  // namespace
 
 }  // namespace mojo