Find Favicon in priority of icon_type.

chrome/browser/history/history_backend.cc

 // 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.
 
 #include "chrome/browser/history/history_backend.h"
 
 #include <algorithm>
 #include <functional>
 #include <list>
 #include <map>
@@ skipping 1678 matching lines @@
     const std::vector<GURL>& icon_urls,
     int icon_types,
     int desired_size_in_dip,
     const std::vector<ui::ScaleFactor>& desired_scale_factors,
     std::vector<chrome::FaviconBitmapResult>* bitmap_results) {
   UpdateFaviconMappingsAndFetchImpl(NULL, icon_urls, icon_types,
                                     desired_size_in_dip, desired_scale_factors,
                                     bitmap_results);
 }
 
+void HistoryBackend::GetLargestFaviconForURL(

[pkotwicz, 2013/10/15 21:39:41]

Nit: Order method in same order as in header file

[michaelbai, 2013/10/15 22:52:34]

Changed the header file order.

On 2013/10/15 21:39:41, pkotwicz wrote:

+      const GURL& page_url,
+      const std::vector<int>& icon_types,

[pkotwicz, 2013/10/15 21:39:41]

Given your current implementation, is there any reason that this cannot be:

const std::vector<chrome::IconType>& icon_types

[michaelbai, 2013/10/15 22:52:34]

No, the icon_types could be

FAVICON
TOUCH_ICON | PRECOMPOSED_TOUCH_ICON.


On 2013/10/15 21:39:41, pkotwicz wrote:

[pkotwicz, 2013/10/16 16:59:41]

I see your point.
In practice we only ever save one of TOUCH_ICON and PRECOMPOSED_TOUCH_ICON into
the database so there would be no difference either way (see 
HistoryBackend::SetFaviconMappingsForPage())
I suggested the change because it makes things simpler to think about.

[michaelbai, 2013/10/16 18:17:05]

Don't like this, the TOUCH_ICON and PRECOMPOSED_TOUCH_ICON is different type of
icon, I want to keep that flexibility for any future requirement.

On 2013/10/16 16:59:41, pkotwicz wrote:

+      int minimal_size_in_pixels,
+      std::vector<chrome::FaviconBitmapResult>* favicon_bitmap_results) {
+  DCHECK(favicon_bitmap_results);
+  favicon_bitmap_results->clear();
+
+  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/15 21:39:41]

Nit: the braces are not necessary

[michaelbai, 2013/10/15 22:52:34]

Done.

+    return;
+  }
+  if (icon_mappings.empty()) return;

[pkotwicz, 2013/10/15 21:39:41]

Nit: Return on a new line

[michaelbai, 2013/10/15 22:52:34]

Sky want to use this style in his previous comment

On 2013/10/15 21:39:41, pkotwicz wrote:

[sky, 2013/10/16 13:27:15]

Not sure what you mean here. I do not like single life if statement. Maybe you
mean my comment here
https://codereview.chromium.org/26563004/diff/33001/chrome/browser/history/hi...
. By that I meant make 1713:

if (!thumbnail_db_->GetIconMappingsForPageURL(page_url, &icon_mappings) ||
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 favicon in icon_types.

[pkotwicz, 2013/10/15 21:39:41]

How about: "Find the largest favicon bitmap for each icon type."

[michaelbai, 2013/10/15 22:52:34]

Done.

+  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;

[pkotwicz, 2013/10/15 21:39:41]

Nit: "continue" on a new line

[michaelbai, 2013/10/15 22:52:34]

ditto

On 2013/10/15 21:39:41, pkotwicz wrote:

+    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.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;

[pkotwicz, 2013/10/15 21:39:41]

Nit: return on a separate line

[michaelbai, 2013/10/15 22:52:34]

ditto

On 2013/10/15 21:39:41, pkotwicz wrote:

+
+  // Find an icon which is larger than minimal_size_in_pixels in the order of
+  // 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.pixel_size.width() < f->second.pixel_size.width() &&
+          largest_icon.pixel_size.height() < f->second.pixel_size.height())
+        largest_icon = f->second;
+    }
+    if (largest_icon.pixel_size.width() > minimal_size_in_pixels &&
+        largest_icon.pixel_size.height() > minimal_size_in_pixels)
+      break;
+  }
+  if (largest_icon.pixel_size.width() == 0 ||
+      largest_icon.pixel_size.height() == 0) {

[pkotwicz, 2013/10/15 21:39:41]

This is wrong. A pixel width and height in the database of 0 does not imply an
invalid bitmap or a bitmap with width and height of 0.
A pixel width and height in the database of 0 implies that the bitmap width and
height in unknown.

I would expect that if all of the results for GetFaviconBitmapIDSizes() had
empty sizes, that the first result would be chosen.

[michaelbai, 2013/10/15 22:52:34]

In which situation, it is unknown?


On 2013/10/15 21:39:41, pkotwicz wrote:

[pkotwicz, 2013/10/16 16:59:41]

FaviconSQLHandler::Insert() is an example.

[michaelbai, 2013/10/16 18:17:05]

Is this the only case?

On 2013/10/16 16:59:41, pkotwicz wrote:

[pkotwicz, 2013/10/16 18:28:24]

HistoryBackend::SetImportedFavicons() is another case

+    return;
+  }
+
+  GURL icon_url;
+  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_results->push_back(bitmap_result);
+
+  HISTOGRAM_TIMES("History.GetLargestFaviconForURL",
+                  TimeTicks::Now() - beginning_time);
+}
+
 void HistoryBackend::GetFaviconsForURL(
     const GURL& page_url,
     int icon_types,
     int desired_size_in_dip,
     const std::vector<ui::ScaleFactor>& desired_scale_factors,
     std::vector<chrome::FaviconBitmapResult>* bitmap_results) {
   DCHECK(bitmap_results);
   GetFaviconsFromDB(page_url, icon_types, desired_size_in_dip,
                     desired_scale_factors, bitmap_results);
 }
@@ skipping 1173 matching lines @@
   int rank = kPageVisitStatsMaxTopSites;
   std::map<GURL, int>::const_iterator it = most_visited_urls_map_.find(url);
   if (it != most_visited_urls_map_.end())
     rank = (*it).second;
   UMA_HISTOGRAM_ENUMERATION("History.TopSitesVisitsByRank",
                             rank, kPageVisitStatsMaxTopSites + 1);
 }
 #endif
 
 }  // namespace history