Always select best favicon bitmap

components/favicon/core/favicon_handler.cc

Index: components/favicon/core/favicon_handler.cc
diff --git a/components/favicon/core/favicon_handler.cc b/components/favicon/core/favicon_handler.cc
index 1ed8333b061954264f02c7fe29288cdc05042090..a48967f307f419b5d2ab73c5ce83d134e3e9da55 100644
--- a/components/favicon/core/favicon_handler.cc
+++ b/components/favicon/core/favicon_handler.cc
@@ -23,29 +23,23 @@
 namespace favicon {
 namespace {
 
+const int kNonTouchLargestIconSize = 192;
 // Size (along each axis) of a touch icon. This currently corresponds to
 // the apple touch icon for iPad.
 const int kTouchIconSize = 144;
 
-bool DoUrlAndIconMatch(const FaviconURL& favicon_url,
-                       const GURL& url,
-                       favicon_base::IconType icon_type) {
-  return favicon_url.icon_url == url && favicon_url.icon_type == icon_type;
-}
-
 // Returns true if all of the icon URLs and icon types in |bitmap_results| are
-// identical and if they match the icon URL and icon type in |favicon_url|.
-// Returns false if |bitmap_results| is empty.
+// identical and if they match |icon_url| and |icon_type|. Returns false if
+// |bitmap_results| is empty.
 bool DoUrlsAndIconsMatch(
-    const FaviconURL& favicon_url,
+    const GURL& icon_url,
+    favicon_base::IconType icon_type,
     const std::vector<favicon_base::FaviconRawBitmapResult>& bitmap_results) {
   if (bitmap_results.empty())
     return false;
 
-  const favicon_base::IconType icon_type = favicon_url.icon_type;
-
   for (const auto& bitmap_result : bitmap_results) {
-    if (favicon_url.icon_url != bitmap_result.icon_url ||
+    if (icon_url != bitmap_result.icon_url ||
         icon_type != bitmap_result.icon_type) {
       return false;
     }
@@ -113,85 +107,40 @@ bool HasValidResult(
       bitmap_results.end();
 }
 
-// Returns the index of the entry with the largest area.
-int GetLargestSizeIndex(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())
-      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);
+std::vector<int> GetDesiredPixelSizes(
+    FaviconDriverObserver::NotificationIconType handler_type) {
+  switch (handler_type) {
+    case FaviconDriverObserver::NON_TOUCH_16_DIP: {
+      std::vector<int> pixel_sizes;
+      for (float scale_factor : favicon_base::GetFaviconScales())
+        pixel_sizes.push_back(scale_factor * gfx::kFaviconSize);

[pkotwicz, 2017/03/27 00:49:06]

Nit: Round up the way that GetPixelSizesForFaviconScales() in
favicon_service_impl.cc does

[mastiz, 2017/03/27 10:09:36]

Done.

+      return pixel_sizes;
+    }
+    case FaviconDriverObserver::NON_TOUCH_LARGEST:
+      return std::vector<int>(1U, kNonTouchLargestIconSize);
+    case FaviconDriverObserver::TOUCH_LARGEST:
+      return std::vector<int>(1U, kTouchIconSize);
   }
-  std::stable_sort(image_urls->begin(), image_urls->end(), CompareIconSize);
-}
-
-// Checks whether two FaviconURLs are equal ignoring the icon sizes.
-bool FaviconURLsEqualIgnoringSizes(const FaviconURL& u1, const FaviconURL& u2) {
-  return u1.icon_type == u2.icon_type && u1.icon_url == u2.icon_url;
+  NOTREACHED();
 }
 
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
 
-FaviconHandler::FaviconCandidate::FaviconCandidate()
-    : score(0), icon_type(favicon_base::INVALID_ICON) {
-}
-
-FaviconHandler::FaviconCandidate::~FaviconCandidate() {
+// static
+FaviconHandler::FaviconCandidate
+FaviconHandler::FaviconCandidate::FromFaviconURL(
+    const favicon::FaviconURL& favicon_url,
+    const std::vector<int>& desired_pixel_sizes) {
+  FaviconCandidate candidate;
+  candidate.icon_url = favicon_url.icon_url;
+  candidate.icon_type = favicon_url.icon_type;
+  SelectFaviconFrameIndices(favicon_url.icon_sizes, desired_pixel_sizes,
+                            /*best_indices=*/nullptr, &candidate.score);

[pkotwicz, 2017/03/27 00:49:06]

https://www.reddit.com/ specifies two icons:

<link rel="icon" href="//www.redditstatic.com/icon.png" sizes="256x256"
type="image/png">
<link rel="shortcut icon" href="//www.redditstatic.com/favicon.ico"
type="image/x-icon">

Notably, there is no sizes information specified for the .ico file

The youtube mobile site also specifies "sizes" information for all of the
favicons except the .ico file

Perhaps if no sizes information was provided and the file is a .ico file, we can
guess that the .ico file has a 16x16 icon and compute the score based on that
guess.
I think that making this guess will result in downloading the .ico file first on
desktop on both reddit and youtube.

[mastiz, 2017/03/27 10:09:36]

I prefer implementing such logic in a dedicated patch, since it requires careful
thinking.

For example, on mobile, it could cause .ico files to be chosen more often than
before, which is likely a bad thing.

[pkotwicz, 2017/03/27 19:12:44]

If you "assign 16x16 to sizes for .ico files if sizes is unspecified in the
<link> tag", it will not cause .ico files to be chosen more often on mobile.

SelectFaviconFrameIndices() with desired_sizes = {192x192} will return a bad
score for frame_pixel_sizes = {16x16}

The only case that the .ico file will be chosen more on mobile is if none of the
icons specify sizes information. In this case, the .ico file with its auto
assigned size will be deemed a better candidate than the icons with no sizes
information

I am ok if you do this in a separate CL. Please file a bug in order to fix this

[mastiz, 2017/03/28 09:09:30]

Done. Filed a bug against you since our team is unlikely to work on improvements
for desktop.

+  return candidate;
 }
 
-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) {}
-
 ////////////////////////////////////////////////////////////////////////////////
 
 FaviconHandler::FaviconHandler(
@@ -238,11 +187,11 @@ void FaviconHandler::FetchFavicon(const GURL& url) {
   redownload_icons_ = false;
   got_favicon_from_history_ = false;
   download_request_.Cancel();
-  image_urls_.clear();
+  candidates_.clear();
   notification_icon_url_ = GURL();
   notification_icon_type_ = favicon_base::INVALID_ICON;
   current_candidate_index_ = 0u;
-  best_favicon_candidate_ = FaviconCandidate();
+  best_favicon_ = DownloadedFavicon();
 
   // Request the favicon from the history service. In parallel to this the
   // renderer is going to notify us (well WebContents) when the favicon url is
@@ -257,21 +206,12 @@ 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();
+bool FaviconHandler::UpdateFaviconCandidate(
+    const DownloadedFavicon& downloaded_favicon) {
+  if (downloaded_favicon.candidate.score > best_favicon_.candidate.score)
+    best_favicon_ = downloaded_favicon;
 
+  if (download_largest_icon_) {
     // The size of the downloaded icon may not match the declared size. Stop
     // downloading if:
     // - current candidate is only candidate.
@@ -280,28 +220,12 @@ bool FaviconHandler::UpdateFaviconCandidate(const GURL& image_url,
     //   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.

[pkotwicz, 2017/03/27 00:49:06]

Nit: Can you please update the comment?

I think this part - "all favicon without sizes attribute are downloaded" is
confusing. (even with the old code)

[mastiz, 2017/03/27 10:09:36]

Done.

[pkotwicz, 2017/03/27 19:12:44]

I don't see any changes to the comment

[mastiz, 2017/03/28 09:09:30]

Sorry, this is the second time I fail to export an updated patchset. Not sure
what's going on...

-    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);
-    }
+    return current_candidate_index_ + 1 >= candidates_.size() ||
+           candidates_[current_candidate_index_ + 1].score <=
+               best_favicon_.candidate.score;
   } 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;
-  }
-  if (replace_best_favicon_candidate) {
-    best_favicon_candidate_ =
-        FaviconCandidate(image_url, image, score, icon_type);
+    return best_favicon_.candidate.score == 1;
   }
-  return exact_match;
 }
 
 void FaviconHandler::SetFavicon(const GURL& icon_url,
@@ -354,27 +278,31 @@ void FaviconHandler::OnUpdateFaviconURL(
   if (page_url != url_)
     return;
 
-  std::vector<FaviconURL> pruned_candidates;
+  std::vector<FaviconCandidate> sorted_candidates;
+  const std::vector<int> desired_pixel_sizes =
+      GetDesiredPixelSizes(handler_type_);
   for (const FaviconURL& candidate : candidates) {
     if (!candidate.icon_url.is_empty() && (candidate.icon_type & icon_types_))

[pkotwicz, 2017/03/27 19:12:43]

Nit: Can you please add braces since the inner code (286 - 287) now takes up two
lines?

[mastiz, 2017/03/28 09:09:30]

Done.

-      pruned_candidates.push_back(candidate);
+      sorted_candidates.push_back(
+          FaviconCandidate::FromFaviconURL(candidate, desired_pixel_sizes));
   }
 
-  if (download_largest_icon_)
-    SortAndPruneImageUrls(&pruned_candidates);
+  std::stable_sort(sorted_candidates.begin(), sorted_candidates.end(),
+                   &FaviconCandidate::CompareScore);
 
-  // Ignore FaviconURL::icon_sizes because FaviconURL::icon_sizes is not stored
-  // in the history database.
-  if (image_urls_.size() == pruned_candidates.size() &&
-      std::equal(pruned_candidates.begin(), pruned_candidates.end(),
-                 image_urls_.begin(), FaviconURLsEqualIgnoringSizes)) {
+  // Note that icon sizes are ignored because FaviconCandidate doesn't store
+  // icon sizes. This is important because icon sizes are not stored in the
+  // history database.

[pkotwicz, 2017/03/27 19:12:43]

This comment is no longer valid since FaviconCandidate::Equals() checks that the
scores are equal.
The score and the order of |sorted_candidates| is based on the size information.
Delete the comment?

[mastiz, 2017/03/28 09:09:30]

Done, removed.

+  if (candidates_.size() == sorted_candidates.size() &&
+      std::equal(sorted_candidates.begin(), sorted_candidates.end(),
+                 candidates_.begin(), &FaviconCandidate::Equals)) {
     return;
   }
 
   download_request_.Cancel();
-  image_urls_ = pruned_candidates;
+  candidates_ = std::move(sorted_candidates);
   current_candidate_index_ = 0u;
-  best_favicon_candidate_ = FaviconCandidate();
+  best_favicon_ = DownloadedFavicon();
 
   // TODO(davemoore) Should clear on empty url. Currently we ignore it.
   // This appears to be what FF does as well.
@@ -382,28 +310,19 @@ 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;
+// static
+int FaviconHandler::GetMaximalIconSize(
+    FaviconDriverObserver::NotificationIconType handler_type) {
+  int max_size = 0;
+  for (int size : GetDesiredPixelSizes(handler_type))
+    max_size = std::max(max_size, size);
+  return max_size;
 }
 
 void FaviconHandler::OnGotInitialHistoryDataAndIconURLCandidates() {
   if (!initial_history_result_expired_or_incomplete_ &&
-      DoUrlAndIconMatch(*current_candidate(), notification_icon_url_,
-                        notification_icon_type_)) {
+      current_candidate()->icon_url == notification_icon_url_ &&
+      current_candidate()->icon_type == notification_icon_type_) {
     // - The data from history is valid and not expired.
     // - The icon URL of the history data matches one of the page's icon URLs.
     // - The icon URL of the history data matches the icon URL of the last
@@ -439,18 +358,13 @@ void FaviconHandler::OnDidDownloadFavicon(
     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));
+      std::vector<size_t> best_indices;
+      SelectFaviconFrameIndices(original_bitmap_sizes,
+                                GetDesiredPixelSizes(handler_type_),
+                                &best_indices, &score);
+      DCHECK_EQ(1U, best_indices.size());
+      image_skia =
+          gfx::ImageSkia::CreateFrom1xBitmap(bitmaps[best_indices.front()]);
     } else {
       image_skia = CreateFaviconImageSkia(bitmaps,
                                           original_bitmap_sizes,
@@ -459,32 +373,41 @@ void FaviconHandler::OnDidDownloadFavicon(
     }
 
     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, icon_type);
+      DownloadedFavicon downloaded_favicon;
+      downloaded_favicon.image = gfx::Image(image_skia);
+      downloaded_favicon.candidate.icon_url = image_url;
+      downloaded_favicon.candidate.icon_type = icon_type;
+      downloaded_favicon.candidate.score = score;
+      request_next_icon = !UpdateFaviconCandidate(downloaded_favicon);
     }
   }
 
-  if (request_next_icon && current_candidate_index_ + 1 < image_urls_.size()) {
+  if (request_next_icon && current_candidate_index_ + 1 < candidates_.size()) {
     // Process the next candidate.
     ++current_candidate_index_;
     DownloadCurrentCandidateOrAskFaviconService();
   } else {
     // We have either found the ideal candidate or run out of candidates.
-    if (best_favicon_candidate_.icon_type != favicon_base::INVALID_ICON) {
+    if (best_favicon_.candidate.icon_type != favicon_base::INVALID_ICON) {
       // No more icons to request, set the favicon from the candidate.
-      SetFavicon(best_favicon_candidate_.image_url,
-                 best_favicon_candidate_.image,
-                 best_favicon_candidate_.icon_type);
+      SetFavicon(best_favicon_.candidate.icon_url, best_favicon_.image,
+                 best_favicon_.candidate.icon_type);
     }
     // Clear download related state.
-    current_candidate_index_ = image_urls_.size();
-    best_favicon_candidate_ = FaviconCandidate();
+    current_candidate_index_ = candidates_.size();
+    best_favicon_ = DownloadedFavicon();
   }
 }
 
+const std::vector<GURL> FaviconHandler::GetIconURLs() const {
+  std::vector<GURL> icon_urls;
+  for (const FaviconCandidate& candidate : candidates_)
+    icon_urls.push_back(candidate.icon_url);
+  return icon_urls;
+}
+
 bool FaviconHandler::HasPendingTasksForTest() {
   return !download_request_.IsCancelled() ||
          cancelable_task_tracker_.HasTrackedTasks();
@@ -510,9 +433,10 @@ void FaviconHandler::OnFaviconDataForInitialURLFromFaviconService(
   redownload_icons_ = initial_history_result_expired_or_incomplete_ &&
                       !favicon_bitmap_results.empty();
 
-  if (has_valid_result &&
-      (!current_candidate() ||
-       DoUrlsAndIconsMatch(*current_candidate(), favicon_bitmap_results))) {
+  if (has_valid_result && (!current_candidate() ||
+                           DoUrlsAndIconsMatch(current_candidate()->icon_url,
+                                               current_candidate()->icon_type,
+                                               favicon_bitmap_results))) {
     // The db knows the favicon (although it may be out of date) and the entry
     // doesn't have an icon. Set the favicon now, and if the favicon turns out
     // to be expired (or the wrong url) we'll fetch later on. This way the
@@ -573,8 +497,9 @@ void FaviconHandler::OnFaviconData(const std::vector<
   }
 
   if (!current_candidate() ||
-      (has_results &&
-       !DoUrlsAndIconsMatch(*current_candidate(), favicon_bitmap_results))) {
+      (has_results && !DoUrlsAndIconsMatch(current_candidate()->icon_url,
+                                           current_candidate()->icon_type,
+                                           favicon_bitmap_results))) {
     // The icon URLs have been updated since the favicon data was requested.
     return;
   }
@@ -601,8 +526,9 @@ void FaviconHandler::ScheduleDownload(const GURL& image_url,
   // A max bitmap size is specified to avoid receiving huge bitmaps in
   // OnDidDownloadFavicon(). See FaviconDriver::StartDownload()
   // for more details about the max bitmap size.
-  const int download_id = delegate_->DownloadImage(
-      image_url, GetMaximalIconSize(icon_type), download_request_.callback());
+  const int download_id =
+      delegate_->DownloadImage(image_url, GetMaximalIconSize(handler_type_),
+                               download_request_.callback());
   DCHECK_NE(download_id, 0);
 }