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..ac5cf7a3df12d1613932e371a9a1375a4a5e4668 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::IconSizesContainsBiggerThanMinimum(
+    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,67 +60,79 @@ 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(
-    const std::vector<content::Manifest::Icon>& unfiltered_icons,
+int ManifestIconSelector::FindBestMatchingIcon(
+    const std::vector<content::Manifest::Icon>& icons,
     float density) {
-  GURL url;
-  std::vector<Manifest::Icon> icons = FilterIconsByType(unfiltered_icons);
+  int best_index = -1;
 
   // The first pass is to find the ideal icon. That icon is of the right size
   // with the default density or the device's density.
   for (size_t i = 0; i < icons.size(); ++i) {
     if (icons[i].density == density &&
         IconSizesContainsPreferredSize(icons[i].sizes)) {
-      return icons[i].src;
+      return i;
     }
 
     // If there is an icon with the right size but not the right density, keep
     // it on the side and only use it if nothing better is found.
     if (icons[i].density == Manifest::Icon::kDefaultDensity &&
         IconSizesContainsPreferredSize(icons[i].sizes)) {
-      url = icons[i].src;
+      best_index = i;
     }
   }
 
+  // Do an early return here if we have a suitable icon.

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

nit: remove comment.

[Lalit Maganti, 2015/08/21 10:46:37]

Done.

+  if (best_index != -1)
+    return best_index;
+
   // The second pass is to find an icon with 'any'. The current device scale
   // factor is preferred. Otherwise, the default scale factor is used.
   for (size_t i = 0; i < icons.size(); ++i) {
     if (icons[i].density == density &&
         IconSizesContainsAny(icons[i].sizes)) {
-      return icons[i].src;
+      return i;
     }
 
     // If there is an icon with 'any' but not the right density, keep it on the
     // side and only use it if nothing better is found.
     if (icons[i].density == Manifest::Icon::kDefaultDensity &&
         IconSizesContainsAny(icons[i].sizes)) {
-      url = icons[i].src;
+      best_index = i;
     }
   }
 
+  // Do an early return here if we have a suitable icon.

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

nit: remove comment.

[Lalit Maganti, 2015/08/21 10:46:37]

Done.

+  if (best_index != -1)
+    return best_index;
+
   // 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;
+  best_index = FindBestMatchingIconForDensity(icons, density);
+  if (best_index != -1 &&
+        IconSizesContainsBiggerThanMinimum(icons[best_index].sizes))
+    return best_index;
+
+  best_index = FindBestMatchingIconForDensity(icons,
+      Manifest::Icon::kDefaultDensity);
+  if (best_index != -1 &&
+        IconSizesContainsBiggerThanMinimum(icons[best_index].sizes))
+    return best_index;
+
+  return -1;
 }
 
 
@@ -116,7 +143,6 @@ bool ManifestIconSelector::IconSizesContainsAny(
     if (sizes[i].IsEmpty())
       return true;
   }
-
   return false;
 }
 
@@ -146,6 +172,18 @@ 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);
-  return selector.FindBestMatchingIcon(unfiltered_icons, device_scale_factor);
+  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;
+
+  std::vector<Manifest::Icon> icons =
+      ManifestIconSelector::FilterIconsByType(unfiltered_icons);
+
+  ManifestIconSelector selector(preferred_icon_size_in_pixels,
+                                minimum_icon_size_in_pixels);
+  int index = selector.FindBestMatchingIcon(icons, device_scale_factor);
+  if (index == -1)
+    return GURL();
+  return unfiltered_icons[index].src;

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

s/unfiltered_icons/icons/

[Lalit Maganti, 2015/08/21 10:46:37]

Done.

 }