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

chrome/browser/manifest/manifest_icon_selector.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/manifest/manifest_icon_selector.h"
 
+#include <algorithm>
+#include <cmath>
 #include <limits>
 
 #include "base/strings/utf_string_conversions.h"
 #include "components/mime_util/mime_util.h"
 #include "content/public/browser/web_contents.h"
 #include "ui/gfx/screen.h"
 
 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(
     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() == preferred_icon_size_in_pixels_)
       return true;
   }
 
   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) {
     if (icons[i].density != density)
         continue;
 
     const std::vector<gfx::Size>& sizes = icons[i].sizes;
     for (size_t j = 0; j < sizes.size(); ++j) {
       if (sizes[j].height() != sizes[j].width())
         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);
+  best_index = FindBestMatchingIconForDensity(icons, density);
+  if (best_index != -1 &&
+        IconSizesContainsBiggerThanMinimum(icons[best_index].sizes))
+    return best_index;
 
-  return url;
+  best_index = FindBestMatchingIconForDensity(icons,
+      Manifest::Icon::kDefaultDensity);
+  if (best_index != -1 &&
+        IconSizesContainsBiggerThanMinimum(icons[best_index].sizes))
+    return best_index;
+
+  return -1;
 }
 
 
 // static
 bool ManifestIconSelector::IconSizesContainsAny(
     const std::vector<gfx::Size>& sizes) {
   for (size_t i = 0; i < sizes.size(); ++i) {
     if (sizes[i].IsEmpty())
       return true;
   }
-
   return false;
 }
 
 // static
 std::vector<Manifest::Icon> ManifestIconSelector::FilterIconsByType(
     const std::vector<content::Manifest::Icon>& icons) {
   std::vector<Manifest::Icon> result;
 
   for (size_t i = 0; i < icons.size(); ++i) {
     if (icons[i].type.is_null() ||
         mime_util::IsSupportedImageMimeType(
             base::UTF16ToUTF8(icons[i].type.string()))) {
       result.push_back(icons[i]);
     }
   }
 
   return result;
 }
 
 // static
 GURL ManifestIconSelector::FindBestMatchingIcon(
     const std::vector<Manifest::Icon>& unfiltered_icons,
     const float preferred_icon_size_in_dp,
     const gfx::Screen* screen) {
   const float device_scale_factor =
       screen->GetPrimaryDisplay().device_scale_factor();
   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.

 }