Reload favicon from HTTP cache on Ctrl+Refresh

chrome/browser/favicon/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 "chrome/browser/favicon/favicon_handler.h"
 
 #include <algorithm>
 #include <cmath>
 #include <vector>
 
@@ skipping 182 matching lines @@
       image(image),
       score(score),
       icon_type(icon_type) {}
 
 ////////////////////////////////////////////////////////////////////////////////
 
 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
                       : favicon_base::TOUCH_ICON |
                             favicon_base::TOUCH_PRECOMPOSED_ICON),
       download_largest_icon_(download_largest_icon),
       client_(client),
       driver_(driver) {
   DCHECK(driver_);
 }
 
 FaviconHandler::~FaviconHandler() {
 }
 
 void FaviconHandler::FetchFavicon(const GURL& url) {
   cancelable_task_tracker_.TryCancelAll();
 
   url_ = url;
 
+  waiting_for_favicon_service_data_ = true;
   favicon_expired_or_incomplete_ = got_favicon_from_history_ = false;
   image_urls_.clear();
 
   // 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.
   if (client_->GetFaviconService()) {
     GetFaviconForURLFromFaviconService(
         url_,
         icon_types_,
@@ skipping 144 matching lines @@
     const GURL& image_url,
     const std::vector<SkBitmap>& bitmaps,
     const std::vector<gfx::Size>& original_bitmap_sizes) {
   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;

[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;
     if (download_largest_icon_ && !bitmaps.empty()) {
       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()) {
       gfx::Image image(image_skia);
       // The downloaded icon is still valid when there is no FaviconURL update
       // 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() &&
         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) {
+      // 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,
                  best_favicon_candidate_.image,
                  best_favicon_candidate_.icon_type);
       // Reset candidate.
       image_urls_.clear();
       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() {
   if (UrlMatches(driver_->GetActiveURL(), url_) && url_.is_valid()) {
     return false;
   }
   // If the URL has changed out from under us (as will happen with redirects)
   // return true.
   return true;
 }
@@ skipping 75 matching lines @@
   }
   NOTREACHED();
   return 0;
 }
 
 void FaviconHandler::OnFaviconDataForInitialURLFromFaviconService(
     const std::vector<favicon_base::FaviconRawBitmapResult>&
         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();
   favicon_expired_or_incomplete_ = has_results && HasExpiredOrIncompleteResult(
       preferred_icon_size(), favicon_bitmap_results);
   bool has_valid_result = HasValidResult(favicon_bitmap_results);
 
   if (has_results && icon_types_ == favicon_base::FAVICON &&
       !download_largest_icon_ && !driver_->GetActiveFaviconValidity() &&
       (!current_candidate() ||
@@ skipping 42 matching lines @@
     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 if (client_->GetFaviconService()) {
     // We don't know the favicon, but we may have previously downloaded the
     // 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)),
           &cancelable_task_tracker_);
     } else {
       // Ask the history service for the icon. This does two things:
       // 1. Attempts to fetch the favicon data from the database.
       // 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_);
     }
   }
 }
 
 void FaviconHandler::OnFaviconData(const std::vector<
     favicon_base::FaviconRawBitmapResult>& favicon_bitmap_results) {
   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);
   bool has_valid_result = HasValidResult(favicon_bitmap_results);
 
   if (has_results && icon_types_ == favicon_base::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
@@ skipping 47 matching lines @@
     if (i->icon_sizes.empty())
       continue;
 
     gfx::Size largest = i->icon_sizes[GetLargestSizeIndex(i->icon_sizes)];
     i->icon_sizes.clear();
     i->icon_sizes.push_back(largest);
   }
   std::stable_sort(image_urls_.begin(), image_urls_.end(),
                    CompareIconSize);
 }