Simplify 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 217 matching lines @@
   }
   return 0;
 }
 
 void FaviconHandler::FetchFavicon(const GURL& url) {
   cancelable_task_tracker_.TryCancelAll();
 
   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
   // available.
   GetFaviconForURLFromFaviconService(
       url_, icon_types_,
       base::Bind(&FaviconHandler::OnFaviconDataForInitialURLFromFaviconService,
                  base::Unretained(this)),
       &cancelable_task_tracker_);
 }
@@ skipping 42 matching lines @@
   return exact_match;
 }
 
 void FaviconHandler::SetFavicon(const GURL& url,
                                 const GURL& icon_url,
                                 const gfx::Image& image,
                                 favicon_base::IconType icon_type) {
   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(),
       preferred_icon_size());
   // The history service sends back results for a single icon URL, so it does
   // 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);
   }
 
   if (download_largest_icon_)
     SortAndPruneImageUrls();
 
   // TODO(davemoore) Should clear on empty url. Currently we ignore it.
   // This appears to be what FF does as well.
@@ skipping 37 matching lines @@
   DownloadRequests::iterator i = download_requests_.find(id);
   if (i == download_requests_.end()) {
     // Currently WebContents notifies us of ANY downloads so that it is
     // possible to get here.
     return;
   }
 
   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()) {
         index = GetLargestSizeIndex(original_bitmap_sizes);
       } else {
         index = GetIndexBySize(original_bitmap_sizes,
                                current_candidate()->icon_sizes[0]);
         // Find largest bitmap if there is no one exactly matched.
         if (index == -1)
           index = GetLargestSizeIndex(original_bitmap_sizes);
       }
       image_skia = gfx::ImageSkia(gfx::ImageSkiaRep(bitmaps[index], 1));
     } else {
       image_skia = CreateFaviconImageSkia(bitmaps,
                                           original_bitmap_sizes,
                                           preferred_icon_size(),
                                           &score);
     }
 
     if (!image_skia.isNull()) {

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

I still need this check because |image_skia| might be NULL if one of |bitmaps|
is empty.

       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);
-      }
+      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();
-    }
+  }
+
+  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() {
   return !download_requests_.empty() ||
          cancelable_task_tracker_.HasTrackedTasks();
 }
 
 bool FaviconHandler::PageChangedSinceFaviconWasRequested() {
   if (UrlMatches(driver_->GetActiveURL(), url_) && url_.is_valid()) {
@@ skipping 102 matching lines @@
 
   if (has_results && handler_type_ == FAVICON &&
       !download_largest_icon_ && !driver_->GetActiveFaviconValidity() &&
       (!current_candidate() ||
        DoUrlsAndIconsMatch(*current_candidate(), favicon_bitmap_results))) {
     if (has_valid_result) {
       // The db knows the favicon (although it may be out of date) and the entry
       // 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.
       // TODO(pkotwicz): Do something better.
       favicon_expired_or_incomplete_ = true;
     }
   }
   if (has_results && !favicon_expired_or_incomplete_) {
     if (current_candidate() &&
         !DoUrlsAndIconsMatch(*current_candidate(), favicon_bitmap_results)) {
       // Mapping in the database is wrong. DownloadFavIconOrAskHistory will
       // update the mapping for this url and download the favicon if we don't
       // already have it.
       DownloadFaviconOrAskFaviconService(driver_->GetActiveURL(),
                                          current_candidate()->icon_url,
                                          current_candidate()->icon_type);
     }
   } else if (current_candidate()) {
     // We know the official url for the favicon, but either don't have the
     // favicon or it's expired. Continue on to DownloadFaviconOrAskHistory to
     // either download or check history again.
     DownloadFaviconOrAskFaviconService(driver_->GetActiveURL(),
                                        current_candidate()->icon_url,
                                        current_candidate()->icon_type);
   }
   // else we haven't got the icon url. When we get it we'll ask the
   // 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(
     const GURL& page_url,
     const GURL& icon_url,
     favicon_base::IconType icon_type) {
   if (favicon_expired_or_incomplete_) {
     // We have the mapping, but the favicon is out of date. Download it now.
     ScheduleDownload(page_url, icon_url, icon_type);
   } else {
@@ skipping 27 matching lines @@
   bool has_results = !favicon_bitmap_results.empty();
   bool has_expired_or_incomplete_result = HasExpiredOrIncompleteResult(
       preferred_icon_size(), favicon_bitmap_results);
   bool has_valid_result = HasValidResult(favicon_bitmap_results);
 
   if (has_results && handler_type_ == FAVICON && !download_largest_icon_) {
     if (has_valid_result) {
       // 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.
       ScheduleDownload(driver_->GetActiveURL(),
                        driver_->GetActiveFaviconURL(),
                        favicon_base::FAVICON);
     }
   } else if (current_candidate() &&
       (!has_results || has_expired_or_incomplete_result ||
        !(DoUrlsAndIconsMatch(*current_candidate(), favicon_bitmap_results)))) {
     // We don't know the favicon, it is out of date or its type is not same as
     // one got from page. Request the current one.
     ScheduleDownload(driver_->GetActiveURL(),
                      current_candidate()->icon_url,
                      current_candidate()->icon_type);
   }
   history_results_ = favicon_bitmap_results;
 
   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
   // 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);
   }
-
-  return download_id;
 }
 
 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