Removes usage of NavigationEntry from favicon_handler.*

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 "build/build_config.h"
 
 #include <algorithm>
 #include <vector>
@@ skipping 310 matching lines @@
 }
 
 void FaviconHandler::SetFavicon(const GURL& url,
                                 const GURL& icon_url,
                                 const gfx::Image& image,
                                 favicon_base::IconType icon_type) {
   if (client_->GetFaviconService() && ShouldSaveFavicon(url))
     SetHistoryFavicons(url, icon_url, icon_type, image);
 
   if (UrlMatches(url, url_) && icon_type == favicon_base::FAVICON) {
-    NavigationEntry* entry = GetEntry();
-    if (entry)
-      SetFaviconOnNavigationEntry(entry, icon_url, image);
+    if (NavigationEntryConcernsFavicon())
+      SetFaviconOnNavigationEntry(icon_url, image);
   }
 }
 
 void FaviconHandler::SetFaviconOnNavigationEntry(
-    NavigationEntry* entry,
     const std::vector<favicon_base::FaviconBitmapResult>&
         favicon_bitmap_results) {
   gfx::Image resized_image = FaviconUtil::SelectFaviconFramesFromPNGs(
       favicon_bitmap_results,
       FaviconUtil::GetFaviconScaleFactors(),
       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;
-  SetFaviconOnNavigationEntry(entry, icon_url, resized_image);
+  SetFaviconOnNavigationEntry(icon_url, resized_image);
 }
 
 void FaviconHandler::SetFaviconOnNavigationEntry(
-    NavigationEntry* entry,
     const GURL& icon_url,
     const gfx::Image& image) {
   // No matter what happens, we need to mark the favicon as being set.
-  entry->GetFavicon().valid = true;
+  driver_->SetActiveFaviconValidity(true);
 
-  bool icon_url_changed = (entry->GetFavicon().url != icon_url);
-  entry->GetFavicon().url = icon_url;
+  bool icon_url_changed = driver_->GetActiveFaviconURL() != icon_url;
+  driver_->SetActiveFaviconURL(icon_url);
 
   if (image.IsEmpty())
     return;
 
   gfx::Image image_with_adjusted_colorspace = image;
   FaviconUtil::SetFaviconColorSpace(&image_with_adjusted_colorspace);
 
-  entry->GetFavicon().image = image_with_adjusted_colorspace;
+  driver_->SetActiveFaviconImage(image_with_adjusted_colorspace);
   NotifyFaviconUpdated(icon_url_changed);
 }
 
 void FaviconHandler::OnUpdateFaviconURL(
     const std::vector<FaviconURL>& candidates) {
   image_urls_.clear();
   best_favicon_candidate_ = FaviconCandidate();
   for (std::vector<FaviconURL>::const_iterator i = candidates.begin();
        i != candidates.end(); ++i) {
     if (!i->icon_url.is_empty() && (i->icon_type & icon_types_))
@@ skipping 10 matching lines @@
 
   if (download_largest_icon_)
     SortAndPruneImageUrls();
 
   ProcessCurrentUrl();
 }
 
 void FaviconHandler::ProcessCurrentUrl() {
   DCHECK(!image_urls_.empty());
 
-  NavigationEntry* entry = GetEntry();
-
   // current_candidate() may return NULL if download_largest_icon_ is true and
   // all the sizes are larger than the max.
-  if (!entry || !current_candidate())
+  if (NavigationEntryConcernsFavicon() || !current_candidate())
     return;
 
   if (current_candidate()->icon_type == FaviconURL::FAVICON) {
-    if (!favicon_expired_or_incomplete_ && entry->GetFavicon().valid &&
+    if (!favicon_expired_or_incomplete_ &&
+        driver_->GetActiveFaviconValidity() &&

[blundell, 2014/05/06 15:04:00]

Hmm, maybe these would all read better as "FooForCurrentPage"?

GetURLForCurrentPage()
GetFaviconValidityForCurrentPage()
SetFaviconImageForCurrentPage()

etc. WDYT?

[jif, 2014/05/09 14:17:18]

Done.

         DoUrlAndIconMatch(*current_candidate(),
-                          entry->GetFavicon().url,
+                          driver_->GetActiveFaviconURL(),
                           favicon_base::FAVICON))
       return;
   } else if (!favicon_expired_or_incomplete_ && got_favicon_from_history_ &&
              HasValidResult(history_results_) &&
              DoUrlsAndIconsMatch(*current_candidate(), history_results_)) {
     return;
   }
 
   if (got_favicon_from_history_)
     DownloadFaviconOrAskFaviconService(
-        entry->GetURL(), current_candidate()->icon_url,
+        driver_->GetActiveURL(),
+        current_candidate()->icon_url,
         ToChromeIconType(current_candidate()->icon_type));
 }
 
 void FaviconHandler::OnDidDownloadFavicon(
     int id,
     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()) {
@@ skipping 36 matching lines @@
 
     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);
       }
     }
-    if (request_next_icon && GetEntry() && image_urls_.size() > 1) {
+    if (request_next_icon && NavigationEntryConcernsFavicon() &&
+        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();
       best_favicon_candidate_ = FaviconCandidate();
     }
   }
   download_requests_.erase(i);
 }
 
