Reload favicon from HTTP cache on Ctrl+Refresh

chrome/browser/favicon/favicon_handler.cc

Index: chrome/browser/favicon/favicon_handler.cc
diff --git a/chrome/browser/favicon/favicon_handler.cc b/chrome/browser/favicon/favicon_handler.cc
index a1bbe173e233fe2a84ef2239ba01678f1c2f9afc..632e47164ed9bb6672233e348426bb8187a0340f 100644
--- a/chrome/browser/favicon/favicon_handler.cc
+++ b/chrome/browser/favicon/favicon_handler.cc
@@ -200,7 +200,8 @@ FaviconHandler::FaviconHandler(FaviconClient* client,
                                FaviconDriver* driver,
                                Type icon_type,
                                bool download_largest_icon)
-    : got_favicon_from_history_(false),
+    : waiting_for_favicon_service_data_(false),
+      got_favicon_from_history_(false),
       favicon_expired_or_incomplete_(false),
       icon_types_(icon_type == FAVICON
                       ? favicon_base::FAVICON
@@ -220,6 +221,7 @@ void FaviconHandler::FetchFavicon(const GURL& url) {
 
   url_ = url;
 
+  waiting_for_favicon_service_data_ = true;
   favicon_expired_or_incomplete_ = got_favicon_from_history_ = false;
   image_urls_.clear();
 
@@ -384,8 +386,11 @@ void FaviconHandler::OnDidDownloadFavicon(
     return;
   }
 
+  DownloadRequest download_request = i->second;

[sky, 2015/02/19 18:08:10]

I'm assuming you didn't make this a const & because you made it so
download_requests_ is cleared below. I don't like clearing download_requests_
for the reasons I mention below, but if you do end up keeping it you should
document here why download_requests is a value. It's subtle.

   if (current_candidate() &&
-      DoUrlAndIconMatch(*current_candidate(), image_url, i->second.icon_type)) {
+      DoUrlAndIconMatch(*current_candidate(),
+                        image_url,
+                        download_request.icon_type)) {
     bool request_next_icon = true;
     float score = 0.0f;
     gfx::ImageSkia image_skia;
@@ -415,7 +420,8 @@ void FaviconHandler::OnDidDownloadFavicon(
       // during the downloading.
       if (!bitmaps.empty()) {
         request_next_icon = !UpdateFaviconCandidate(
-            i->second.url, image_url, image, score, i->second.icon_type);
+            download_request.url, image_url, image, score,
+            download_request.icon_type);
       }
     }
     if (request_next_icon && !PageChangedSinceFaviconWasRequested() &&
@@ -425,6 +431,11 @@ void FaviconHandler::OnDidDownloadFavicon(
       ProcessCurrentUrl();
     } else if (best_favicon_candidate_.icon_type !=
                favicon_base::INVALID_ICON) {
+      // Clear |download_requests_| so that a receiver of the notification
+      // sent by FaviconDriver::OnFaviconAvailable() can correctly compute
+      // HasPendingDownloadsForTest().

[sky, 2015/02/19 18:08:10]

I'm confused. Why do we need to do this? If it's just for a test can't the test
figure out whatever it needs without changing this?

+      download_requests_.clear();
+
       // No more icons to request, set the favicon from the candidate.
       SetFavicon(best_favicon_candidate_.url,
                  best_favicon_candidate_.image_url,
@@ -435,7 +446,15 @@ void FaviconHandler::OnDidDownloadFavicon(
       best_favicon_candidate_ = FaviconCandidate();
     }
   }
-  download_requests_.erase(i);
+  download_requests_.erase(id);

[sky, 2015/02/19 18:08:10]

This is subtle too. |i| may have been invalidated. Again, I don't think you
should be messing with download_requests_ on 437. It makes this function harder
to reason about.

+}
+
+bool FaviconHandler::HasPendingFaviconServiceRequestsForTest() const {
+  return waiting_for_favicon_service_data_;
+}
+
+bool FaviconHandler::HasPendingDownloadsForTest() const {
+  return !download_requests_.empty();
 }
 
 bool FaviconHandler::PageChangedSinceFaviconWasRequested() {
@@ -531,6 +550,7 @@ void FaviconHandler::OnFaviconDataForInitialURLFromFaviconService(
         favicon_bitmap_results) {
   if (PageChangedSinceFaviconWasRequested())
     return;
+  waiting_for_favicon_service_data_ = false;
   got_favicon_from_history_ = true;
   history_results_ = favicon_bitmap_results;
   bool has_results = !favicon_bitmap_results.empty();
@@ -593,6 +613,7 @@ void FaviconHandler::DownloadFaviconOrAskFaviconService(
     // favicon for another page that shares the same favicon. Ask for the
     // favicon given the favicon URL.
     if (driver_->IsOffTheRecord()) {
+      waiting_for_favicon_service_data_ = true;

[sky, 2015/02/19 18:08:10]

Is there a reason you don't want to move setting this to
GetFaviconFromFaviconService and UpdateFaviconMappingAndFetch?

       GetFaviconFromFaviconService(
           icon_url, icon_type,
           base::Bind(&FaviconHandler::OnFaviconData, base::Unretained(this)),
@@ -603,7 +624,8 @@ void FaviconHandler::DownloadFaviconOrAskFaviconService(
       // 2. If the favicon exists in the database, this updates the database to
       //    include the mapping between the page url and the favicon url.
       // This is asynchronous. The history service will call back when done.
-      UpdateFaviconMappingAndFetch(
+     waiting_for_favicon_service_data_ = true;
+     UpdateFaviconMappingAndFetch(
           page_url, icon_url, icon_type,
           base::Bind(&FaviconHandler::OnFaviconData, base::Unretained(this)),
           &cancelable_task_tracker_);
@@ -616,6 +638,8 @@ void FaviconHandler::OnFaviconData(const std::vector<
   if (PageChangedSinceFaviconWasRequested())
     return;
 
+  waiting_for_favicon_service_data_ = false;
+
   bool has_results = !favicon_bitmap_results.empty();
   bool has_expired_or_incomplete_result = HasExpiredOrIncompleteResult(
       preferred_icon_size(), favicon_bitmap_results);