Move WasUnableToDownload logic to FaviconHandler

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 adbb5df8e2a4b4c769f4cca819fc95f424a78fda..57e196ab2a6b0a24b9dcf0769dfd54a5f0158a42 100644
--- a/components/favicon/core/favicon_handler.cc
+++ b/components/favicon/core/favicon_handler.cc
@@ -475,9 +475,8 @@ void FaviconHandler::OnDidDownloadFavicon(
     }
   }
 
-  if (request_next_icon && current_candidate_index_ + 1 < image_urls_.size()) {
+  if (request_next_icon && IncrementCurrentCandidateIndex()) {
     // Process the next candidate.
-    ++current_candidate_index_;
     DownloadCurrentCandidateOrAskFaviconService();
   } else {
     // We have either found the ideal candidate or run out of candidates.
@@ -599,6 +598,13 @@ void FaviconHandler::OnFaviconDataForInitialURLFromFaviconService(
     OnGotInitialHistoryDataAndIconURLCandidates();
 }
 
+bool FaviconHandler::IncrementCurrentCandidateIndex() {
+  if (current_candidate_index_ + 1 >= image_urls_.size())
+    return false;
+  ++current_candidate_index_;
+  return true;
+}
+
 void FaviconHandler::DownloadCurrentCandidateOrAskFaviconService() {
   GURL icon_url = current_candidate()->icon_url;
   favicon_base::IconType icon_type = current_candidate()->icon_type;
@@ -661,6 +667,14 @@ void FaviconHandler::OnFaviconData(const std::vector<
 void FaviconHandler::ScheduleDownload(const GURL& image_url,
                                       favicon_base::IconType icon_type) {
   DCHECK(image_url.is_valid());
+  if (service_ && service_->WasUnableToDownloadFavicon(image_url)) {

[pkotwicz, 2017/03/02 21:20:26]

Won't this cause a regression? In particular if a user starts out with nothing
in history and visits the following two pages in order

OnFaviconUpdated() won't be called for the second page

Page #1 Icons:
https://www.google.com/aerae -> 404

Page #2 Icons
10.png -> 10x10px PNG
11.png -> 11x11px PNG
https://www.google.com/aerae -> 404

[mastiz, 2017/03/03 14:21:14]

I think you're right. I tried to add tests that would reproduce this case
(https://codereview.chromium.org/2732653002/) but failed: 
1. If sizes are explicit: FaviconHandler will stop as soon as the 11x11 one is
fetched (because it sorts them first).
2. If sizes are not explicit:  FaviconHandler will stop as soon as the 10x10 is
fetched, because the following one doesn't have an explicit size.

So I'm not entirely sure the bug is reproducible, but anyway addressed your
concern.

[pkotwicz, 2017/03/03 18:31:16]

This bug is reproduceable on the desktop for a FaviconHandler constructed with
FaviconDriverObserver::NON_TOUCH_16_DIP.

It is probably possible to create a test case for which the bug reproduces for
FaviconDriverObserver::NON_TOUCH_LARGEST by having incorrects "sizes"
information in the <link> tags

[mastiz, 2017/03/03 20:37:01]

This seems so unlikely that I'd rather not add tests for it. If you do want
tests, I'll add them in either of the two ongoing CLs that improve test coverage
(preferably as part of the FaviconHandler tests).

[pkotwicz, 2017/03/03 20:56:14]

Fair enough

+    DVLOG(1) << "Skip Failed FavIcon: " << image_url;
+    // Process the next candidate.
+    if (IncrementCurrentCandidateIndex()) {
+      DownloadCurrentCandidateOrAskFaviconService();
+    }
+    return;
+  }
   // A max bitmap size is specified to avoid receiving huge bitmaps in
   // OnDidDownloadFavicon(). See FaviconDriver::StartDownload()
   // for more details about the max bitmap size.