Simplify 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 aff16261e47917a0680e7ec581d1644f29875f1f..ed01175de06fc6894e6f9421809ab86e366bfa98 100644
--- a/components/favicon/core/favicon_handler.cc
+++ b/components/favicon/core/favicon_handler.cc
@@ -235,7 +235,10 @@ void FaviconHandler::FetchFavicon(const GURL& url) {
   url_ = url;
 
   favicon_expired_or_incomplete_ = got_favicon_from_history_ = false;
+  download_requests_.clear();
   image_urls_.clear();
+  history_results_.clear();
+  best_favicon_candidate_ = FaviconCandidate();
 
   // Request the favicon from the history service. In parallel to this the
   // renderer is going to notify us (well WebContents) when the favicon url is
@@ -298,19 +301,12 @@ void FaviconHandler::SetFavicon(const GURL& url,
   if (ShouldSaveFavicon(url))
     SetHistoryFavicons(url, icon_url, icon_type, image);
 
-  if (!UrlMatches(url, url_) || PageChangedSinceFaviconWasRequested())

[pkotwicz, 2015/04/13 02:55:30]

I removed the UrlMatches() check because we clear |download_requests_| whenver
|url_| changes.

[sky, 2015/04/13 19:47:37]

It's not clear to me this is right. From the comment in
PageChangedSinceFaviconWasRequested: "If the URL has changed out from under us
(as will happen with redirects)..." If a redirect doesn't change url_ then we
need to continue calling PageChangedSinceFaviconWasRequested.

[pkotwicz, 2015/04/13 20:06:46]

I am still checking PageChangedSinceFaviconWasRequested(). I have just moved the
check to the caller: OnDidDownloadFavicon()

[sky, 2015/04/13 21:01:21]

Is there a particular reason to push the check like you did? I prefer it here
with the early out, but maybe I'm not understanding why you want to call through
to the driver.

[pkotwicz, 2015/04/13 21:19:18]

I made this change because I had a hard time understanding why
PageChangedSinceFaviconWasRequested() was being called where it was (e.g why is
PageChangedSinceFaviconWasRequested() not called at the beginning of
OnDidDownloadFavicon()?).

Calling PageChangedSinceFaviconWasRequested() at the start of
OnDidDownloadFavicon() is easier to understand and is equivalent in terms of
behavior.

I don't mind adding a PageChangedSinceFaviconWasRequested() check at the
beginning of SetFavicon() if it feels safer to have it there too

[sky, 2015/04/13 22:50:42]

The idea is that we still persist a favicon we downloaded, even if it isn't for
the current page. By that I mean if we start a download for url1, the user
navigates to url2, then we get the icon for url1 we still persist url1's icon if
necessary. I suspect this got broke a long time ago...

[pkotwicz, 2015/04/19 23:59:04]

Yes, this got broken in 2012 (https://chromiumcodereview.appspot.com/9810023 in
particular)
I think that the feature: "Persisting the favicon which was downloaded even if
it was not for the current page" is as old as Chrome itself. (It is already
present in https://codereview.chromium.org/6672065/)
However:
- The feature made more sense back when we did not consider pages with multiple
icons
- I am unsure whether the value of having this feature was ever assessed.

Currently:
- We incorrectly clear image_urls_ on line 446 in the old code even if
PageChangedSinceFaviconWasRequested() is false.
- We might end up saving data for a suboptimal icon URL if the page has several
icon URLs. We interpret the page URL changing as "save the best one we got so
far". This will produce interesting results if we are replacing an expired
favicon.

In terms of resolutions:
- I am ok with fixing the feature if it was intended.
- I am ok with fixing the feature and adding UMA stats to identify how often
this feature is used
- I am of course ok with removing the feature because it helps reduce code
complexity

-    return;
-
-  NotifyFaviconAvailable(
-      icon_url,
-      image,
-      icon_type == favicon_base::FAVICON && !download_largest_icon_);
+  NotifyFaviconAvailable(icon_url, image);
 }
 
 void FaviconHandler::NotifyFaviconAvailable(
     const std::vector<favicon_base::FaviconRawBitmapResult>&
-        favicon_bitmap_results,
-    bool is_active_favicon) {
+        favicon_bitmap_results) {
   gfx::Image resized_image = favicon_base::SelectFaviconFramesFromPNGs(
       favicon_bitmap_results,
       favicon_base::GetFaviconScales(),
@@ -319,23 +315,27 @@ void FaviconHandler::NotifyFaviconAvailable(
   // not matter which result we get the |icon_url| from.
   const GURL icon_url = favicon_bitmap_results.empty() ?
       GURL() : favicon_bitmap_results[0].icon_url;
-  NotifyFaviconAvailable(icon_url, resized_image, is_active_favicon);
+  NotifyFaviconAvailable(icon_url, resized_image);
 }
 
 void FaviconHandler::NotifyFaviconAvailable(const GURL& icon_url,
-                                            const gfx::Image& image,
-                                            bool is_active_favicon) {
+                                            const gfx::Image& image) {
   gfx::Image image_with_adjusted_colorspace = image;
   favicon_base::SetFaviconColorSpace(&image_with_adjusted_colorspace);
 
+  bool is_active_favicon =

[Roger McFarlane (Chromium), 2015/04/13 14:21:49]

How would this interact with download requests initiating from a different page
(like the NTP, for a pointed example)?

For example, the LargeIconSource would be referenced from the NTP to trigger
on-demand downloads of large icon bitmaps. Would this run afoul of the handler
trying to infer if the icon was for the current page?

[pkotwicz, 2015/04/13 15:59:21]

Downloads initiated from a different part of Chrome should have no impact.
MetroPinTabHelper::TogglePinnedToStartScreen() already does this for example.
Care will be needed when LargeIconSource writes data to the Favicons database so
that it does not clobber data inserted by FaviconHandler.

+      (handler_type_ == FAVICON && !download_largest_icon_);
+
   driver_->OnFaviconAvailable(
       image_with_adjusted_colorspace, icon_url, is_active_favicon);
 }
 
 void FaviconHandler::OnUpdateFaviconURL(
     const std::vector<FaviconURL>& candidates) {
+  download_requests_.clear();
   image_urls_.clear();
   best_favicon_candidate_ = FaviconCandidate();
+
   for (const FaviconURL& candidate : candidates) {
     if (!candidate.icon_url.is_empty() && (candidate.icon_type & icon_types_))
       image_urls_.push_back(candidate);
@@ -393,14 +393,19 @@ void FaviconHandler::OnDidDownloadFavicon(
   DownloadRequest download_request = i->second;
   download_requests_.erase(i);
 
-  if (current_candidate() &&
-      DoUrlAndIconMatch(*current_candidate(),
-                        image_url,
-                        download_request.icon_type)) {
-    bool request_next_icon = true;
-    float score = 0.0f;
-    gfx::ImageSkia image_skia;
-    if (download_largest_icon_ && !bitmaps.empty()) {
+  if (PageChangedSinceFaviconWasRequested() ||

[pkotwicz, 2015/04/13 02:55:30]

I think that the original code has the PageChangedSinceFaviconWasRequested()
check where it does because OnDidDownloadFavicon() is supposed to work even if
download_request.url is for an old page (Stuff gets saved to the database even
if a user browses through sites really fast)
However, the original code does not do this correctly, because it resets
|image_urls_| even if download_request.url does not match

(And having the check here makes things simpler)

+      !current_candidate() ||
+      !DoUrlAndIconMatch(*current_candidate(),
+                         image_url,
+                         download_request.icon_type)) {
+    return;
+  }
+
+  bool request_next_icon = true;
+  float score = 0.0f;
+  gfx::ImageSkia image_skia;
+  if (!bitmaps.empty()) {
+    if (download_largest_icon_) {
       int index = -1;
       // Use the largest bitmap if FaviconURL doesn't have sizes attribute.
       if (current_candidate()->icon_sizes.empty()) {
@@ -424,30 +429,27 @@ void FaviconHandler::OnDidDownloadFavicon(
       gfx::Image image(image_skia);
       // The downloaded icon is still valid when there is no FaviconURL update
       // during the downloading.
-      if (!bitmaps.empty()) {

[pkotwicz, 2015/04/13 02:55:30]

I removed this check because it is redundant

-        request_next_icon = !UpdateFaviconCandidate(
-            download_request.url, image_url, image, score,
-            download_request.icon_type);
-      }
-    }
-    if (request_next_icon && !PageChangedSinceFaviconWasRequested() &&
-        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);
-      // Reset candidate.
-      image_urls_.clear();
-      download_requests_.clear();
-      best_favicon_candidate_ = FaviconCandidate();
+      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);
+    // Reset candidate.
+    image_urls_.clear();
+    download_requests_.clear();
+    best_favicon_candidate_ = FaviconCandidate();
+  }
 }
 
 bool FaviconHandler::HasPendingTasksForTest() {
@@ -569,7 +571,7 @@ void FaviconHandler::OnFaviconDataForInitialURLFromFaviconService(
       // 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.
-      NotifyFaviconAvailable(favicon_bitmap_results, true);
+      NotifyFaviconAvailable(favicon_bitmap_results);
     } else {
       // If |favicon_bitmap_results| does not have any valid results, treat the
       // favicon as if it's expired.
@@ -599,7 +601,7 @@ void FaviconHandler::OnFaviconDataForInitialURLFromFaviconService(
   // renderer to download the icon.
 
   if (has_valid_result && (handler_type_ != FAVICON || download_largest_icon_))
-    NotifyFaviconAvailable(favicon_bitmap_results, false);
+    NotifyFaviconAvailable(favicon_bitmap_results);
 }
 
 void FaviconHandler::DownloadFaviconOrAskFaviconService(
@@ -647,7 +649,7 @@ void FaviconHandler::OnFaviconData(const std::vector<
       // There is a favicon, set it now. If expired we'll download the current
       // one again, but at least the user will get some icon instead of the
       // default and most likely the current one is fine anyway.
-      NotifyFaviconAvailable(favicon_bitmap_results, true);
+      NotifyFaviconAvailable(favicon_bitmap_results);
     }
     if (has_expired_or_incomplete_result) {
       // The favicon is out of date. Request the current one.
@@ -668,11 +670,11 @@ void FaviconHandler::OnFaviconData(const std::vector<
 
   if (has_valid_result &&
       (handler_type_ != FAVICON || download_largest_icon_)) {
-    NotifyFaviconAvailable(favicon_bitmap_results, false);
+    NotifyFaviconAvailable(favicon_bitmap_results);
   }
 }
 
-int FaviconHandler::ScheduleDownload(const GURL& url,
+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
@@ -686,8 +688,6 @@ int FaviconHandler::ScheduleDownload(const GURL& url,
     download_requests_[download_id] =
         DownloadRequest(url, image_url, icon_type);
   }
-
-  return download_id;
 }
 
 void FaviconHandler::SortAndPruneImageUrls() {