Find Favicon in priority of icon_type.

chrome/browser/favicon/favicon_service.h

 // Copyright (c) 2012 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_FAVICON_FAVICON_SERVICE_H_
 #define CHROME_BROWSER_FAVICON_FAVICON_SERVICE_H_
 
 #include <vector>
 
 #include "base/callback.h"
@@ skipping 28 matching lines @@
   virtual ~FaviconService();
 
   // Auxiliary argument structure for requesting favicons for URLs.
   struct FaviconForURLParams {
     FaviconForURLParams(Profile* profile,
                         const GURL& page_url,
                         int icon_types,
                         int desired_size_in_dip)
         : profile(profile),
           page_url(page_url),
-          icon_types(icon_types),
-          desired_size_in_dip(desired_size_in_dip) {}
+          desired_size_in_dip(desired_size_in_dip),
+          threshold_for_next_icon_types(0) {
+      icon_types_priority.push_back(icon_types);
+    }
+
+    // Used to get best fit icon with icon types priority, see
+    // HistoryService::GetFaviconsForURL.
+    FaviconForURLParams(Profile* profile,
+                        const GURL& page_url,
+                        const std::vector<int> icon_types_priority,

[sky, 2013/10/08 23:47:52]

const std::vector<int>&

[michaelbai, 2013/10/10 05:51:43]

Done.

+                        int desired_size_in_dip,
+                        int threshold_for_next_icon_types)
+        : profile(profile),
+          page_url(page_url),
+          icon_types_priority(icon_types_priority),
+          desired_size_in_dip(desired_size_in_dip),
+          threshold_for_next_icon_types(threshold_for_next_icon_types) {}

[pkotwicz, 2013/10/09 17:11:51]

I would expect to have a vector of thresholds with the same amount of elements
as |icon_types_priority|.
The constructor should also not have a |desired_size_in_dip| given that it is
unclear how |threshold_for_next_icon_types| should be handled when
|desired_size_in_dip| != 0

Some overarching questions:
1) How common are 32x32 touch icons? If they are rare, we could change the logic
to prefer:
  i) Closest match to |desired_size_in_dip| and whichever scale factors are
passed in
  ii) Favicons over touch icons in case that the two have the same size.

2) If we expect to always be passing in the same values for
|icon_types_priority| and |threshold_for_new_icon_types| it may make sense to
make those values an implementation detail of HistoryBackend.

[michaelbai, 2013/10/09 18:35:09]

Yes, currently, it will always be the same values, but I am not sure it is good
to have this specific logic in HistoryBackend.

 
     Profile* profile;
     GURL page_url;
-    int icon_types;
+    std::vector<int> icon_types_priority;

[sky, 2013/10/08 23:47:52]

How come these take an int instead of IconType?
Also, name it icon_types as order is implied by it being a vector.

[michaelbai, 2013/10/09 18:35:09]

Using int because The icon_types could be

icon_types_prority.push_back(chrome::FAVICON);
icon_types_prority.push_back(chrome::TOUCH_ICON|chrome::PRECOMPOSED_TOUCH_ICON);


On 2013/10/08 23:47:52, sky wrote:

     int desired_size_in_dip;
+    int threshold_for_next_icon_types;
   };
 
   // Callback for GetFaviconImage() and GetFaviconImageForURL().
   // |FaviconImageResult::image| is constructed from the bitmaps for the
   // passed in URL and icon types which most which closely match the passed in
   // |desired_size_in_dip| at the scale factors supported by the current
   // platform (eg MacOS) in addition to 1x.
   // |FaviconImageResult::icon_url| is the favicon that the favicon bitmaps in
   // |image| originate from.
   // TODO(pkotwicz): Enable constructing |image| from bitmaps from several
@@ skipping 195 matching lines @@
   void RunFaviconRawCallbackWithBitmapResults(
       const FaviconRawCallback& callback,
       int desired_size_in_dip,
       ui::ScaleFactor desired_scale_factor,
       const std::vector<chrome::FaviconBitmapResult>& favicon_bitmap_results);
 
   DISALLOW_COPY_AND_ASSIGN(FaviconService);
 };
 
 #endif  // CHROME_BROWSER_FAVICON_FAVICON_SERVICE_H_