Refactor ManifestIconSelector and update it for Manifest.icon.purpose

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 139511b750e1cf8c17314f8a5ee703b02105c065..29d5a1a78f5802ba79634492750e69db2ec14447 100644
--- a/chrome/browser/manifest/manifest_icon_selector.cc
+++ b/chrome/browser/manifest/manifest_icon_selector.cc
@@ -4,141 +4,77 @@
 
 #include "chrome/browser/manifest/manifest_icon_selector.h"
 
-#include <stddef.h>
-
-#include <algorithm>
-#include <cmath>
 #include <limits>
 
+#include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/mime_util/mime_util.h"
-#include "content/public/browser/web_contents.h"
 
-using content::Manifest;
+// static
+GURL ManifestIconSelector::FindBestMatchingIcon(
+    const std::vector<content::Manifest::Icon>& icons,
+    int ideal_icon_size_in_px,
+    int minimum_icon_size_in_px,
+    content::Manifest::Icon::IconPurpose purpose) {
+  // Icon with exact matching size has priority over icon with size "any", which
+  // has priority over icon with closest matching size.
+  int latest_size_any_index = -1;
+  int closest_size_match_index = -1;
+  int best_delta_in_size = std::numeric_limits<int>::min();
 
-ManifestIconSelector::ManifestIconSelector(int ideal_icon_size_in_px,
-                                           int minimum_icon_size_in_px)
-    : ideal_icon_size_in_px_(ideal_icon_size_in_px),
-      minimum_icon_size_in_px_(minimum_icon_size_in_px) {
-}
+  for (size_t i = 0; i < icons.size(); ++i) {
+    const auto& icon = icons[i];
 
-bool ManifestIconSelector::IconSizesContainsPreferredSize(
-    const std::vector<gfx::Size>& sizes) const {
-  for (size_t i = 0; i < sizes.size(); ++i) {
-    if (sizes[i].height() != sizes[i].width())
+    // Check for supported image MIME types.
+    if (!(icon.type.empty() ||
+          mime_util::IsSupportedImageMimeType(base::UTF16ToUTF8(icon.type)))) {

[pkotwicz, 2017/01/31 18:24:53]

Nit: Expand this out

icon.type.empty() &&
!mime_util::IsSupportedImageMimeType(base::UTF16ToUTF8(icon.type))

[F, 2017/01/31 18:42:50]

Done.

       continue;
-    if (sizes[i].width() == ideal_icon_size_in_px_)
-      return true;
-  }
-
-  return false;
-}
+    }
 
-bool ManifestIconSelector::IconSizesContainsBiggerThanMinimumSize(
-    const std::vector<gfx::Size>& sizes) const {
-  for (size_t i = 0; i < sizes.size(); ++i) {
-    if (sizes[i].height() != sizes[i].width())
+    // Check for icon purpose.
+    if (!base::ContainsValue(icon.purpose, purpose))

[pkotwicz, 2017/01/31 18:24:53]

Cool! I didn't know about base::ContainsValue()

[F, 2017/01/31 18:42:50]

:)

       continue;
-    if (sizes[i].width() >= minimum_icon_size_in_px_)
-      return true;
-  }
-  return false;
-}
-
-int ManifestIconSelector::FindClosestIconToIdealSize(
-    const std::vector<content::Manifest::Icon>& icons) const {
-  int best_index = -1;
-  int best_delta = std::numeric_limits<int>::min();
 
-  for (size_t i = 0; i < icons.size(); ++i) {
-    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())
+    // Check for size constraints.
+    for (const auto& size : icon.sizes) {

[pkotwicz, 2017/01/31 18:24:53]

Nit: "const auto& size" -> "const gfx::Size& size". I only use auto when it
saves a lot of typing

[F, 2017/01/31 18:42:50]

Done.

+      // Check for size "any". Return this icon if no better one is found.

[pkotwicz, 2017/01/31 18:24:53]

Remove "Return this icon if no better one is found." it is obvious from the code

[F, 2017/01/31 18:42:50]

Resolved offline.

+      if (size.IsEmpty()) {
+        latest_size_any_index = i;
         continue;
-      int delta = sizes[j].width() - ideal_icon_size_in_px_;
-      if (delta == 0)
-        return i;
-      if (best_delta > 0 && delta < 0)
-        continue;
-      if ((best_delta > 0 && delta < best_delta) ||
-          (best_delta < 0 && delta > best_delta)) {
-        best_index = i;
-        best_delta = delta;
       }
-    }
-  }
-
-  return best_index;
-}
-
-int ManifestIconSelector::FindBestMatchingIcon(
-    const std::vector<content::Manifest::Icon>& icons) const {
-  int best_index = -1;
-
-  // The first pass is to find the ideal icon - one with the exact right size.
-  for (size_t i = 0; i < icons.size(); ++i) {
-    if (IconSizesContainsPreferredSize(icons[i].sizes))
-      return i;
-
-    // If there is an icon size 'any', keep it on the side and only use it if
-    // nothing better is found.
-    if (IconSizesContainsAny(icons[i].sizes))
-      best_index = i;
-  }
-  if (best_index != -1)
-    return best_index;
 
-  // The last pass will try to find the smallest icon larger than the ideal
-  // size, or the largest icon smaller than the ideal size.
-  best_index = FindClosestIconToIdealSize(icons);
-
-  if (best_index != -1 &&
-        IconSizesContainsBiggerThanMinimumSize(icons[best_index].sizes))
-    return best_index;
+      // Check for squareness.
+      if (size.width() != size.height())
+        continue;
 
-  return -1;
-}
+      // Check for minimum size.
+      if (size.width() < minimum_icon_size_in_px)
+        continue;
 
+      // Check for ideal size. Return this icon immediately.

[pkotwicz, 2017/01/31 18:24:53]

Remove "Return this icon immediately." It is obvious from the code

[F, 2017/01/31 18:42:50]

Resolved offline.

+      if (size.width() == ideal_icon_size_in_px)
+        return icon.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;
-  }
-  return false;
-}
+      // Check for closest match.
+      int delta = size.width() - ideal_icon_size_in_px;
 
