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

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 1d5eac7ccdf34289398f1c2aa256434a160ccb5a..1c6e33e8856368536f327c0ffa6cfe8340c9633b 100644
--- a/components/favicon/core/favicon_handler.cc
+++ b/components/favicon/core/favicon_handler.cc
@@ -439,12 +439,13 @@ void FaviconHandler::OnDidDownloadFavicon(
     // 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();
@@ -682,11 +683,15 @@ void FaviconHandler::ScheduleDownload(const GURL& url,
   // 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>());
+
   }
 }