Avoid unnecessary favicon downloads on desktop

Prior to this patch, desktop favicon downloading would only stop when
a truly ideal icon is found, which means a favicon with perfect bitmaps
sizes for all supported screen densities (often a .ico file with a
16x16 AND a 32x32 bitmap). This often leaded to all icons being
downloaded, even for cases where it's obviously unnecessary.

The patch adopts the logic we currently use on mobile within
FaviconHandler::UpdateFaviconCandidate() in order to stop processing
favicon candidates if there is little hope that the next ones will be
any better.

This requires some smartness for favicon candidates without an explicit
'sizes' attribute, which would score very low prior to this patch and
hence would rarely be processed (if other candidates exist). Hence,
we special-case them for desktop and score them highest during initial
sorting, which mimics the behavior prior to this patch and guarantees
they will be processed unless an ideal favicon is found before.

It's rather trivial to rule out regressions for the following common
scenarios:
A. Mobile platforms: no behavioral changes as score stays zero for
   candidates without explicit 'sizes' attribute.
B. A single favicon is listed by the page.
C. Multiple favicons are listed and none have a 'sizes' attribute
   (sorting does nothing and all are processed unless an ideal one is
   found).
D. Multiple favicons are listed and they all have an accurate 'sizes'
   attribute (scenario improved in this patch).

The more complex scenario is when a web page lists multiple favicons
and only some have a 'sizes' attribute. In this case, there is some
minor risk for regressions given that the patch scores highest the
favicons without a 'sizes' attribute, so they will be downloaded and
processed earlier.

Bug: 895175,705900,672076
Change-Id: I8216652ede25f55332cacc54d3e0a806fd8b7bc3
Reviewed-on: https://chromium-review.googlesource.com/c/1323551
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610800}

components/favicon/core/favicon_handler.cc