-NavigationEntry* FaviconHandler::GetEntry() {
+bool FaviconHandler::NavigationEntryConcernsFavicon() {

[blundell, 2014/05/06 15:04:00]

I think this is just something like "PageHasChangedSinceFoo", where "Foo" is the
event that causes url_ to be set.

[jif, 2014/05/09 14:17:18]

Done.

   NavigationEntry* entry = driver_->GetActiveEntry();

[blundell, 2014/05/06 15:04:00]

Could you get rid of all mentions of NavigationEntry in this file?

[jif, 2014/05/09 14:17:18]

Yeah, just wanted to get to a state were it was building.

-  if (entry && UrlMatches(entry->GetURL(), url_))
-    return entry;
+  if (entry && UrlMatches(driver_->GetActiveURL(), url_))
+    return true;
 
   // If the URL has changed out from under us (as will happen with redirects)
-  // return NULL.
-  return NULL;
+  // return false.
+  return false;
 }
 
 int FaviconHandler::DownloadFavicon(const GURL& image_url,
                                     int max_bitmap_size) {
   if (!image_url.is_valid()) {
     NOTREACHED();
     return 0;
   }
   return driver_->StartDownload(image_url, max_bitmap_size);
 }
@@ skipping 49 matching lines @@
   return client_->IsBookmarked(url);
 }
 
 void FaviconHandler::NotifyFaviconUpdated(bool icon_url_changed) {
   driver_->NotifyFaviconUpdated(icon_url_changed);
 }
 
 void FaviconHandler::OnFaviconDataForInitialURLFromFaviconService(
     const std::vector<favicon_base::FaviconBitmapResult>&
         favicon_bitmap_results) {
-  NavigationEntry* entry = GetEntry();

[blundell, 2014/05/06 15:39:48]

This code was previously getting the active NavigationEntry and then
getting/setting state from it through the rest of this method body. In the CL
under development here, the state getters and setters would each get the active
NavigationEntry in their own method bodies. My understanding is that this change
is OK to make because the active NavigationEntry won't change while this code is
executing. Is that correct?

[jam, 2014/05/06 18:18:28]

yep I don't see how that would change. the nav entry would be committed based on
IPCs from the renderer, which aren't going to be dispatched while this code is
running

-  if (!entry)
+  if (!NavigationEntryConcernsFavicon())
     return;
 
   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);
 
   if (has_results && icon_types_ == favicon_base::FAVICON &&
-      !entry->GetFavicon().valid &&
+      !driver_->GetActiveFaviconValidity() &&
       (!current_candidate() ||
        DoUrlsAndIconsMatch(*current_candidate(), favicon_bitmap_results))) {
     if (HasValidResult(favicon_bitmap_results)) {
       // 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.
-      SetFaviconOnNavigationEntry(entry, favicon_bitmap_results);
+      SetFaviconOnNavigationEntry(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(
-          entry->GetURL(), current_candidate()->icon_url,
+          driver_->GetActiveURL(),
+          current_candidate()->icon_url,
           ToChromeIconType(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(
-        entry->GetURL(), current_candidate()->icon_url,
+        driver_->GetActiveURL(),
+        current_candidate()->icon_url,
         ToChromeIconType(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.
 }
 
 void FaviconHandler::DownloadFaviconOrAskFaviconService(
     const GURL& page_url,
     const GURL& icon_url,
     favicon_base::IconType icon_type) {
@@ skipping 18 matching lines @@
       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::FaviconBitmapResult>& favicon_bitmap_results) {
-  NavigationEntry* entry = GetEntry();
-  if (!entry)
+  if (!NavigationEntryConcernsFavicon())
     return;
 
   bool has_results = !favicon_bitmap_results.empty();
   bool has_expired_or_incomplete_result = HasExpiredOrIncompleteResult(
       preferred_icon_size(), favicon_bitmap_results);
 
   if (has_results && icon_types_ == favicon_base::FAVICON) {
     if (HasValidResult(favicon_bitmap_results)) {
       // 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.
-      SetFaviconOnNavigationEntry(entry, favicon_bitmap_results);
+      SetFaviconOnNavigationEntry(favicon_bitmap_results);
     }
     if (has_expired_or_incomplete_result) {
       // The favicon is out of date. Request the current one.
-      ScheduleDownload(
-          entry->GetURL(), entry->GetFavicon().url, favicon_base::FAVICON);
+      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(entry->GetURL(), current_candidate()->icon_url,
+    ScheduleDownload(driver_->GetActiveURL(),
+                     current_candidate()->icon_url,
                      ToChromeIconType(current_candidate()->icon_type));
   }
   history_results_ = favicon_bitmap_results;
 }
 
 int 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()
@@ skipping 24 matching lines @@
     } else {
       gfx::Size largest = i->icon_sizes[index];
       i->icon_sizes.clear();
       i->icon_sizes.push_back(largest);
       ++i;
     }
   }
   std::stable_sort(image_urls_.begin(), image_urls_.end(),
                    CompareIconSize);
 }