Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(363)

Unified Diff: components/favicon/core/favicon_handler.cc

Issue 2739173002: Always select best favicon bitmap (Closed)
Patch Set: Updated. Created 3 years, 9 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « components/favicon/core/favicon_handler.h ('k') | components/favicon/core/favicon_handler_unittest.cc » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: components/favicon/core/favicon_handler.cc
diff --git a/components/favicon/core/favicon_handler.cc b/components/favicon/core/favicon_handler.cc
index 8ca5df8a1b250aa1722a999d5166ab0075751d75..35c9fa63229dda18bf409a8b080c3724fe21021b 100644
--- a/components/favicon/core/favicon_handler.cc
+++ b/components/favicon/core/favicon_handler.cc
@@ -23,9 +23,13 @@
namespace favicon {
namespace {
-// Size (along each axis) of a touch icon. This currently corresponds to
-// the apple touch icon for iPad.
-const int kTouchIconSize = 144;
+// Size (along each axis) of a touch icon.
+#if defined(OS_IOS)
+// This currently corresponds to the apple touch icon for iPad.
+const int kLargestIconSizeInPixels = 144;
+#else
+const int kLargestIconSizeInPixels = 192;
+#endif
bool DoUrlAndIconMatch(const FaviconURL& favicon_url,
const GURL& url,
@@ -63,47 +67,11 @@ bool IsValid(const favicon_base::FaviconRawBitmapResult& bitmap_result) {
return bitmap_result.is_valid();
}
-// Returns true if |bitmap_results| is non-empty and:
-// - At least one of the bitmaps in |bitmap_results| is expired
-// OR
-// - |bitmap_results| is missing favicons for |desired_size_in_dip| and one of
-// the scale factors in favicon_base::GetFaviconScales().
-bool HasExpiredOrIncompleteResult(
- int desired_size_in_dip,
+// Returns true at least one of the bitmaps in |bitmap_results| is expired.
+bool HasExpiredResult(
const std::vector<favicon_base::FaviconRawBitmapResult>& bitmap_results) {
- if (bitmap_results.empty())
- return false;
-
- // Check if at least one of the bitmaps is expired.
- std::vector<favicon_base::FaviconRawBitmapResult>::const_iterator it =
- std::find_if(bitmap_results.begin(), bitmap_results.end(), IsExpired);
- if (it != bitmap_results.end())
- return true;
-
- // Any favicon size is good if the desired size is 0.
- if (desired_size_in_dip == 0)
- return false;
-
- // Check if the favicon for at least one of the scale factors is missing.
- // |bitmap_results| should always be complete for data inserted by
- // FaviconHandler as the FaviconHandler stores favicons resized to all
- // of favicon_base::GetFaviconScales() into the history backend.
- // Examples of when |bitmap_results| can be incomplete:
- // - Favicons inserted into the history backend by sync.
- // - Favicons for imported bookmarks.
- std::vector<gfx::Size> favicon_sizes;
- for (const auto& bitmap_result : bitmap_results)
- favicon_sizes.push_back(bitmap_result.pixel_size);
-
- std::vector<float> favicon_scales = favicon_base::GetFaviconScales();
- for (float favicon_scale : favicon_scales) {
- int edge_size_in_pixel = std::ceil(desired_size_in_dip * favicon_scale);
- auto it = std::find(favicon_sizes.begin(), favicon_sizes.end(),
- gfx::Size(edge_size_in_pixel, edge_size_in_pixel));
- if (it == favicon_sizes.end())
- return true;
- }
- return false;
+ return std::find_if(bitmap_results.begin(), bitmap_results.end(),
+ IsExpired) != bitmap_results.end();
}
// Returns true if at least one of |bitmap_results| is valid.
@@ -113,57 +81,43 @@ bool HasValidResult(
bitmap_results.end();
}
-// Returns the index of the entry with the largest area.
-int GetLargestSizeIndex(const std::vector<gfx::Size>& sizes) {
+// Returns the index of the entry with the best size match.
+int GetBestMatchSizeIndex(const FaviconHandler::TargetSpec& target_size,
+ const std::vector<gfx::Size>& sizes) {
DCHECK(!sizes.empty());
size_t ret = 0;
for (size_t i = 1; i < sizes.size(); ++i) {
- if (sizes[ret].GetArea() < sizes[i].GetArea())
+ if (target_size.IsBetterMatch(sizes[i], sizes[ret]))
ret = i;
}
return static_cast<int>(ret);
}
-// Return the index of a size which is same as the given |size|, -1 returned if
-// there is no such bitmap.
-int GetIndexBySize(const std::vector<gfx::Size>& sizes,
- const gfx::Size& size) {
- DCHECK(!sizes.empty());
- std::vector<gfx::Size>::const_iterator i =
- std::find(sizes.begin(), sizes.end(), size);
- if (i == sizes.end())
- return -1;
-
- return static_cast<int>(i - sizes.begin());
-}
-
// Compare function used for std::stable_sort to sort as descend.
-bool CompareIconSize(const FaviconURL& b1, const FaviconURL& b2) {
- int area1 = 0;
- if (!b1.icon_sizes.empty())
- area1 = b1.icon_sizes.front().GetArea();
-
- int area2 = 0;
- if (!b2.icon_sizes.empty())
- area2 = b2.icon_sizes.front().GetArea();
-
- return area1 > area2;
-}
-
-// Sorts the entries in |image_urls| by icon size in descending order.
-// Discards all but the largest size for each FaviconURL.
-void SortAndPruneImageUrls(std::vector<FaviconURL>* image_urls) {
- // Not using const-reference since the loop mutates FaviconURL::icon_sizes.
- for (FaviconURL& image_url : *image_urls) {
- if (image_url.icon_sizes.empty())
- continue;
-
- gfx::Size largest =
- image_url.icon_sizes[GetLargestSizeIndex(image_url.icon_sizes)];
- image_url.icon_sizes.clear();
- image_url.icon_sizes.push_back(largest);
+bool CompareScore(const std::pair<float, FaviconURL>& b1,
+ const std::pair<float, FaviconURL>& b2) {
+ return b1.first > b2.first;
+}
+
+// Sorts the entries in |image_urls|, best icon size matches first for
+// |target_size|.
+void SortImageUrls(const FaviconHandler::TargetSpec& target_size,
+ std::vector<FaviconURL>* image_urls) {
+ std::vector<std::pair<float, FaviconURL>> scored_candidates;
+ for (const FaviconURL& image_url : *image_urls) {
+ float max_score = 0;
+ for (const gfx::Size& size : image_url.icon_sizes) {
+ max_score = std::max(
+ max_score,
+ GetFaviconCandidateScore(size, target_size.GetMaxPixelSize()));
pkotwicz 2017/03/15 03:49:39 You should use SelectFaviconFrameIndices() to comp
mastiz 2017/03/15 16:34:33 I believe this is a moot point now, because of the
+ }
+ scored_candidates.emplace_back(max_score, std::move(image_url));
}
- std::stable_sort(image_urls->begin(), image_urls->end(), CompareIconSize);
+ std::stable_sort(scored_candidates.begin(), scored_candidates.end(),
+ CompareScore);
+ image_urls->clear();
+ for (auto& scored_candidate : scored_candidates)
+ image_urls->push_back(std::move(scored_candidate.second));
}
// Checks whether two FaviconURLs are equal ignoring the icon sizes.
@@ -171,10 +125,118 @@ bool FaviconURLsEqualIgnoringSizes(const FaviconURL& u1, const FaviconURL& u2) {
return u1.icon_type == u2.icon_type && u1.icon_url == u2.icon_url;
}
+FaviconHandler::TargetSpec GetTargetSpec(
+ FaviconDriverObserver::NotificationIconType handler_type) {
+ switch (handler_type) {
+ case FaviconDriverObserver::NON_TOUCH_16_DIP:
+ return FaviconHandler::TargetSpec::For16x16Dips();
+ case FaviconDriverObserver::NON_TOUCH_LARGEST:
+ case FaviconDriverObserver::TOUCH_LARGEST:
+ return FaviconHandler::TargetSpec::ForLargest();
+ }
+ NOTREACHED();
+}
+
} // namespace
////////////////////////////////////////////////////////////////////////////////
+// static
+FaviconHandler::TargetSpec FaviconHandler::TargetSpec::ForLargest() {
+ FaviconHandler::TargetSpec target_size;
+ target_size.pixel_sizes_.insert({kLargestIconSizeInPixels, 1.0f});
+ target_size.ensure_exact_size_ = false;
+ return target_size;
+}
+
+// static
+FaviconHandler::TargetSpec FaviconHandler::TargetSpec::For16x16Dips() {
+ FaviconHandler::TargetSpec target_size;
+ for (float scale : favicon_base::GetFaviconScales()) {
+ target_size.pixel_sizes_.insert(
+ {std::ceil(scale * gfx::kFaviconSize), scale});
+ }
+ target_size.ensure_exact_size_ = true;
+ return target_size;
+}
+
+FaviconHandler::TargetSpec::TargetSpec(const TargetSpec&) = default;
+
+FaviconHandler::TargetSpec::~TargetSpec() = default;
+
+std::vector<int> FaviconHandler::TargetSpec::GetPixelSizes() const {
+ std::vector<int> result;
+ for (const auto& pixel_size : pixel_sizes_)
+ result.push_back(pixel_size.first);
+ return result;
+}
+
+int FaviconHandler::TargetSpec::GetMaxPixelSize() const {
+ return pixel_sizes_.rbegin()->first;
+}
+
+bool FaviconHandler::TargetSpec::WantsBestBitmapOnly() const {
+ return !ensure_exact_size_;
+}
+
+bool FaviconHandler::TargetSpec::HasCompleteResult(
+ const std::vector<favicon_base::FaviconRawBitmapResult>& bitmap_results)
+ const {
+ if (WantsBestBitmapOnly())
+ return HasValidResult(bitmap_results);
+
+ // Check if the favicon for at least one of the scale factors is missing.
+ // |bitmap_results| should always be complete for data inserted by
+ // FaviconHandler as the FaviconHandler stores favicons resized to all
+ // of |favicon_scales| into the history backend.
+ // Examples of when |bitmap_results| can be incomplete:
+ // - Favicons inserted into the history backend by sync.
+ // - Favicons for imported bookmarks.
+ std::vector<gfx::Size> favicon_sizes;
+ for (const auto& bitmap_result : bitmap_results) {
+ if (IsValid(bitmap_result))
+ favicon_sizes.push_back(bitmap_result.pixel_size);
+ }
+
+ for (const auto& pixel_size : pixel_sizes_) {
+ const int edge_size_in_pixel = pixel_size.first;
+ auto it = std::find(favicon_sizes.begin(), favicon_sizes.end(),
+ gfx::Size(edge_size_in_pixel, edge_size_in_pixel));
+ if (it == favicon_sizes.end())
+ return false;
+ }
+ return true;
+}
+
+bool FaviconHandler::TargetSpec::IsBetterMatch(const gfx::Size& size1,
+ const gfx::Size& size2) const {
+ return GetFaviconCandidateScore(size1, GetMaxPixelSize()) >
+ GetFaviconCandidateScore(size2, GetMaxPixelSize());
+}
+
+gfx::ImageSkia FaviconHandler::TargetSpec::CreateImageSkia(
+ const std::vector<SkBitmap>& bitmaps,
+ const std::vector<gfx::Size>& original_bitmap_sizes) const {
+ if (WantsBestBitmapOnly()) {
+ return gfx::ImageSkia(gfx::ImageSkiaRep(
+ bitmaps[GetBestMatchSizeIndex(*this, original_bitmap_sizes)], 1));
+ } else {
+ return CreateFaviconImageSkia(bitmaps, original_bitmap_sizes,
+ gfx::kFaviconSize, /*score=*/nullptr);
+ }
+}
+
+gfx::Image FaviconHandler::TargetSpec::CreateImageFromCache(
+ const std::vector<favicon_base::FaviconRawBitmapResult>& bitmap_results)
+ const {
+ return favicon_base::SelectFaviconFramesForPixelSizesFromPNGs(bitmap_results,
+ pixel_sizes_);
+}
+
+FaviconHandler::TargetSpec::TargetSpec() = default;
+
+////////////////////////////////////////////////////////////////////////////////
+
FaviconHandler::DownloadRequest::DownloadRequest()
: icon_type(favicon_base::INVALID_ICON) {
}
@@ -191,8 +253,7 @@ FaviconHandler::DownloadRequest::DownloadRequest(
////////////////////////////////////////////////////////////////////////////////
FaviconHandler::FaviconCandidate::FaviconCandidate()
- : score(0), icon_type(favicon_base::INVALID_ICON) {
-}
+ : icon_type(favicon_base::INVALID_ICON) {}
FaviconHandler::FaviconCandidate::~FaviconCandidate() {
}
@@ -200,11 +261,9 @@ FaviconHandler::FaviconCandidate::~FaviconCandidate() {
FaviconHandler::FaviconCandidate::FaviconCandidate(
const GURL& image_url,
const gfx::Image& image,
- float score,
favicon_base::IconType icon_type)
: image_url(image_url),
image(image),
- score(score),
icon_type(icon_type) {}
////////////////////////////////////////////////////////////////////////////////
@@ -218,9 +277,7 @@ FaviconHandler::FaviconHandler(
initial_history_result_expired_or_incomplete_(false),
redownload_icons_(false),
icon_types_(FaviconHandler::GetIconTypesFromHandlerType(handler_type)),
- download_largest_icon_(
- handler_type == FaviconDriverObserver::NON_TOUCH_LARGEST ||
- handler_type == FaviconDriverObserver::TOUCH_LARGEST),
+ target_spec_(GetTargetSpec(handler_type)),
notification_icon_type_(favicon_base::INVALID_ICON),
service_(service),
delegate_(delegate),
@@ -265,7 +322,7 @@ void FaviconHandler::FetchFavicon(const GURL& url) {
// available.
if (service_) {
service_->GetFaviconForPageURL(
- url_, icon_types_, preferred_icon_size(),
+ url_, icon_types_, target_spec_.GetPixelSizes(),
base::Bind(
&FaviconHandler::OnFaviconDataForInitialURLFromFaviconService,
base::Unretained(this)),
@@ -275,49 +332,42 @@ void FaviconHandler::FetchFavicon(const GURL& url) {
bool FaviconHandler::UpdateFaviconCandidate(const GURL& image_url,
const gfx::Image& image,
- float score,
favicon_base::IconType icon_type) {
- bool replace_best_favicon_candidate = false;
- bool exact_match = false;
- if (download_largest_icon_) {
- replace_best_favicon_candidate =
- image.Size().GetArea() >
- best_favicon_candidate_.image.Size().GetArea();
-
- gfx::Size largest = best_favicon_candidate_.image.Size();
- if (replace_best_favicon_candidate)
- largest = image.Size();
-
- // The size of the downloaded icon may not match the declared size. Stop
- // downloading if:
- // - current candidate is only candidate.
- // - next candidate doesn't have sizes attributes, in this case, the rest
- // candidates don't have sizes attribute either, stop downloading now,
- // otherwise, all favicon without sizes attribute are downloaded.
- // - next candidate has sizes attribute and it is not larger than largest,
- // - current candidate is maximal one we want.
- const int maximal_size = GetMaximalIconSize(icon_type);
- if (current_candidate_index_ + 1 >= image_urls_.size()) {
- exact_match = true;
- } else {
- FaviconURL next_image_url = image_urls_[current_candidate_index_ + 1];
- exact_match = next_image_url.icon_sizes.empty() ||
- next_image_url.icon_sizes[0].GetArea() <= largest.GetArea() ||
- (image.Size().width() == maximal_size &&
- image.Size().height() == maximal_size);
- }
- } else {
- exact_match = score == 1 || preferred_icon_size() == 0;
- replace_best_favicon_candidate =
- exact_match ||
- best_favicon_candidate_.icon_type == favicon_base::INVALID_ICON ||
- score > best_favicon_candidate_.score;
+ // The size of the downloaded icon may not match the declared size so compare
+ // before overriding |best_favicon_candidate_|.
+ if (target_spec_.IsBetterMatch(image.Size(),
+ best_favicon_candidate_.image.Size())) {
+ best_favicon_candidate_ = FaviconCandidate(image_url, image, icon_type);
+ }
+
+ // Stop downloading if current candidate is only candidate.
+ if (current_candidate_index_ + 1 >= image_urls_.size()) {
+ return true;
}
- if (replace_best_favicon_candidate) {
- best_favicon_candidate_ =
- FaviconCandidate(image_url, image, score, icon_type);
+ const FaviconURL& next_image_url = image_urls_[current_candidate_index_ + 1];
+
+ // For exact sizes (i.e. not best only), we download all candidates until an
+ // exact match is found.
+ if (!target_spec_.WantsBestBitmapOnly()) {
+ const int desired_max_size = target_spec_.GetMaxPixelSize();
+ return best_favicon_candidate_.image.Size() ==
+ gfx::Size(desired_max_size, desired_max_size);
}
- return exact_match;
+
+ // The size of the downloaded icon may not match the declared size. Stop
+ // downloading if:
+ // - next candidate doesn't have sizes attributes, in this case, the rest
+ // candidates don't have sizes attribute either, stop downloading now,
+ // otherwise, all favicon without sizes attribute are downloaded.
+ // - next candidate has sizes attribute and it is not better than best,
+ if (next_image_url.icon_sizes.empty())
+ return true;
+
+ const gfx::Size& best_next_size =
+ next_image_url.icon_sizes[
+ GetBestMatchSizeIndex(target_spec_, next_image_url.icon_sizes)];
+ return !target_spec_.IsBetterMatch(best_next_size,
+ best_favicon_candidate_.image.Size());
}
void FaviconHandler::SetFavicon(const GURL& icon_url,
@@ -335,10 +385,9 @@ void FaviconHandler::NotifyFaviconUpdated(
if (favicon_bitmap_results.empty())
return;
- gfx::Image resized_image = favicon_base::SelectFaviconFramesFromPNGs(
- favicon_bitmap_results,
- favicon_base::GetFaviconScales(),
- preferred_icon_size());
+ gfx::Image resized_image =
+ target_spec_.CreateImageFromCache(favicon_bitmap_results);
+
// The history service sends back results for a single icon URL and icon
// type, so it does not matter which result we get |icon_url| and |icon_type|
// from.
@@ -376,8 +425,7 @@ void FaviconHandler::OnUpdateFaviconURL(
pruned_candidates.push_back(candidate);
}
- if (download_largest_icon_)
- SortAndPruneImageUrls(&pruned_candidates);
+ SortImageUrls(target_spec_, &pruned_candidates);
// Ignore FaviconURL::icon_sizes because FaviconURL::icon_sizes is not stored
// in the history database.
@@ -398,24 +446,6 @@ void FaviconHandler::OnUpdateFaviconURL(
OnGotInitialHistoryDataAndIconURLCandidates();
}
-int FaviconHandler::GetMaximalIconSize(favicon_base::IconType icon_type) {
- switch (icon_type) {
- case favicon_base::FAVICON:
-#if defined(OS_ANDROID)
- return 192;
-#else
- return gfx::ImageSkia::GetMaxSupportedScale() * gfx::kFaviconSize;
-#endif
- case favicon_base::TOUCH_ICON:
- case favicon_base::TOUCH_PRECOMPOSED_ICON:
- return kTouchIconSize;
- case favicon_base::INVALID_ICON:
- return 0;
- }
- NOTREACHED();
- return 0;
-}
-
void FaviconHandler::OnGotInitialHistoryDataAndIconURLCandidates() {
if (!initial_history_result_expired_or_incomplete_ &&
DoUrlAndIconMatch(*current_candidate(), notification_icon_url_,
@@ -465,34 +495,15 @@ void FaviconHandler::OnDidDownloadFavicon(
bool request_next_icon = true;
if (!bitmaps.empty()) {
- float score = 0.0f;
- gfx::ImageSkia image_skia;
- if (download_largest_icon_) {
- int index = -1;
- // Use the largest bitmap if FaviconURL doesn't have sizes attribute.
- if (current_candidate()->icon_sizes.empty()) {
- index = GetLargestSizeIndex(original_bitmap_sizes);
- } else {
- index = GetIndexBySize(original_bitmap_sizes,
- current_candidate()->icon_sizes[0]);
- // Find largest bitmap if there is no one exactly matched.
- if (index == -1)
- index = GetLargestSizeIndex(original_bitmap_sizes);
- }
- image_skia = gfx::ImageSkia(gfx::ImageSkiaRep(bitmaps[index], 1));
- } else {
- image_skia = CreateFaviconImageSkia(bitmaps,
- original_bitmap_sizes,
- preferred_icon_size(),
- &score);
- }
+ gfx::ImageSkia image_skia =
+ target_spec_.CreateImageSkia(bitmaps, original_bitmap_sizes);
if (!image_skia.isNull()) {
gfx::Image image(image_skia);
// The downloaded icon is still valid when there is no FaviconURL update
// during the downloading.
- request_next_icon = !UpdateFaviconCandidate(image_url, image, score,
- download_request.icon_type);
+ request_next_icon =
+ !UpdateFaviconCandidate(image_url, image, download_request.icon_type);
pkotwicz 2017/03/15 03:49:39 UpdateFaviconCandidate() should be fed |bitmaps| a
}
}
@@ -534,9 +545,8 @@ void FaviconHandler::OnFaviconDataForInitialURLFromFaviconService(
got_favicon_from_history_ = true;
bool has_valid_result = HasValidResult(favicon_bitmap_results);
initial_history_result_expired_or_incomplete_ =
- !has_valid_result ||
- HasExpiredOrIncompleteResult(preferred_icon_size(),
- favicon_bitmap_results);
+ !has_valid_result || HasExpiredResult(favicon_bitmap_results) ||
+ !target_spec_.HasCompleteResult(favicon_bitmap_results);
redownload_icons_ = initial_history_result_expired_or_incomplete_ &&
!favicon_bitmap_results.empty();
@@ -567,7 +577,8 @@ void FaviconHandler::DownloadCurrentCandidateOrAskFaviconService() {
// favicon given the favicon URL.
if (delegate_->IsOffTheRecord()) {
service_->GetFavicon(
- icon_url, icon_type, preferred_icon_size(),
+ icon_url, icon_type,
+ target_spec_.GetPixelSizes(),
base::Bind(&FaviconHandler::OnFaviconData, base::Unretained(this)),
&cancelable_task_tracker_);
} else {
@@ -579,7 +590,7 @@ void FaviconHandler::DownloadCurrentCandidateOrAskFaviconService() {
// TODO(pkotwicz): pass in all of |image_urls_| to
// UpdateFaviconMappingsAndFetch().
service_->UpdateFaviconMappingsAndFetch(
- url_, {icon_url}, icon_type, preferred_icon_size(),
+ url_, {icon_url}, icon_type, target_spec_.GetPixelSizes(),
base::Bind(&FaviconHandler::OnFaviconData, base::Unretained(this)),
&cancelable_task_tracker_);
}
@@ -591,8 +602,8 @@ void FaviconHandler::OnFaviconData(const std::vector<
bool has_results = !favicon_bitmap_results.empty();
bool has_valid_result = HasValidResult(favicon_bitmap_results);
bool has_expired_or_incomplete_result =
- !has_valid_result || HasExpiredOrIncompleteResult(preferred_icon_size(),
- favicon_bitmap_results);
+ !has_valid_result || HasExpiredResult(favicon_bitmap_results) ||
+ !target_spec_.HasCompleteResult(favicon_bitmap_results);
if (has_valid_result) {
// There is a valid favicon. Notify any observers. It is useful to notify
@@ -631,7 +642,7 @@ void FaviconHandler::ScheduleDownload(const GURL& image_url,
// OnDidDownloadFavicon(). See FaviconDriver::StartDownload()
// for more details about the max bitmap size.
const int download_id =
- delegate_->DownloadImage(image_url, GetMaximalIconSize(icon_type),
+ delegate_->DownloadImage(image_url, target_spec_.GetMaxPixelSize(),
base::Bind(&FaviconHandler::OnDidDownloadFavicon,
weak_ptr_factory_.GetWeakPtr()));
DCHECK_NE(download_id, 0);
« no previous file with comments | « components/favicon/core/favicon_handler.h ('k') | components/favicon/core/favicon_handler_unittest.cc » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698