-// static
-std::vector<Manifest::Icon> ManifestIconSelector::FilterIconsByType(
-    const std::vector<content::Manifest::Icon>& icons) {
-  std::vector<Manifest::Icon> result;
+      // Smallest icon larger than ideal size has priority over largest icon
+      // smaller than ideal size.
+      if (best_delta_in_size > 0 && delta < 0)
+        continue;
 
-  for (size_t i = 0; i < icons.size(); ++i) {
-    if (icons[i].type.empty() ||
-        mime_util::IsSupportedImageMimeType(base::UTF16ToUTF8(icons[i].type))) {
-      result.push_back(icons[i]);
+      if ((best_delta_in_size > 0 && delta < best_delta_in_size) ||
+          (best_delta_in_size < 0 && delta > best_delta_in_size)) {
+        closest_size_match_index = i;
+        best_delta_in_size = delta;
+      }
     }
   }
 
-  return result;
-}
-
-// static
-GURL ManifestIconSelector::FindBestMatchingIcon(
-    const std::vector<Manifest::Icon>& unfiltered_icons,
-    const int ideal_icon_size_in_px,
-    const int minimum_icon_size_in_px) {
-  DCHECK(minimum_icon_size_in_px <= ideal_icon_size_in_px);

[pkotwicz, 2017/01/31 18:24:53]

Shouldn't we keep this DCHECK?

[F, 2017/01/31 18:42:50]

Done. Thanks!

-
-  std::vector<Manifest::Icon> icons =
-      ManifestIconSelector::FilterIconsByType(unfiltered_icons);
-
-  ManifestIconSelector selector(ideal_icon_size_in_px,
-                                minimum_icon_size_in_px);
-  int index = selector.FindBestMatchingIcon(icons);
-  if (index == -1)
+  if (latest_size_any_index != -1)
+    return icons[latest_size_any_index].src;
+  else if (closest_size_match_index != -1)
+    return icons[closest_size_match_index].src;
+  else
     return GURL();
-  return icons[index].src;
 }