Remove DownloadRequest registry from 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 157 matching lines @@
 
 // Checks whether two FaviconURLs are equal ignoring the icon sizes.
 bool FaviconURLsEqualIgnoringSizes(const FaviconURL& u1, const FaviconURL& u2) {
   return u1.icon_type == u2.icon_type && u1.icon_url == u2.icon_url;
 }
 
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
 
-FaviconHandler::DownloadRequest::DownloadRequest()
-    : icon_type(favicon_base::INVALID_ICON) {
-}
-
-FaviconHandler::DownloadRequest::~DownloadRequest() {
-}
-
-FaviconHandler::DownloadRequest::DownloadRequest(
-    const GURL& image_url,
-    favicon_base::IconType icon_type)
-    : image_url(image_url), icon_type(icon_type) {
-}
-
-////////////////////////////////////////////////////////////////////////////////
-
 FaviconHandler::FaviconCandidate::FaviconCandidate()
     : score(0), icon_type(favicon_base::INVALID_ICON) {
 }
 
 FaviconHandler::FaviconCandidate::~FaviconCandidate() {
 }
 
 FaviconHandler::FaviconCandidate::FaviconCandidate(
     const GURL& image_url,
     const gfx::Image& image,
@@ skipping 14 matching lines @@
       got_favicon_from_history_(false),
       initial_history_result_expired_or_incomplete_(false),
       redownload_icons_(false),
       icon_types_(FaviconHandler::GetIconTypesFromHandlerType(handler_type)),
       download_largest_icon_(
           handler_type == FaviconDriverObserver::NON_TOUCH_LARGEST ||
           handler_type == FaviconDriverObserver::TOUCH_LARGEST),
       notification_icon_type_(favicon_base::INVALID_ICON),
       service_(service),
       delegate_(delegate),
-      current_candidate_index_(0u),
-      weak_ptr_factory_(this) {
+      current_candidate_index_(0u) {
   DCHECK(delegate_);
 }
 
 FaviconHandler::~FaviconHandler() {
 }
 
 // static
 int FaviconHandler::GetIconTypesFromHandlerType(
     FaviconDriverObserver::NotificationIconType handler_type) {
   switch (handler_type) {
     case FaviconDriverObserver::NON_TOUCH_16_DIP:
     case FaviconDriverObserver::NON_TOUCH_LARGEST:
       return favicon_base::FAVICON;
     case FaviconDriverObserver::TOUCH_LARGEST:
       return favicon_base::TOUCH_ICON | favicon_base::TOUCH_PRECOMPOSED_ICON;
   }
   return 0;
 }
 
 void FaviconHandler::FetchFavicon(const GURL& url) {
   cancelable_task_tracker_.TryCancelAll();
 
   url_ = url;
 
   initial_history_result_expired_or_incomplete_ = false;
   redownload_icons_ = false;
   got_favicon_from_history_ = false;
-  download_requests_.clear();
+  download_request_.Cancel();
   image_urls_.clear();
   notification_icon_url_ = GURL();
   notification_icon_type_ = favicon_base::INVALID_ICON;
   current_candidate_index_ = 0u;
   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.
   if (service_) {
@@ skipping 113 matching lines @@
     SortAndPruneImageUrls(&pruned_candidates);
 
   // Ignore FaviconURL::icon_sizes because FaviconURL::icon_sizes is not stored
   // in the history database.
   if (image_urls_.size() == pruned_candidates.size() &&
       std::equal(pruned_candidates.begin(), pruned_candidates.end(),
                  image_urls_.begin(), FaviconURLsEqualIgnoringSizes)) {
     return;
   }
 
-  download_requests_.clear();
+  download_request_.Cancel();
   image_urls_ = pruned_candidates;
   current_candidate_index_ = 0u;
   best_favicon_candidate_ = FaviconCandidate();
 
   // TODO(davemoore) Should clear on empty url. Currently we ignore it.
   // This appears to be what FF does as well.
   if (current_candidate() && got_favicon_from_history_)
     OnGotInitialHistoryDataAndIconURLCandidates();
 }
 
@@ skipping 27 matching lines @@
     // TODO: Store all of the icon URLs associated with a page in history so
     // that we can check whether the page's icon URLs match the page's icon URLs
     // at the time that the favicon data was stored to the history database.
     return;
   }
 
   DownloadCurrentCandidateOrAskFaviconService();
 }
 
 void FaviconHandler::OnDidDownloadFavicon(
+    favicon_base::IconType icon_type,
     int id,
     int http_status_code,
     const GURL& image_url,
     const std::vector<SkBitmap>& bitmaps,
     const std::vector<gfx::Size>& original_bitmap_sizes) {
+  // Mark ongoing download as finalized.

[pkotwicz, 2017/03/10 03:42:29]

Nit: How about: "Mark download as finished."

[mastiz, 2017/03/10 11:34:50]

Done.

+  download_request_.Cancel();
+
   if (bitmaps.empty() && http_status_code == 404) {
     DVLOG(1) << "Failed to Download Favicon:" << image_url;
     if (service_)
       service_->UnableToDownloadFavicon(image_url);
   }
 
-  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)) {
-    return;
-  }
+  DCHECK(current_candidate());
+  DCHECK(DoUrlAndIconMatch(*current_candidate(), image_url, icon_type));

