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 feb1664dabcf0792729a04d66c0bd65cccccdcb6..b3efd8d864caa2674351828b0ac45b88b684ba64 100644
--- a/chrome/browser/history/history_backend.cc
+++ b/chrome/browser/history/history_backend.cc
@@ -1696,6 +1696,100 @@ void HistoryBackend::GetFavicons(
                                     bitmap_results);
 }
 
+void HistoryBackend::GetLargestFaviconForURL(
+      const GURL& page_url,
+      const std::vector<int>& icon_types,
+      int minimum_size_in_pixels,

[pkotwicz, 2013/10/18 01:29:35]

Nit: minimum_size_in_pixel (without the s)

[michaelbai, 2013/10/18 04:37:02]

This required by sky 

https://codereview.chromium.org/26563004/diff/33001/chrome/browser/favicon/fa...

On 2013/10/18 01:29:35, pkotwicz wrote:

+      chrome::FaviconBitmapResult* favicon_bitmap_result) {
+  DCHECK(favicon_bitmap_result);
+
+  if (!db_ || !thumbnail_db_)
+    return;
+
+  TimeTicks beginning_time = TimeTicks::Now();
+
+  std::vector<IconMapping> icon_mappings;
+  if (!thumbnail_db_->GetIconMappingsForPageURL(page_url, &icon_mappings) ||

[pkotwicz, 2013/10/18 01:29:35]

Nit: separate this into 2 if statements.

[michaelbai, 2013/10/18 04:37:02]

This required by sky

https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/hi...

On 2013/10/18 01:29:35, pkotwicz wrote:

+      icon_mappings.empty())
+    return;
+
+  int required_icon_types = 0;
+  for (std::vector<int>::const_iterator i = icon_types.begin();
+       i != icon_types.end(); ++i) {
+    required_icon_types |= *i;
+  }
+
+  // Find the largest bitmap for each IconType placing in
+  // |largest_favicon_bitmaps|.
+  std::map<chrome::IconType, FaviconBitmap> largest_favicon_bitmaps;
+  for (std::vector<IconMapping>::const_iterator i = icon_mappings.begin();
+       i != icon_mappings.end(); ++i) {
+    if (!(i->icon_type & required_icon_types))
+      continue;
+    std::vector<FaviconBitmapIDSize> bitmap_id_sizes;
+    thumbnail_db_->GetFaviconBitmapIDSizes(i->icon_id, &bitmap_id_sizes);
+    FaviconBitmap& largest = largest_favicon_bitmaps[i->icon_type];
+    for (std::vector<FaviconBitmapIDSize>::const_iterator j =
+             bitmap_id_sizes.begin(); j != bitmap_id_sizes.end(); ++j) {
+      if (largest.bitmap_id == 0 ||
+          (largest.pixel_size.width() < j->pixel_size.width() &&
+           largest.pixel_size.height() < j->pixel_size.height())) {
+        largest.icon_id = i->icon_id;
+        largest.bitmap_id = j->bitmap_id;
+        largest.pixel_size = j->pixel_size;
+      }
+    }
+  }
+  if (largest_favicon_bitmaps.empty())
+    return;
+
+  // Find an icon which is larger than minimum_size_in_pixels in the order of

[pkotwicz, 2013/10/18 01:29:35]

Nit: |minimum_size_in_pixel|

[michaelbai, 2013/10/18 04:37:02]

ditto

On 2013/10/18 01:29:35, pkotwicz wrote:

+  // icon_types.
+  FaviconBitmap largest_icon;
+  for (std::vector<int>::const_iterator t = icon_types.begin();
+       t != icon_types.end(); ++t) {
+    for (std::map<chrome::IconType, FaviconBitmap>::const_iterator f =
+            largest_favicon_bitmaps.begin(); f != largest_favicon_bitmaps.end();
+        ++f) {
+      if (f->first & *t &&
+          (largest_icon.bitmap_id == 0 ||
+           (largest_icon.pixel_size.height() < f->second.pixel_size.height() &&
+            largest_icon.pixel_size.width() < f->second.pixel_size.width()))) {
+        largest_icon = f->second;
+      }
+    }
+    if (largest_icon.pixel_size.width() > minimum_size_in_pixels &&
+        largest_icon.pixel_size.height() > minimum_size_in_pixels)
+      break;
+  }
+
+  GURL icon_url;

[sky, 2013/10/18 01:21:59]

Isn't it possible to get here and largest_icon not be > minimum_size_in_pixels?
If that happens, do you really want to continue?

[michaelbai, 2013/10/18 04:37:02]

In this case, no icon is larger than minimum_size_in_pixels, return the largest
one.


On 2013/10/18 01:21:59, sky wrote:

[sky, 2013/10/18 14:11:40]

Then please update the docs.

On 2013/10/18 04:37:02, michaelbai wrote:

[michaelbai, 2013/10/18 17:31:47]

The doc covers this case, did I miss something?

On 2013/10/18 14:11:40, sky wrote:

+  chrome::IconType icon_type;
+  if (!thumbnail_db_->GetFaviconHeader(largest_icon.icon_id, &icon_url,
+                                       &icon_type)) {
+    return;
+  }
+
+  base::Time last_updated;
+  chrome::FaviconBitmapResult bitmap_result;
+  bitmap_result.icon_url = icon_url;
+  bitmap_result.icon_type = icon_type;
+  if (!thumbnail_db_->GetFaviconBitmap(largest_icon.bitmap_id,
+                                       &last_updated,
+                                       &bitmap_result.bitmap_data,
+                                       &bitmap_result.pixel_size)) {
+    return;
+  }
+
+  bitmap_result.expired = (Time::Now() - last_updated) >
+      TimeDelta::FromDays(kFaviconRefetchDays);
+  if (bitmap_result.is_valid())
+    *favicon_bitmap_result = bitmap_result;
+
+  HISTOGRAM_TIMES("History.GetLargestFaviconForURL",
+                  TimeTicks::Now() - beginning_time);
+}
+
 void HistoryBackend::GetFaviconsForURL(
     const GURL& page_url,
     int icon_types,