@@ -121,16 +121,30 @@ bool FaviconURLEquals(const FaviconURL& lhs, const FaviconURL& rhs) {
 FaviconHandler::FaviconCandidate
 FaviconHandler::FaviconCandidate::FromFaviconURL(
     const favicon::FaviconURL& favicon_url,
-    const std::vector<int>& desired_pixel_sizes) {
+    const std::vector<int>& desired_pixel_sizes,
+    bool want_largest_icon) {
   FaviconCandidate candidate;
   candidate.icon_url = favicon_url.icon_url;
   candidate.icon_type = favicon_url.icon_type;
-  // TODO(crbug.com/705900): For candidates without explicit size information,
-  // sizes could be inferred for the most common cases. Namely, .ico files tend
-  // to contain the 16x16 bitmap, which would allow to improve the
-  // prioritization on desktop.
+
+  if (!favicon_url.icon_sizes.empty()) {
+    // For candidates with explicit size information, the score is computed
+    // based on similarity with |desired_pixel_sizes|.
     SelectFaviconFrameIndices(favicon_url.icon_sizes, desired_pixel_sizes,
                               /*best_indices=*/nullptr, &candidate.score);
+  } else if (want_largest_icon) {
+    // If looking for largest icon (mobile), candidates without explicit size
+    // information are scored low because they are likely small.
+    candidate.score = 0.0f;
+  } else {
+    // If looking for small icons (desktop), candidates without explicit size
+    // information are scored highest, as high as candidates with an ideal
+    // explicit size information. This guarantees all candidates without
+    // explicit size information will be processed until an ideal candidate is
+    // found (if available).
+    candidate.score = 1.0f;
+  }
+
   return candidate;
 }
 
@@ -222,24 +236,19 @@ bool FaviconHandler::UpdateFaviconCandidate(
   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. It's
-    // important to stop downloading if:
-    // - current candidate is only candidate.
-    // - next candidate has sizes attribute and it is not better than the best
-    //   one observed so far, which means any following candidate should also
-    //   be worse or equal too.
-    // - next candidate doesn't have sizes attributes, which means further
-    //   candidates don't have sizes attribute either (because the score lowest
-    //   and hence get sorted last during prioritization). We stop immediately
-    //   to avoid downloading them all, although we don't have the certainty
-    //   that no better favicon is among them.
-    return current_candidate_index_ + 1 >= final_candidates_->size() ||
-           (*final_candidates_)[current_candidate_index_ + 1].score <=
-               best_favicon_.candidate.score;
-  } else {
-    return best_favicon_.candidate.score == 1;
-  }
+  // Stop downloading if the current candidate is the last candidate.
+  if (current_candidate_index_ + 1 >= final_candidates_->size())
+    return true;
+
+  // |next_candidate_score| is based on the sizes provided in the <link> tag,
+  // see FaviconCandidate::FromFaviconURL().
+  float next_candidate_score =
+      (*final_candidates_)[current_candidate_index_ + 1].score;
+
+  // Stop downloading if the next candidate is not better than the best one
+  // observed so far, which means any following candidate should also be worse
+  // or equal too.
+  return next_candidate_score <= best_favicon_.candidate.score;
 }
 
 void FaviconHandler::SetFavicon(const GURL& icon_url,
@@ -420,8 +429,8 @@ void FaviconHandler::OnGotFinalIconURLCandidates(
   for (const FaviconURL& candidate : candidates) {
     if (!candidate.icon_url.is_empty() &&
         (icon_types_.count(candidate.icon_type) != 0)) {
-      sorted_candidates.push_back(
-          FaviconCandidate::FromFaviconURL(candidate, desired_pixel_sizes));
+      sorted_candidates.push_back(FaviconCandidate::FromFaviconURL(
+          candidate, desired_pixel_sizes, download_largest_icon_));
     }
   }
 

components/favicon/core/favicon_handler.h

@@ -168,7 +168,8 @@ class FaviconHandler {
     // Builds a scored candidate by selecting the best bitmap sizes.
     static FaviconCandidate FromFaviconURL(
         const favicon::FaviconURL& favicon_url,
-        const std::vector<int>& desired_pixel_sizes);
+        const std::vector<int>& desired_pixel_sizes,
+        bool want_largest_icon);
 
     // Compare function used for std::stable_sort to sort in descending order.
     static bool CompareScore(const FaviconCandidate& lhs,

components/favicon/core/favicon_handler_unittest.cc

@@ -36,6 +36,7 @@ namespace favicon {
 namespace {
 
 using favicon_base::FaviconRawBitmapResult;
+using testing::_;
 using testing::AnyNumber;
 using testing::Assign;
 using testing::Contains;
@@ -45,7 +46,7 @@ using testing::Invoke;
 using testing::IsEmpty;
 using testing::Not;
 using testing::Return;
-using testing::_;
+using testing::SizeIs;
 
 using IntVector = std::vector<int>;
 using URLVector = std::vector<GURL>;
@@ -1405,6 +1406,43 @@ TEST_F(FaviconHandlerMultipleFaviconsTest, ChooseMinorUpsamplingOverHugeIcon) {
   EXPECT_EQ(17, DownloadTillDoneIgnoringHistory(IntVector{17, 256}));
 }
 
+// Test a page with multiple favicon candidates with explicit sizes information.
+// Only the best one should be downloaded.
+TEST_F(FaviconHandlerMultipleFaviconsTest,
+       StopsDownloadingWhenRemainingCandidatesWorse) {
+  RunHandlerWithCandidates(FaviconDriverObserver::NON_TOUCH_16_DIP,
+                           {
+                               FaviconURL(kIconURL16x16, kFavicon,
+                                          SizeVector(1U, gfx::Size(16, 16))),
+                               FaviconURL(kIconURL64x64, kFavicon,
+                                          SizeVector(1U, gfx::Size(64, 64))),
+                           });
+
+  EXPECT_THAT(delegate_.downloads(), SizeIs(1));
+}
+
+TEST_F(FaviconHandlerMultipleFaviconsTest,
+       DownloadsAllIconsWithoutSizesAttributeIfNotWantsLargest) {
+  RunHandlerWithCandidates(FaviconDriverObserver::NON_TOUCH_16_DIP,
+                           {
+                               FaviconURL(kIconURL16x16, kFavicon, kEmptySizes),
+                               FaviconURL(kIconURL64x64, kFavicon, kEmptySizes),
+                           });
+
+  EXPECT_THAT(delegate_.downloads(), SizeIs(2));
+}
+
+TEST_F(FaviconHandlerMultipleFaviconsTest,
+       DownloadsOnlyOneIconWithoutSizesAttributeIfWantsLargest) {
+  RunHandlerWithCandidates(FaviconDriverObserver::NON_TOUCH_LARGEST,
+                           {
+                               FaviconURL(kIconURL16x16, kFavicon, kEmptySizes),
+                               FaviconURL(kIconURL64x64, kFavicon, kEmptySizes),
+                           });
+
+  EXPECT_THAT(delegate_.downloads(), ElementsAre(kIconURL16x16));
+}
+
 TEST_F(FaviconHandlerTest, Report404) {
   const GURL k404IconURL("http://www.google.com/404.png");