Always select best favicon bitmap

components/favicon/core/favicon_handler.h

Index: components/favicon/core/favicon_handler.h
diff --git a/components/favicon/core/favicon_handler.h b/components/favicon/core/favicon_handler.h
index f3d9d4b662c5d9c6f034cacffc7f5a8cfd67d9db..af0694b32b10a9e72a390016de281f1a3fd2ae22 100644
--- a/components/favicon/core/favicon_handler.h
+++ b/components/favicon/core/favicon_handler.h
@@ -7,6 +7,7 @@
 
 #include <stddef.h>
 
+#include <list>
 #include <map>
 #include <vector>
 
@@ -14,16 +15,16 @@
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
+#include "base/optional.h"
 #include "base/task/cancelable_task_tracker.h"
 #include "components/favicon/core/favicon_driver_observer.h"
 #include "components/favicon/core/favicon_url.h"
 #include "components/favicon_base/favicon_callback.h"
 #include "ui/gfx/favicon_size.h"
 #include "ui/gfx/image/image.h"
+#include "ui/gfx/image/image_skia.h"
 #include "url/gurl.h"
 
-class SkBitmap;
-
 namespace favicon {
 
 class FaviconService;
@@ -68,14 +69,113 @@ class FaviconService;
 // favicon.
 //
 // When the renderer downloads favicons, it considers the entire list of
-// favicon candidates, if |download_largest_favicon_| is true, the largest
-// favicon will be used, otherwise the one that best matches the preferred size
-// is chosen (or the first one if there is no preferred  size). Once the
-// matching favicon has been determined, SetFavicon is called which updates
-// the page's favicon and notifies the database to save the favicon.
+// favicon candidates. Among those, the one that matches |target_spec_|
+// is chosen first. Once the matching favicon has been determined, SetFavicon
+// is called which updates the page's favicon and notifies the database to save
+// the favicon.
 
 class FaviconHandler {
  public:
+  class TargetSizeSpec {
+   public:
+    static TargetSizeSpec ForLargest();
+    static std::vector<TargetSizeSpec> For16x16Dips();
+
+    TargetSizeSpec(const TargetSizeSpec&);
+    ~TargetSizeSpec();
+
+    TargetSizeSpec(TargetSizeSpec&&);
+    TargetSizeSpec& operator=(TargetSizeSpec&&);
+
+    // Returns the target pixel size.
+    int PixelSize() const { return pixel_size_; }
+
+    // Returns the target scale factor.
+    float ScaleFactor() const { return scale_factor_; }
+
+    // Returns whether the spec is requiring the best-matching bitmap only.
+    bool WantsBestBitmapOnly() const;
+
+    // Checks if |bitmap_results| contains a bitmap satistying this spec.
+    bool HasMatchingResult(
+        const std::vector<favicon_base::FaviconRawBitmapResult>& bitmap_results)
+        const;
+
+   private:
+    TargetSizeSpec();
+
+    int pixel_size_;
+    float scale_factor_;
+
+    // If true, FaviconHandler will resample candidates whenever needed to
+    // ensure that bitmaps for all |pixel_sizes| are produced.
+    bool ensure_exact_size_;
+  };
+
+  // Used to track a candidate for the favicon.
+  struct BitmapCandidate {
+    // Builds a scored candidate by selecting the best bitmap size.
+    BitmapCandidate(const TargetSizeSpec& target_size_spec,
+                    const favicon::FaviconURL& favicon_url);
+    BitmapCandidate(const BitmapCandidate&);
+    BitmapCandidate(BitmapCandidate&&);
+    BitmapCandidate& operator=(BitmapCandidate&&);
+
+    GURL icon_url;
+    // Size hint provided by the page or (0, 0).
+    gfx::Size icon_size;
+    favicon_base::IconType icon_type;
+    float score;
+  };
+
+  class BitmapCandidateQueue {

[pkotwicz, 2017/03/16 05:31:45]

It is weird for a queue to have so much logic in it. Rename this to
FaviconSelector perhaps?

+   public:
+    // |candidates| must be non-empty.
+    BitmapCandidateQueue(const TargetSizeSpec& target_size_spec,
+                         const std::vector<favicon::FaviconURL>& candidates);
+    ~BitmapCandidateQueue();
+    BitmapCandidateQueue(BitmapCandidateQueue&&);
+    BitmapCandidateQueue& operator=(BitmapCandidateQueue&&);
+
+    const TargetSizeSpec& TargetSizeSpec() const { return target_size_spec_; }
+
+    // Returns the next candidate to be processed, or an empty value if no
+    // further work is demanded, although clients are allowed to feed in new
+    // downloads via ProcessDownloadedImage().
+    base::Optional<BitmapCandidate> DequeueCandidate();
+
+    // Returns the current candidate without dequeueing it.
+    const BitmapCandidate* CurrentCandidate() const;
+
+    void ProcessDownloadedImage(const BitmapCandidate& candidate,
+                                const std::vector<SkBitmap>& bitmaps);
+
+    // Returns the best-matching bitmap downloaded/processed so far, or null if
+    // none available. If non-null, it's guarantee to contain a valid.
+    // |bitmap_result|.
+    const std::pair<BitmapCandidate, SkBitmap>* BestBitmap() const {
+      return best_candidate_.get();
+    }
+
+   private:
+    // Returns whether a good enough bitmap was found (i.e. no further work
+    // demanded by this queue, although callers are allowed to process and feed
+    // in more processed downloads)
+    bool IsSatisfied() const;
+
+    // Desired icon size.
+    class TargetSizeSpec target_size_spec_;
+
+    // The prioritized favicon candidates from the page. There cannot be
+    // multiple candidates for the same icon URL.
+    std::list<BitmapCandidate> pending_candidates_;
+
+    // Best bitmap we've seen so far. null if none.
+    std::unique_ptr<std::pair<BitmapCandidate, SkBitmap>> best_candidate_;

[pkotwicz, 2017/03/16 05:31:45]

I think that it would be easier to have:
best_candidate_
best_score_
best_bitmap_

as separate variables

This would allow you to remove BitmapCandidate::score

+
+    DISALLOW_COPY_AND_ASSIGN(BitmapCandidateQueue);
+  };
+
   class Delegate {
    public:
     // Mimics WebContents::ImageDownloadCallback.
@@ -155,26 +255,13 @@ class FaviconHandler {
     favicon_base::IconType icon_type;
   };
 
-  // Used to track a candidate for the favicon.
-  struct FaviconCandidate {
-    FaviconCandidate();
-    ~FaviconCandidate();
-
-    FaviconCandidate(const GURL& image_url,
-                     const gfx::Image& image,
-                     float score,
-                     favicon_base::IconType icon_type);
-
-    GURL image_url;
-    gfx::Image image;
-    float score;
-    favicon_base::IconType icon_type;
-  };
-
   // Returns the bit mask of favicon_base::IconType based on the handler's type.
   static int GetIconTypesFromHandlerType(
       FaviconDriverObserver::NotificationIconType handler_type);
 
+  std::vector<int> GetTargetPixelSizes() const;
+  std::map<int, float> GetTargetPixelSizesAndScaleFactors() const;
+
   // Called when the history request for favicon data mapped to |url_| has
   // completed and the renderer has told us the icon URLs used by |url_|
   void OnGotInitialHistoryDataAndIconURLCandidates();
@@ -187,7 +274,7 @@ class FaviconHandler {
   // current candidate favicon from the renderer. Otherwise requests data for
   // the current favicon from history. If data is requested from history,
   // OnFaviconData() is called with the history data once it has been retrieved.
-  void DownloadCurrentCandidateOrAskFaviconService();
+  void DownloadNextCandidateOrAskFaviconService();
 
   // See description above class for details.
   void OnFaviconData(const std::vector<favicon_base::FaviconRawBitmapResult>&
@@ -208,10 +295,10 @@ class FaviconHandler {
 
   bool ShouldSaveFavicon();
 
-  // Updates |favicon_candidate_| and returns true if it is an exact match.
+  // Updates candidate queues and returns true if no more candidates should be
+  // processed (e.g. an exact match was found).
   bool UpdateFaviconCandidate(const GURL& image_url,
                               const gfx::Image& image,
-                              float score,
                               favicon_base::IconType icon_type);
 
   // Sets the image data for the favicon.
@@ -231,19 +318,6 @@ class FaviconHandler {
                             favicon_base::IconType icon_type,
                             const gfx::Image& image);
 
-  // Return the current candidate if any.
-  favicon::FaviconURL* current_candidate() {
-    return current_candidate_index_ < image_urls_.size()
-               ? &image_urls_[current_candidate_index_]
-               : nullptr;
-  }
-
-  // Returns the preferred size of the image. 0 means no preference (any size
-  // will do).
-  int preferred_icon_size() const {
-    return download_largest_icon_ ? 0 : gfx::kFaviconSize;
-  }
-
   // Used for FaviconService requests.
   base::CancelableTaskTracker cancelable_task_tracker_;
 
@@ -263,8 +337,7 @@ class FaviconHandler {
   bool initial_history_result_expired_or_incomplete_;
 
   // Whether FaviconHandler should ignore history state and determine the
-  // optimal icon URL out of |image_urls_| for |url_| by downloading
-  // |image_urls_| one by one.
+  // optimal icon URL out of downloading input candidates.
   bool redownload_icons_;
 
   // Requests to the renderer to download favicons.
@@ -274,10 +347,15 @@ class FaviconHandler {
   // The combination of the supported icon types.
   const int icon_types_;
 
-  // Whether the largest icon should be downloaded.
-  const bool download_largest_icon_;
+  // The target "buckets" or requirements that the FaviconHandler will try to
+  // fulfill.
+  const std::vector<TargetSizeSpec> target_size_specs_;
+  std::vector<BitmapCandidateQueue> candidate_queues_;
+
+  // Current candidate being processed.
+  base::Optional<BitmapCandidate> current_candidate_;
 
-  // The prioritized favicon candidates from the page back from the renderer.
+  // The filtered (per icon type) candidates provided by the page.
   std::vector<favicon::FaviconURL> image_urls_;
 
   // The icon URL and the icon type of the favicon in the most recent
@@ -292,16 +370,6 @@ class FaviconHandler {
   // This handler's delegate.
   Delegate* delegate_;
 
-  // The index of the favicon URL in |image_urls_| which is currently being
-  // requested from history or downloaded.
-  size_t current_candidate_index_;
-
-  // Best image we've seen so far.  As images are downloaded from the page they
-  // are stored here. When there is an exact match, or no more images are
-  // available the favicon service and the current page are updated (assuming
-  // the image is for a favicon).
-  FaviconCandidate best_favicon_candidate_;
-
   base::WeakPtrFactory<FaviconHandler> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(FaviconHandler);