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

chrome/browser/manifest/manifest_icon_selector.h

Index: chrome/browser/manifest/manifest_icon_selector.h
diff --git a/chrome/browser/manifest/manifest_icon_selector.h b/chrome/browser/manifest/manifest_icon_selector.h
index c0c582d729c26cf2440f97cedfa247aed36ea4f5..4ac1d45cb9d95943c7ddb05b22b6e8dbceb56572 100644
--- a/chrome/browser/manifest/manifest_icon_selector.h
+++ b/chrome/browser/manifest/manifest_icon_selector.h
@@ -40,7 +40,8 @@ class ManifestIconSelector {
       const gfx::Screen* screen);
 
  private:
-  explicit ManifestIconSelector(float preferred_icon_size_in_pixels);
+  ManifestIconSelector(float preferred_icon_size_in_pixels,
+                       float minimum_icon_size_in_pixels);

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

You should update the public method to make it clear to the callers that the
icon will be of at least some size.


We might want to consider exposing the minimum size in the public method later.

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

Done.

Yes similar to the mention of pixels, doing this when the class is generalized
may be a good idea.

   virtual ~ManifestIconSelector() {}
 
   // Runs the algorithm to find the best matching icon in the icons listed in
@@ -53,14 +54,18 @@ class ManifestIconSelector {
   // Runs an algorithm only based on icon declared sizes. It will try to find
   // size that is the closest to preferred_icon_size_in_pixels_ but bigger than
   // preferred_icon_size_in_pixels_ if possible.
-  // Returns the icon url if a suitable icon is found. An empty URL otherwise.
-  GURL FindBestMatchingIconForDensity(
+  // Returns the index of a suitable icon if one is found. -1 otherwise.
+  int FindBestMatchingIconForDensity(
       const std::vector<content::Manifest::Icon>& icons,
       float density);
 
   // Returns whether the |preferred_icon_size_in_pixels_| is in |sizes|.
   bool IconSizesContainsPreferredSize(const std::vector<gfx::Size>& sizes);
 
+  // Returns whether a size bigger than |minimun_icon_size_in_pixels_| is in
+  // |sizes|.
+  bool IconSizesBiggerThanMinimum(const std::vector<gfx::Size>& sizes);

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

nit: for consistency, rename to "IconSizesBiggerThanMinimum"

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

did you mean IconSizesContainsBiggerThanMinimum?

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

Done.

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

I meant IconSizesContainsBiggerThanMinimumSize()

[mlamouri (slow - plz ping), 2015/08/21 10:50:54]

I think you missed that comment.

[Lalit Maganti, 2015/08/21 11:18:20]

Done. (Yeah I missed it because I usually read comments by going through the
latest patch and looking at the comments - probably a recipe for missing
comments on old patches - will be more careful of this :/)

+
   // Returns an array containing the items in |icons| without the unsupported
   // image MIME types.
   static std::vector<content::Manifest::Icon> FilterIconsByType(
@@ -70,6 +75,7 @@ class ManifestIconSelector {
   static bool IconSizesContainsAny(const std::vector<gfx::Size>& sizes);
 
   const int preferred_icon_size_in_pixels_;
+  const int minimum_icon_size_in_pixels_;
 
   friend class ManifestIconSelectorTest;