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

chrome/browser/manifest/manifest_icon_selector.h

 // 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.
 
 #ifndef CHROME_BROWSER_MANIFEST_MANIFEST_ICON_SELECTOR_H_
 #define CHROME_BROWSER_MANIFEST_MANIFEST_ICON_SELECTOR_H_
 
 #include "base/basictypes.h"
 #include "content/public/common/manifest.h"
 #include "url/gurl.h"
@@ skipping 22 matching lines @@
   // If/when this class is generalized, it may be a good idea to switch this to
   // taking in pixels, instead.
   //
   // Returns the icon url if a suitable icon is found. An empty URL otherwise.
   static GURL FindBestMatchingIcon(
       const std::vector<content::Manifest::Icon>& icons,
       float preferred_icon_size_in_dp,
       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
   // the Manifest.
   // Returns the icon url if a suitable icon is found. An empty URL otherwise.
   GURL FindBestMatchingIcon(

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

Maybe this could return an int too?

       const std::vector<content::Manifest::Icon>& icons,
       float density);
 
   // 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(
       const std::vector<content::Manifest::Icon>& icons);
 
   // Returns whether the 'any' (ie. gfx::Size(0,0)) is in |sizes|.
   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;
 
   DISALLOW_COPY_AND_ASSIGN(ManifestIconSelector);
 };
 
 #endif  // CHROME_BROWSER_MANIFEST_MANIFEST_ICON_SELECTOR_H_