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 8ca5df8a1b250aa1722a999d5166ab0075751d75..a7a320bdea37aa6f11e0d7b768d316563f315c2b 100644
--- a/components/favicon/core/favicon_handler.cc
+++ b/components/favicon/core/favicon_handler.cc
@@ -23,30 +23,24 @@
 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;
-
-bool DoUrlAndIconMatch(const FaviconURL& favicon_url,
+bool DoUrlAndIconMatch(const FaviconSelector::Candidate& candidate,
                        const GURL& url,
                        favicon_base::IconType icon_type) {
-  return favicon_url.icon_url == url && favicon_url.icon_type == icon_type;
+  return candidate.icon_url == url && candidate.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|.
+// identical and if they match the icon URL and icon type in |candidate|.
 // Returns false if |bitmap_results| is empty.
 bool DoUrlsAndIconsMatch(
-    const FaviconURL& favicon_url,
+    const FaviconSelector::Candidate& candidate,
     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 ||
-        icon_type != bitmap_result.icon_type) {
+    if (!DoUrlAndIconMatch(candidate, bitmap_result.icon_url,
+                           bitmap_result.icon_type)) {
       return false;
     }
   }
@@ -113,64 +107,23 @@ 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::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;
 }
 
+std::vector<FaviconSelector::TargetSizeSpec> GetTargetSizeSpecs(
+    FaviconDriverObserver::NotificationIconType handler_type) {
+  switch (handler_type) {
+    case FaviconDriverObserver::NON_TOUCH_16_DIP:
+      return FaviconSelector::TargetSizeSpec::For16x16Dips();
+    case FaviconDriverObserver::NON_TOUCH_LARGEST:
+    case FaviconDriverObserver::TOUCH_LARGEST:
+      return {FaviconSelector::TargetSizeSpec::ForLargest()};
+  }
+  NOTREACHED();
+}
+
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -185,27 +138,7 @@ FaviconHandler::DownloadRequest::~DownloadRequest() {
 FaviconHandler::DownloadRequest::DownloadRequest(
     const GURL& image_url,
     favicon_base::IconType icon_type)
-    : image_url(image_url), icon_type(icon_type) {
-}
-
-////////////////////////////////////////////////////////////////////////////////
-
-FaviconHandler::FaviconCandidate::FaviconCandidate()
-    : score(0), icon_type(favicon_base::INVALID_ICON) {
-}
-
-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) {}
+    : image_url(image_url), icon_type(icon_type) {}
 
 ////////////////////////////////////////////////////////////////////////////////
 
@@ -218,13 +151,10 @@ 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_size_spec_(GetTargetSizeSpecs(handler_type).front()),

[pkotwicz, 2017/03/20 03:25:16]

Won't this regress the current behavior?

[mastiz, 2017/03/20 08:07:27]

As I mentioned in my earlier message, there's the behavioral difference that one
could consider a regression, which is reflected by the test chage in
ChooseExactMatchDespiteUpsampling. More on this below, let me know if you had
some other regression in mind.

       notification_icon_type_(favicon_base::INVALID_ICON),
       service_(service),
       delegate_(delegate),
-      current_candidate_index_(0u),
       weak_ptr_factory_(this) {
   DCHECK(delegate_);
 }
@@ -257,8 +187,9 @@ void FaviconHandler::FetchFavicon(const GURL& url) {
   image_urls_.clear();
   notification_icon_url_ = GURL();
   notification_icon_type_ = favicon_base::INVALID_ICON;
-  current_candidate_index_ = 0u;
-  best_favicon_candidate_ = FaviconCandidate();
+  current_candidate_.reset();
+  selector_.reset();
+  best_favicon_image_ = gfx::Image();
 
   // 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
@@ -273,53 +204,6 @@ 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;
-  }
-  if (replace_best_favicon_candidate) {
-    best_favicon_candidate_ =
-        FaviconCandidate(image_url, image, score, icon_type);
-  }
-  return exact_match;
-}
-
 void FaviconHandler::SetFavicon(const GURL& icon_url,
                                 const gfx::Image& image,
                                 favicon_base::IconType icon_type) {
@@ -370,55 +254,47 @@ void FaviconHandler::OnUpdateFaviconURL(
   if (page_url != url_)
     return;
 
-  std::vector<FaviconURL> pruned_candidates;
+  std::vector<FaviconURL> filtered_candidates;
+  std::set<GURL> urls_to_filter;
+  urls_to_filter.insert(GURL());
+
   for (const FaviconURL& candidate : candidates) {
-    if (!candidate.icon_url.is_empty() && (candidate.icon_type & icon_types_))
-      pruned_candidates.push_back(candidate);
+    if (urls_to_filter.count(candidate.icon_url) == 0 &&
+        (candidate.icon_type & icon_types_)) {
+      filtered_candidates.push_back(candidate);
+      urls_to_filter.insert(candidate.icon_url);
+    }
   }
 
-  if (download_largest_icon_)
-    SortAndPruneImageUrls(&pruned_candidates);
-
   // 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(),
+  if (image_urls_.size() == filtered_candidates.size() &&
+      std::equal(filtered_candidates.begin(), filtered_candidates.end(),
                  image_urls_.begin(), FaviconURLsEqualIgnoringSizes)) {
     return;
   }
 
+  image_urls_ = filtered_candidates;
+  selector_.reset();
+  current_candidate_.reset();
+  best_favicon_image_ = gfx::Image();
   download_requests_.clear();
-  image_urls_ = pruned_candidates;
-  current_candidate_index_ = 0u;
-  best_favicon_candidate_ = FaviconCandidate();
+
+  if (image_urls_.empty())
+    return;
+
+  // Reset the state of all candidate queues.
+  selector_.reset(new FaviconSelector(target_size_spec_, image_urls_));
 
   // TODO(davemoore) Should clear on empty url. Currently we ignore it.
   // This appears to be what FF does as well.
-  if (current_candidate() && got_favicon_from_history_)
+  if (got_favicon_from_history_)
     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_,
+      DoUrlAndIconMatch(*selector_->CurrentCandidate(), notification_icon_url_,
                         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.
@@ -431,7 +307,7 @@ void FaviconHandler::OnGotInitialHistoryDataAndIconURLCandidates() {
     return;
   }
 
-  DownloadCurrentCandidateOrAskFaviconService();
+  DownloadNextCandidateOrAskFaviconService();
 }
 
 void FaviconHandler::OnDidDownloadFavicon(
@@ -440,6 +316,8 @@ void FaviconHandler::OnDidDownloadFavicon(
     const GURL& image_url,
     const std::vector<SkBitmap>& bitmaps,
     const std::vector<gfx::Size>& original_bitmap_sizes) {
+  DCHECK(current_candidate_);
+
   if (bitmaps.empty() && http_status_code == 404) {
     DVLOG(1) << "Failed to Download Favicon:" << image_url;
     if (service_)
@@ -456,63 +334,32 @@ void FaviconHandler::OnDidDownloadFavicon(
   DownloadRequest download_request = i->second;
   download_requests_.erase(i);
 
-  if (!current_candidate() ||
-      !DoUrlAndIconMatch(*current_candidate(),
-                         image_url,
-                         download_request.icon_type)) {
+  if (!current_candidate_ || !DoUrlAndIconMatch(*current_candidate_, image_url,
+                                                download_request.icon_type)) {
     return;
   }
 
-  bool request_next_icon = true;
-  if (!bitmaps.empty()) {
-    float score = 0.0f;
+  if (!bitmaps.empty() &&
+      selector_->ProcessDownloadedImage(current_candidate_->icon_url,
+                                        current_candidate_->icon_type, bitmaps,
+                                        original_bitmap_sizes)) {
+    // The bitmap has been chosen as the best so far so let's build the
+    // corresponding gfx::Image() and save it in |best_favicon_image_|.
     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));
+    if (target_size_spec_.WantsBestBitmapOnly()) {
+      image_skia =
+          gfx::ImageSkia(gfx::ImageSkiaRep(selector_->BestBitmap()->bitmap, 1));
     } else {
-      image_skia = CreateFaviconImageSkia(bitmaps,
-                                          original_bitmap_sizes,
+      image_skia = CreateFaviconImageSkia(bitmaps, original_bitmap_sizes,
                                           preferred_icon_size(),
-                                          &score);
+                                          /*score=*/nullptr);
     }
 
-    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);
-    }
+    if (!image_skia.isNull())
+      best_favicon_image_ = gfx::Image(image_skia);
   }
 
-  if (request_next_icon && current_candidate_index_ + 1 < image_urls_.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) {
-      // 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);
-    }
-    // Clear download related state.
-    download_requests_.clear();
-    current_candidate_index_ = image_urls_.size();
-    best_favicon_candidate_ = FaviconCandidate();
-  }
+  DownloadNextCandidateOrAskFaviconService();
 }
 
 bool FaviconHandler::HasPendingTasksForTest() {
@@ -541,8 +388,8 @@ void FaviconHandler::OnFaviconDataForInitialURLFromFaviconService(
                       !favicon_bitmap_results.empty();
 
   if (has_valid_result &&
-      (!current_candidate() ||
-       DoUrlsAndIconsMatch(*current_candidate(), favicon_bitmap_results))) {
+      (!current_candidate_ ||
+       DoUrlsAndIconsMatch(*current_candidate_, favicon_bitmap_results))) {

[pkotwicz, 2017/03/20 03:25:16]

This if() statement will now always return true because |current_candidate_|
will always be null when the statement is hit?

Can we use selector_->CurrentCandidate()

|FaviconSelector::CurrentCandidate()| and |FaviconHandler::current_candidate_|
is confusing

[mastiz, 2017/03/20 08:07:27]

You're right, thanks. I couldn't decide whether it's best to dequeue the element
before this point (then followed by
DownloadCurrentCandidateOrAskFaviconService()), or do it as proposed in this
version (i.e. all dequeuing in DownloadNextCandidateOrAskFaviconService(). I'm
leaning towards the second.

Wrt naming confusion, I renamed CurrentCandidate()->PeekCandidate(). Do you like
this better?

     // 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
@@ -550,13 +397,32 @@ void FaviconHandler::OnFaviconDataForInitialURLFromFaviconService(
     NotifyFaviconUpdated(favicon_bitmap_results);
   }
 
-  if (current_candidate())
+  if (selector_)
     OnGotInitialHistoryDataAndIconURLCandidates();
 }
 
-void FaviconHandler::DownloadCurrentCandidateOrAskFaviconService() {
-  GURL icon_url = current_candidate()->icon_url;
-  favicon_base::IconType icon_type = current_candidate()->icon_type;
+void FaviconHandler::DownloadNextCandidateOrAskFaviconService() {
+  current_candidate_ = selector_->DequeueCandidate();
+
+  if (!current_candidate_) {
+    // We have either found the ideal candidate or run out of candidates.
+    const base::Optional<FaviconSelector::BestCandidate>& best =
+        selector_->BestBitmap();
+    if (best) {
+      // No more icons to request, set the favicon from the candidate.
+      SetFavicon(best->candidate.icon_url, best_favicon_image_,
+                 best->candidate.icon_type);
+    }
+    // Clear download related state.
+    download_requests_.clear();
+    current_candidate_.reset();
+    best_favicon_image_ = gfx::Image();
+    selector_.reset();
+    return;
+  }
+
+  GURL icon_url = current_candidate_->icon_url;
+  favicon_base::IconType icon_type = current_candidate_->icon_type;
 
   if (redownload_icons_) {
     // We have the mapping, but the favicon is out of date. Download it now.
@@ -602,16 +468,16 @@ void FaviconHandler::OnFaviconData(const std::vector<
     NotifyFaviconUpdated(favicon_bitmap_results);
   }
 
-  if (!current_candidate() ||
+  if (!current_candidate_ ||
       (has_results &&
-       !DoUrlsAndIconsMatch(*current_candidate(), favicon_bitmap_results))) {
+       !DoUrlsAndIconsMatch(*current_candidate_, favicon_bitmap_results))) {
     // The icon URLs have been updated since the favicon data was requested.
     return;
   }
 
   if (has_expired_or_incomplete_result) {
-    ScheduleDownload(current_candidate()->icon_url,
-                     current_candidate()->icon_type);
+    ScheduleDownload(current_candidate_->icon_url,
+                     current_candidate_->icon_type);
   }
 }
 
@@ -630,8 +496,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 max_pixel_size = target_size_spec_.pixel_size();
   const int download_id =
-      delegate_->DownloadImage(image_url, GetMaximalIconSize(icon_type),
+      delegate_->DownloadImage(image_url, max_pixel_size,
                                base::Bind(&FaviconHandler::OnDidDownloadFavicon,
                                           weak_ptr_factory_.GetWeakPtr()));
   DCHECK_NE(download_id, 0);