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::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) {
     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,
     float density) {
   GURL url;
   std::vector<Manifest::Icon> icons = FilterIconsByType(unfiltered_icons);
 
   // 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.
@@ skipping 20 matching lines @@
     }
 
     // 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;
     }
   }
 
+  // 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;
 }
 
 
 // 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;
   }
@@ skipping 20 matching lines @@
 // 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);
+  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.

 }