Find Favicon in priority of icon_type.

chrome/browser/history/history_backend.cc

Index: chrome/browser/history/history_backend.cc
diff --git a/chrome/browser/history/history_backend.cc b/chrome/browser/history/history_backend.cc
index 2fa226dc351f2c6164ef8c762e32fcbc7ae33726..e7e6997a5ffdafdb57f54993dd879de15ccf8fce 100644
--- a/chrome/browser/history/history_backend.cc
+++ b/chrome/browser/history/history_backend.cc
@@ -1699,13 +1699,36 @@ void HistoryBackend::GetFavicons(
 
 void HistoryBackend::GetFaviconsForURL(

[pkotwicz, 2013/10/11 16:14:19]

I think it may be best to create a new method in HistoryBackend to deal with the
extra functionality you are adding in.
HistoryBackend::GetLargestFaviconForURL(
  GURL page_url,
  const std::vector<int> icon_types,
  const std::vector<int> thresholds_for_next_icon_type,
  chrome::FaviconBitmapresult* bitmap_result)

The behavior you currently have when |icon_types| has size 1 and when
|icon_types| has size > 1 is completely different. There is no reason in my mind
that the two should be handled by the same method.

Have you considered other ways of tackling this bug? For instance, not storing
touch icons in the database at all if we do not want to retrieve them. This is a
non trivial change, but you will need to change FaviconHandler anyways to store
large favicons of type FAVICON. This change would not necessitate changing
HistoryBackend at all because favicons expire within 7 days.

[michaelbai, 2013/10/11 16:34:32]

I had discussed adding new method with sky offline, he preferred to use one
method. They are not complete different, they actually share a lot of code. As
you can see, the previous feature can be implemented easily by
setting icon_types with one element, and we probably also still need the
capability to select the icon by desired_size_in_dip and desired_scale_factors.

I considered the solution to only download the icon that will be used before
write this CL, it moves how we select the icon to the download time, probably
will duplicate the logic in HistoryBackend, as you said this is not a trivial
change, and we probably will not support touch icon in the future, it might not
worthy to make such change right now. 


On 2013/10/11 16:14:19, pkotwicz wrote:

[sky, 2013/10/11 18:03:29]

In looking at this bit more I agree with Peter. This is trying to shoe horn too
much into a single function. Move new behavior into a separate function, maybe
GetLargestRawFaviconForURL. Create a new structure (or just pass in parameters
and no structure) for it as well. That is, don't overload FaviconForURLParams to
handle both.


On 2013/10/11 16:34:32, michaelbai wrote:

     const GURL& page_url,
-    int icon_types,
+    const std::vector<int>& icon_types,
     int desired_size_in_dip,
     const std::vector<ui::ScaleFactor>& desired_scale_factors,
+    int threshold_for_next_icon_types,
     std::vector<chrome::FaviconBitmapResult>* bitmap_results) {
   DCHECK(bitmap_results);
-  GetFaviconsFromDB(page_url, icon_types, desired_size_in_dip,
-                    desired_scale_factors, bitmap_results);
+  chrome::FaviconBitmapResult bitmap_result;
+  int bitmap_size_in_pixel = 0;
+  for (std::vector<int>::const_iterator i = icon_types.begin();
+      i != icon_types.end(); i++) {

[sky, 2013/10/11 18:03:29]

always use pre-increment for iterators.

+    GetFaviconsFromDB(page_url, *i, desired_size_in_dip, desired_scale_factors,
+                      bitmap_results);
+    if (icon_types.size() == 1)
+      return;
+
+    for (std::vector<chrome::FaviconBitmapResult>::const_iterator j =
+             bitmap_results->begin(); j != bitmap_results->end(); j++) {
+      if (bitmap_size_in_pixel < j->pixel_size.width() &&
+          bitmap_size_in_pixel < j->pixel_size.height()) {
+        bitmap_size_in_pixel = std::min(j->pixel_size.width(),
+                                        j->pixel_size.height());
+        bitmap_result = *j;
+      }
+    }
+    bitmap_results->clear();

[sky, 2013/10/11 18:03:29]

Don't reuse the passed in vector, that's just confusing and error prone.

+    if (bitmap_size_in_pixel > threshold_for_next_icon_types)

[sky, 2013/10/11 18:03:29]

Why is threshold_for_next_icon_types applied to all and not a vector<int>? Given
the usage a better name if desired_size.

+      break;
+  }
+  if (bitmap_size_in_pixel != 0)
+    bitmap_results->push_back(bitmap_result);
 }
 
 void HistoryBackend::GetFaviconForID(