Move WasUnableToDownload logic to FaviconHandler

components/favicon/core/favicon_handler.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/favicon/core/favicon_handler.h"
 
 #include <algorithm>
 #include <cmath>
 #include <vector>
 
@@ skipping 457 matching lines @@
 
     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 (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.
     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.
@@ skipping 101 matching lines @@
     // 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
     // user doesn't see a flash of the default favicon.
     NotifyFaviconUpdated(favicon_bitmap_results);
   }
 
   if (current_candidate())
     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;
 
   if (redownload_icons_) {
     // We have the mapping, but the favicon is out of date. Download it now.
     ScheduleDownload(icon_url, icon_type);
   } else {
     // We don't know the favicon, but we may have previously downloaded the
     // favicon for another page that shares the same favicon. Ask for the
@@ skipping 42 matching lines @@
 
   if (has_expired_or_incomplete_result) {
     ScheduleDownload(current_candidate()->icon_url,
                      current_candidate()->icon_type);
   }
 }
 
 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.
   const int download_id =
       delegate_->DownloadImage(image_url, GetMaximalIconSize(icon_type),
                                base::Bind(&FaviconHandler::OnDidDownloadFavicon,
                                           weak_ptr_factory_.GetWeakPtr()));
 
   // Download ids should be unique.
   DCHECK(download_requests_.find(download_id) == download_requests_.end());
   download_requests_[download_id] = DownloadRequest(image_url, icon_type);
 
   if (download_id == 0) {

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

Looking at the implementation of DownloadImage() I think that it is no longer
possible for |download_id| == 0

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

Tests currently return download_id in case of failure. I will address this in
future CLs where FaviconHandler tests are rewritten.

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

Please add a TODO and file a bug :)

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

Added a TODO, not filing a bug because it's a minor task and already addressed
by one of my CLs under review.

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

Fair enough

     // If DownloadFavicon() did not start a download, it returns a download id
     // of 0. We still need to call OnDidDownloadFavicon() because the method is
     // responsible for initiating the data request for the next candidate.
     OnDidDownloadFavicon(download_id, 0, image_url, std::vector<SkBitmap>(),
                          std::vector<gfx::Size>());
   }
 }
 
 }  // namespace favicon