manifest: rework icon selector to include small icon cut-off

chrome/browser/manifest/manifest_icon_selector_unittest.cc

Index: chrome/browser/manifest/manifest_icon_selector_unittest.cc
diff --git a/chrome/browser/manifest/manifest_icon_selector_unittest.cc b/chrome/browser/manifest/manifest_icon_selector_unittest.cc
index cff4263a61b2b4c4bee34e54d036b466339bcdb2..41709b77dcad689643e27ce75b8f624faa8757d5 100644
--- a/chrome/browser/manifest/manifest_icon_selector_unittest.cc
+++ b/chrome/browser/manifest/manifest_icon_selector_unittest.cc
@@ -83,7 +83,7 @@ TEST_F(ManifestIconSelectorTest, MIMETypeFiltering) {
   // Icons with type specified to a MIME type that isn't a valid image MIME type
   // are ignored.
   std::vector<gfx::Size> sizes;
-  sizes.push_back(gfx::Size(10, 10));
+  sizes.push_back(gfx::Size(48, 48));
 
   std::vector<content::Manifest::Icon> icons;
   icons.push_back(
@@ -192,7 +192,7 @@ TEST_F(ManifestIconSelectorTest, DeviceDensityFirst) {
   // This test has three icons each are marked with sizes set to the preferred
   // icon size for the associated density.
   std::vector<gfx::Size> sizes;
-  sizes.push_back(gfx::Size(2, 2));
+  sizes.push_back(gfx::Size(97, 97));

[mlamouri (slow - plz ping), 2015/08/19 15:02:54]

Put a very high value so it's clear we don't filter on it.

[Lalit Maganti, 2015/08/20 14:53:50]

Done.

 
   std::vector<content::Manifest::Icon> icons;
   icons.push_back(CreateIcon("http://foo.com/icon_x1.png", "", 1.0, sizes));
@@ -216,7 +216,7 @@ TEST_F(ManifestIconSelectorTest, DeviceDensityFallback) {
   // If there is no perfect icon but and no icon of the current display density,
   // an icon of density 1.0 will be used.
   std::vector<gfx::Size> sizes;
-  sizes.push_back(gfx::Size(2, 2));
+  sizes.push_back(gfx::Size(97, 97));

[mlamouri (slow - plz ping), 2015/08/19 15:02:54]

ditto

[Lalit Maganti, 2015/08/20 14:53:49]

Done.

 
   std::vector<content::Manifest::Icon> icons;
   icons.push_back(CreateIcon("http://foo.com/icon_x1.png", "", 1.0, sizes));
@@ -227,6 +227,45 @@ TEST_F(ManifestIconSelectorTest, DeviceDensityFallback) {
   EXPECT_EQ("http://foo.com/icon_x1.png", url.spec());
 }
 
+TEST_F(ManifestIconSelectorTest, DeviceDensityFirstRejectsSmall) {
+  // We check that size is no smaller than one density bucket smaller if we have
+  // to resort to densities to find an icon.
+  std::vector<gfx::Size> sizes;
+  sizes.push_back(gfx::Size(47, 47));
+
+  std::vector<content::Manifest::Icon> icons;
+  icons.push_back(CreateIcon("http://foo.com/icon_x1.png", "", 1.0, sizes));
+  icons.push_back(CreateIcon("http://foo.com/icon_x2.png", "", 2.0, sizes));
+  icons.push_back(CreateIcon("http://foo.com/icon_x3.png", "", 3.0, sizes));
+
+  SetDisplayDeviceScaleFactor(1.0f);

[mlamouri (slow - plz ping), 2015/08/19 15:02:54]

nit: add comment explaining what happens.

[gone, 2015/08/19 22:31:56]

Yeah, I can't even tell what the test is supposed to be doing because the
comment doesn't make sense when everything is rejected.

[Lalit Maganti, 2015/08/20 14:53:50]

Made the comment much clearer.

+  GURL url = FindBestMatchingIcon(icons);
+  EXPECT_EQ("", url.spec());
+
+  SetDisplayDeviceScaleFactor(2.0f);
+  url = FindBestMatchingIcon(icons);
+  EXPECT_EQ("", url.spec());
+
+  SetDisplayDeviceScaleFactor(3.0f);
+  url = FindBestMatchingIcon(icons);
+  EXPECT_EQ("", url.spec());
+}
+
+TEST_F(ManifestIconSelectorTest, DeviceDensityFallbackRejectsSmall) {

[mlamouri (slow - plz ping), 2015/08/19 15:02:54]

I don't think that test is needed.

[Lalit Maganti, 2015/08/20 14:53:49]

Done.

+  // We check that size is no smaller than one density bucket smaller if we have
+  // to resort to densities to find an icon.
+  std::vector<gfx::Size> sizes;
+  sizes.push_back(gfx::Size(47, 47));
+
+  std::vector<content::Manifest::Icon> icons;
+  icons.push_back(CreateIcon("http://foo.com/icon_x1.png", "", 1.0, sizes));
+  icons.push_back(CreateIcon("http://foo.com/icon_x2.png", "", 2.0, sizes));
+
+  SetDisplayDeviceScaleFactor(3.0f);
+  GURL url = FindBestMatchingIcon(icons);
+  EXPECT_EQ("", url.spec());
+}
+
 TEST_F(ManifestIconSelectorTest, DoNotUseOtherDensities) {
   // If there are only icons of densities that are not the current display
   // density or the default density, they are ignored.
@@ -264,7 +303,7 @@ TEST_F(ManifestIconSelectorTest, ClosestIconToPreferred) {
   int big = GetPreferredIconSizeInDp() * 2;
   int very_big = GetPreferredIconSizeInDp() * 4;
 
-  // (very_small, bit_small) => bit_small
+  // (very_small, bit_small) => empty (since both are too small)
   {
     std::vector<gfx::Size> sizes_1;
     sizes_1.push_back(gfx::Size(very_small, very_small));
@@ -277,10 +316,10 @@ TEST_F(ManifestIconSelectorTest, ClosestIconToPreferred) {
     icons.push_back(CreateIcon("http://foo.com/icon.png", "", 1.0, sizes_2));
 
     GURL url = FindBestMatchingIcon(icons);
-    EXPECT_EQ("http://foo.com/icon.png", url.spec());
+    EXPECT_EQ("", url.spec());
   }
 
-  // (very_small, bit_small, smaller) => bit_small
+  // (very_small, bit_small, smaller) => empty (since both are too small)
   {
     std::vector<gfx::Size> sizes_1;
     sizes_1.push_back(gfx::Size(very_small, very_small));
@@ -297,7 +336,7 @@ TEST_F(ManifestIconSelectorTest, ClosestIconToPreferred) {
     icons.push_back(CreateIcon("http://foo.com/icon_no.png", "", 1.0, sizes_3));
 
     GURL url = FindBestMatchingIcon(icons);
-    EXPECT_EQ("http://foo.com/icon.png", url.spec());
+    EXPECT_EQ("", url.spec());
   }
 
   // (very_big, big) => big