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

chrome/browser/manifest/manifest_icon_selector.cc

Index: chrome/browser/manifest/manifest_icon_selector.cc
diff --git a/chrome/browser/manifest/manifest_icon_selector.cc b/chrome/browser/manifest/manifest_icon_selector.cc
index 7e3bd1f900508d23abdba6ff4efb41443fff4285..f6f5f4f8c0bb5d8e1973a3034579740605df350f 100644
--- a/chrome/browser/manifest/manifest_icon_selector.cc
+++ b/chrome/browser/manifest/manifest_icon_selector.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/manifest/manifest_icon_selector.h"
 
+#include <algorithm>
+#include <cmath>
 #include <limits>
 
 #include "base/strings/utf_string_conversions.h"
@@ -13,8 +15,10 @@
 
 using content::Manifest;
 
-ManifestIconSelector::ManifestIconSelector(float preferred_icon_size_in_pixels)
-    : preferred_icon_size_in_pixels_(preferred_icon_size_in_pixels) {
+ManifestIconSelector::ManifestIconSelector(float preferred_icon_size_in_pixels,
+                                           float minimum_icon_size_in_pixels)
+    : preferred_icon_size_in_pixels_(preferred_icon_size_in_pixels),
+      minimum_icon_size_in_pixels_(minimum_icon_size_in_pixels) {
 }
 
 bool ManifestIconSelector::IconSizesContainsPreferredSize(
@@ -29,10 +33,21 @@ bool ManifestIconSelector::IconSizesContainsPreferredSize(
   return false;
 }
 
-GURL ManifestIconSelector::FindBestMatchingIconForDensity(
+bool ManifestIconSelector::IconSizesBiggerThanMinimum(
+    const std::vector<gfx::Size>& sizes) {
+  for (size_t i = 0; i < sizes.size(); ++i) {
+    if (sizes[i].height() != sizes[i].width())
+      continue;
+    if (sizes[i].width() >= minimum_icon_size_in_pixels_)
+      return true;
+  }
+  return false;
+}
+
+int ManifestIconSelector::FindBestMatchingIconForDensity(
     const std::vector<content::Manifest::Icon>& icons,
     float density) {
-  GURL url;
+  int best_index = -1;
   int best_delta = std::numeric_limits<int>::min();
 
   for (size_t i = 0; i < icons.size(); ++i) {
@@ -45,18 +60,18 @@ GURL ManifestIconSelector::FindBestMatchingIconForDensity(
         continue;
       int delta = sizes[j].width() - preferred_icon_size_in_pixels_;
       if (delta == 0)
-        return icons[i].src;
+        return i;
       if (best_delta > 0 && delta < 0)
         continue;
       if ((best_delta > 0 && delta < best_delta) ||
           (best_delta < 0 && delta > best_delta)) {
-        url = icons[i].src;
+        best_index = i;
         best_delta = delta;
       }
     }
   }
 
-  return url;
+  return best_index;
 }
 
 GURL ManifestIconSelector::FindBestMatchingIcon(
@@ -97,15 +112,21 @@ GURL ManifestIconSelector::FindBestMatchingIcon(
     }
   }
 
+  // Do an early return here if we have a suitable URL.

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

Is the first pass's fallback URL (where you don't do an early return) less
preferred than the second pass's?  We don't even check the URL before
overwriting it.

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

Very good point. Mounir: is there a reason for this?

[mlamouri (slow - plz ping), 2015/08/20 22:18:18]

It's on purpose. We don't want to return in the first fallback because the
second loop might find a better icon. Then, it doesn't really matter which icon
is picked IMO. Feel free to change if you think otherwise.

+  if (url.is_valid())
+    return url;
+
   // The last pass will try to find the best suitable icon for the device's
   // scale factor. If none, another pass will be run using kDefaultDensity.
-  if (!url.is_valid())
-    url = FindBestMatchingIconForDensity(icons, density);
-  if (!url.is_valid())
-    url = FindBestMatchingIconForDensity(icons,
-                                         Manifest::Icon::kDefaultDensity);
-
-  return url;
+  int best_index = FindBestMatchingIconForDensity(icons, density);

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

For consistency, you could have a best_index only instead of url and best_index.
Then, you could return it.

It should simplify the code below too.

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

Done.

+  if (best_index == -1 ||

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

if (best_index != -1 && IconSizesBiggerThanMinimum(icons[best_index].sizes))
  return best_index;

best_index = FindBestMatchingIconForDensity(icons,
Manifest::Icon::kDefaultDensity);
if (best_index != -1 && IconSizesBiggerThanMinimum(icons[best_index].sizes))
  return best_index;

return -1;

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

Done.

+        !IconSizesBiggerThanMinimum(icons[best_index].sizes)) {
+    best_index = FindBestMatchingIconForDensity(
+        icons, Manifest::Icon::kDefaultDensity);
+  }
+  return best_index == -1 ||
+      !IconSizesBiggerThanMinimum(icons[best_index].sizes)
+      ? GURL() : icons[best_index].src;
 }
 
 
@@ -146,6 +167,12 @@ GURL ManifestIconSelector::FindBestMatchingIcon(
   const float preferred_icon_size_in_pixels =
       preferred_icon_size_in_dp * device_scale_factor;
 
-  ManifestIconSelector selector(preferred_icon_size_in_pixels);
+  const int minimum_scale_factor = std::max(
+      static_cast<int>(floor(device_scale_factor - 1)), 1);
+  const float minimum_icon_size_in_pixels =
+      preferred_icon_size_in_dp * minimum_scale_factor;
+
+  ManifestIconSelector selector(preferred_icon_size_in_pixels,
+                                minimum_icon_size_in_pixels);
   return selector.FindBestMatchingIcon(unfiltered_icons, device_scale_factor);

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

int index = selector.FindBestMatchingIcon(unfiltered_icons,
device_scale_factor);
if (index == -1)
  return GURL();
return unfiltered_icons[index].src;

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

Done.

 }