Find Favicon in priority of icon_type.

chrome/browser/favicon/favicon_service.h

Index: chrome/browser/favicon/favicon_service.h
diff --git a/chrome/browser/favicon/favicon_service.h b/chrome/browser/favicon/favicon_service.h
index 692a6ebe74e1d570670da09befcfc51ec8658a50..7f32ec91d11f6b993585f9a5bd84900130c9dd5f 100644
--- a/chrome/browser/favicon/favicon_service.h
+++ b/chrome/browser/favicon/favicon_service.h
@@ -46,13 +46,29 @@ class FaviconService : public CancelableRequestProvider,
                         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().