[pkotwicz, 2017/03/10 03:42:29]

Nit: Remove the DCHECKs(). I generally don't think that DCHECKs are very useful.

[mastiz, 2017/03/10 11:34:50]

Done.

 
   bool request_next_icon = true;
   if (!bitmaps.empty()) {
     float score = 0.0f;
     gfx::ImageSkia image_skia;
     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()) {
       gfx::Image image(image_skia);
       // The downloaded icon is still valid when there is no FaviconURL update
       // during the downloading.
-      request_next_icon = !UpdateFaviconCandidate(image_url, image, score,
-                                                  download_request.icon_type);
+      request_next_icon =
+          !UpdateFaviconCandidate(image_url, image, score, icon_type);
     }
   }
 
   if (request_next_icon && current_candidate_index_ + 1 < image_urls_.size()) {
     // Process the next candidate.
     ++current_candidate_index_;
     DownloadCurrentCandidateOrAskFaviconService();
   } else {
     // We have either found the ideal candidate or run out of candidates.
     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_.image_url,
                  best_favicon_candidate_.image,
                  best_favicon_candidate_.icon_type);
     }
     // Clear download related state.
-    download_requests_.clear();
     current_candidate_index_ = image_urls_.size();
     best_favicon_candidate_ = FaviconCandidate();
   }
 }
 
 bool FaviconHandler::HasPendingTasksForTest() {
-  return !download_requests_.empty() ||
+  return !download_request_.IsCancelled() ||
          cancelable_task_tracker_.HasTrackedTasks();
 }
 
 bool FaviconHandler::ShouldSaveFavicon() {
   if (!delegate_->IsOffTheRecord())
     return true;
 
   // Always save favicon if the page is bookmarked.
   return delegate_->IsBookmarked(url_);
 }
@@ skipping 81 matching lines @@
 
   if (has_expired_or_incomplete_result) {
     ScheduleDownload(current_candidate()->icon_url,
                      current_candidate()->icon_type);
   }
 }
 
 void FaviconHandler::ScheduleDownload(const GURL& image_url,
                                       favicon_base::IconType icon_type) {
   DCHECK(image_url.is_valid());
+  // Note that CancelableCallback starts cancelled.
+  DCHECK(download_request_.IsCancelled()) << "More than one ongoing download";
   if (service_ && service_->WasUnableToDownloadFavicon(image_url)) {
     DVLOG(1) << "Skip Failed FavIcon: " << image_url;
-    // Registration in download_requests_ is needed to prevent
-    // OnDidDownloadFavicon() from returning early.
-    download_requests_[0] = DownloadRequest(image_url, icon_type);
-    OnDidDownloadFavicon(0, 0, image_url, std::vector<SkBitmap>(),
+    OnDidDownloadFavicon(icon_type, 0, 0, image_url, std::vector<SkBitmap>(),
                          std::vector<gfx::Size>());
     return;
   }
+  download_request_.Reset(base::Bind(&FaviconHandler::OnDidDownloadFavicon,
+                                     base::Unretained(this), 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 =
-      delegate_->DownloadImage(image_url, GetMaximalIconSize(icon_type),
-                               base::Bind(&FaviconHandler::OnDidDownloadFavicon,
-                                          weak_ptr_factory_.GetWeakPtr()));
+  const int download_id = delegate_->DownloadImage(
+      image_url, GetMaximalIconSize(icon_type), download_request_.callback());
   DCHECK_NE(download_id, 0);
-
-  // Download ids should be unique.
-  DCHECK(download_requests_.find(download_id) == download_requests_.end());
-  download_requests_[download_id] = DownloadRequest(image_url, icon_type);
 }
 
 }  // namespace favicon