Fix FaviconHandler when it has multiple icon URLs and one of them is invalid

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 421 matching lines @@
       request_next_icon = !UpdateFaviconCandidate(
           download_request.url, image_url, image, score,
           download_request.icon_type);
     }
   }
 
   if (request_next_icon && image_urls_.size() > 1) {
     // Remove the first member of image_urls_ and process the remaining.
     image_urls_.erase(image_urls_.begin());
     ProcessCurrentUrl();
-  } else 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_.url,
-               best_favicon_candidate_.image_url,
-               best_favicon_candidate_.image,
-               best_favicon_candidate_.icon_type);
+  } else {

[Roger McFarlane (Chromium), 2015/04/27 14:25:48]

nit: Maybe return after ProcessCurrentUrl() ...


More importantly, add a comment block indicating...


"We get here when we are no longer going to continue downloading icons, either
because we've found a suitable one, or run out of candidates. (1) If we have a
valid candidate downloaded, save it to the
favicon database. (2) Clear the download bookkeeping structures."

Or some such...

+    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_.url, best_favicon_candidate_.image_url,
+                 best_favicon_candidate_.image,
+                 best_favicon_candidate_.icon_type);
+    }
     // Reset candidate.
     image_urls_.clear();
     download_requests_.clear();
     best_favicon_candidate_ = FaviconCandidate();
   }
 }
 
 bool FaviconHandler::HasPendingTasksForTest() {
   return !download_requests_.empty() ||
          cancelable_task_tracker_.HasTrackedTasks();
@@ skipping 217 matching lines @@
 }
 
 void FaviconHandler::ScheduleDownload(const GURL& url,
                                      const GURL& image_url,
                                      favicon_base::IconType icon_type) {
   // 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 = DownloadFavicon(image_url,
                                           GetMaximalIconSize(icon_type));
-  if (download_id) {
-    // Download ids should be unique.
-    DCHECK(download_requests_.find(download_id) == download_requests_.end());
-    download_requests_[download_id] =
-        DownloadRequest(url, image_url, icon_type);
+
+  // Download ids should be unique.
+  DCHECK(download_requests_.find(download_id) == download_requests_.end());
+  download_requests_[download_id] = DownloadRequest(url, image_url, icon_type);

[Roger McFarlane (Chromium), 2015/04/27 14:25:48]

This reads like you could have collisions on multiple failures. Correct me if
I'm wrong, but it seems like multiple failed requests (download_id == 0) are
immediately handled to completion below? Maybe a comment to that effect?

+
+  if (download_id == 0) {

[Roger McFarlane (Chromium), 2015/04/27 14:25:48]

As per above, comment that you trigger the handler with an empty image on
failure to start the download.

[pkotwicz, 2015/04/27 18:50:54]

You are right, this is non-obvious. I have added a comment

+    OnDidDownloadFavicon(download_id, image_url, std::vector<SkBitmap>(),
+                         std::vector<gfx::Size>());
+
   }
 }
 
 void FaviconHandler::SortAndPruneImageUrls() {
   // 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);
 }
 
 }  // namespace favicon