Removes usage of NavigationEntry from favicon_handler.*

BUG=359598

Review URL: https://codereview.chromium.org/261403003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@269871 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/favicon/favicon_handler.cc

@@ -15,8 +15,6 @@
 #include "chrome/browser/favicon/favicon_service_factory.h"
 #include "chrome/browser/favicon/favicon_util.h"
 #include "components/favicon_base/select_favicon_frames.h"
-#include "content/public/browser/favicon_status.h"
-#include "content/public/browser/navigation_entry.h"
 #include "skia/ext/image_operations.h"
 #include "ui/gfx/codec/png_codec.h"
 #include "ui/gfx/image/image.h"
@@ -24,7 +22,6 @@
 #include "ui/gfx/image/image_util.h"
 
 using content::FaviconURL;
-using content::NavigationEntry;
 
 namespace {
 
@@ -328,16 +325,13 @@ void FaviconHandler::SetFavicon(const GURL& 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 (!PageChangedSinceFaviconWasRequested())
+      SetFaviconOnActivePage(icon_url, image);
   }
 }
 
-void FaviconHandler::SetFaviconOnNavigationEntry(
-    NavigationEntry* entry,
-    const std::vector<favicon_base::FaviconBitmapResult>&
-        favicon_bitmap_results) {
+void FaviconHandler::SetFaviconOnActivePage(const std::vector<
+    favicon_base::FaviconBitmapResult>& favicon_bitmap_results) {
   gfx::Image resized_image = FaviconUtil::SelectFaviconFramesFromPNGs(
       favicon_bitmap_results,
       FaviconUtil::GetFaviconScaleFactors(),
@@ -346,18 +340,16 @@ void FaviconHandler::SetFaviconOnNavigationEntry(
   // 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);
+  SetFaviconOnActivePage(icon_url, resized_image);
 }
 
-void FaviconHandler::SetFaviconOnNavigationEntry(
-    NavigationEntry* entry,
-    const GURL& icon_url,
-    const gfx::Image& image) {
+void FaviconHandler::SetFaviconOnActivePage(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;
@@ -365,7 +357,7 @@ void FaviconHandler::SetFaviconOnNavigationEntry(
   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);
 }
 
@@ -396,17 +388,16 @@ void FaviconHandler::OnUpdateFaviconURL(
 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 (PageChangedSinceFaviconWasRequested() || !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() &&
         DoUrlAndIconMatch(*current_candidate(),
-                          entry->GetFavicon().url,
+                          driver_->GetActiveFaviconURL(),
                           favicon_base::FAVICON))
       return;
   } else if (!favicon_expired_or_incomplete_ && got_favicon_from_history_ &&
@@ -417,7 +408,8 @@ void FaviconHandler::ProcessCurrentUrl() {
 
   if (got_favicon_from_history_)
     DownloadFaviconOrAskFaviconService(
-        entry->GetURL(), current_candidate()->icon_url,
+        driver_->GetActiveURL(),
+        current_candidate()->icon_url,
         ToChromeIconType(current_candidate()->icon_type));
 }
 
@@ -474,7 +466,8 @@ void FaviconHandler::OnDidDownloadFavicon(
             i->second.url, image_url, image, score, i->second.icon_type);
       }
     }
-    if (request_next_icon && GetEntry() && image_urls_.size() > 1) {
+    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();
@@ -493,14 +486,13 @@ void FaviconHandler::OnDidDownloadFavicon(
   download_requests_.erase(i);
 }
 
-NavigationEntry* FaviconHandler::GetEntry() {
-  NavigationEntry* entry = driver_->GetActiveEntry();
-  if (entry && UrlMatches(entry->GetURL(), url_))
-    return entry;
-
+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 NULL.
-  return NULL;
+  // return true.
+  return true;
 }
 
 int FaviconHandler::DownloadFavicon(const GURL& image_url,
@@ -570,19 +562,15 @@ void FaviconHandler::NotifyFaviconUpdated(bool icon_url_changed) {
 void FaviconHandler::OnFaviconDataForInitialURLFromFaviconService(
     const std::vector<favicon_base::FaviconBitmapResult>&
         favicon_bitmap_results) {
-  NavigationEntry* entry = GetEntry();
-  if (!entry)
+  if (PageChangedSinceFaviconWasRequested())
     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)) {
@@ -590,7 +578,7 @@ void FaviconHandler::OnFaviconDataForInitialURLFromFaviconService(
       // 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);
+      SetFaviconOnActivePage(favicon_bitmap_results);
     } else {
       // If |favicon_bitmap_results| does not have any valid results, treat the
       // favicon as if it's expired.
@@ -598,7 +586,6 @@ void FaviconHandler::OnFaviconDataForInitialURLFromFaviconService(
       favicon_expired_or_incomplete_ = true;
     }
   }
-
   if (has_results && !favicon_expired_or_incomplete_) {
     if (current_candidate() &&
         !DoUrlsAndIconsMatch(*current_candidate(), favicon_bitmap_results)) {
@@ -606,7 +593,8 @@ void FaviconHandler::OnFaviconDataForInitialURLFromFaviconService(
       // 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()) {
@@ -614,7 +602,8 @@ void FaviconHandler::OnFaviconDataForInitialURLFromFaviconService(
     // 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
@@ -653,8 +642,7 @@ void FaviconHandler::DownloadFaviconOrAskFaviconService(
 
 void FaviconHandler::OnFaviconData(const std::vector<
     favicon_base::FaviconBitmapResult>& favicon_bitmap_results) {
-  NavigationEntry* entry = GetEntry();
-  if (!entry)
+  if (PageChangedSinceFaviconWasRequested())
     return;
 
   bool has_results = !favicon_bitmap_results.empty();
@@ -666,19 +654,21 @@ void FaviconHandler::OnFaviconData(const std::vector<
       // 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);
+      SetFaviconOnActivePage(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;

chrome/browser/favicon/favicon_handler.h

@@ -28,11 +28,7 @@ namespace base {
 class RefCountedMemory;
 }
 
-namespace content {
-class NavigationEntry;
-}
-
-// FaviconHandler works with FaviconTabHelper to fetch the specific type of
+// FaviconHandler works with FaviconDriver to fetch the specific type of
 // favicon.
 //
 // FetchFavicon requests the favicon from the favicon service which in turn
@@ -40,7 +36,7 @@ class NavigationEntry;
 // we only know the URL of the page, and not necessarily the url of the
 // favicon. To ensure we handle reloading stale favicons as well as
 // reloading a favicon on page reload we always request the favicon from
-// history regardless of whether the NavigationEntry has a favicon.
+// history regardless of whether the active favicon is valid.
 //
 // After the navigation two types of events are delivered (which is
 // first depends upon who is faster): notification from the history
@@ -48,13 +44,13 @@ class NavigationEntry;
 // (OnFaviconDataForInitialURLFromFaviconService), or a message from the
 // renderer giving us the URL of the favicon for the page (SetFaviconURL).
 // . If the history db has a valid up to date favicon for the page, we update
-//   the NavigationEntry and use the favicon.
-// . When we receive the favicon url if it matches that of the NavigationEntry
-//   and the NavigationEntry's favicon is set, we do nothing (everything is
+//   the current page and use the favicon.
+// . When we receive the favicon url if it matches that of the current page
+//   and the current page's favicon is set, we do nothing (everything is
 //   ok).
 // . On the other hand if the database does not know the favicon for url, or
 //   the favicon is out date, or the URL from the renderer does not match that
-//   NavigationEntry we proceed to DownloadFaviconOrAskHistory. Before we
+//   of the current page we proceed to DownloadFaviconOrAskHistory. Before we
 //   invoke DownloadFaviconOrAskHistory we wait until we've received both
 //   the favicon url and the callback from history. We wait to ensure we
 //   truly know both the favicon url and the state of the database.
@@ -67,7 +63,7 @@ class NavigationEntry;
 //   possible for the db to already have the favicon, just not the mapping
 //   between page to favicon url. The callback for this is OnFaviconData.
 //
-// OnFaviconData either updates the favicon of the NavigationEntry (if the
+// OnFaviconData either updates the favicon of the current page (if the
 // db knew about the favicon), or requests the renderer to download the
 // favicon.
 //
@@ -76,8 +72,7 @@ class NavigationEntry;
 // favicon will be used, otherwise the one that best matches the preferred size
 // is chosen (or the first one if there is no preferred  size). Once the
 // matching favicon has been determined, SetFavicon is called which updates
-// the favicon of the NavigationEntry and notifies the database to save the
-// favicon.
+// the page's favicon and notifies the database to save the favicon.
 
 class FaviconHandler {
  public:
@@ -123,10 +118,6 @@ class FaviconHandler {
   // These virtual methods make FaviconHandler testable and are overridden by
   // TestFaviconHandler.
 
-  // Return the NavigationEntry for the active entry, or NULL if the active
-  // entries URL does not match that of the URL last passed to FetchFavicon.
-  virtual content::NavigationEntry* GetEntry();
-
   // Asks the render to download favicon, returns the request id.
   virtual int DownloadFavicon(const GURL& image_url, int max_bitmap_size);
 
@@ -232,21 +223,19 @@ class FaviconHandler {
                   const gfx::Image& image,
                   favicon_base::IconType icon_type);
 
-  // Sets the favicon's data on the NavigationEntry.
-  // If the WebContents has a delegate, it is invalidated (INVALIDATE_TYPE_TAB).
-  void SetFaviconOnNavigationEntry(
-      content::NavigationEntry* entry,
-      const std::vector<favicon_base::FaviconBitmapResult>&
-          favicon_bitmap_results);
-  void SetFaviconOnNavigationEntry(content::NavigationEntry* entry,
-                                   const GURL& icon_url,
-                                   const gfx::Image& image);
+  // Sets the favicon's data.
+  void SetFaviconOnActivePage(const std::vector<
+      favicon_base::FaviconBitmapResult>& favicon_bitmap_results);
+  void SetFaviconOnActivePage(const GURL& icon_url, const gfx::Image& image);
 
   // Return the current candidate if any.
   content::FaviconURL* current_candidate() {
     return (!image_urls_.empty()) ? &image_urls_.front() : NULL;
   }
 
+  // Returns whether the page's url changed since the favicon was requested.
+  bool PageChangedSinceFaviconWasRequested();
+
   // Returns the preferred size of the image. 0 means no preference (any size
   // will do).
   int preferred_icon_size() const {
@@ -299,7 +288,7 @@ class FaviconHandler {
 
   // Best image we've seen so far.  As images are downloaded from the page they
   // are stored here. When there is an exact match, or no more images are
-  // available the favicon service and the NavigationEntry are updated (assuming
+  // available the favicon service and the current page are updated (assuming
   // the image is for a favicon).
   FaviconCandidate best_favicon_candidate_;
 

chrome/browser/favicon/favicon_handler_unittest.cc

@@ -8,10 +8,6 @@
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
-#include "content/public/browser/favicon_status.h"
-#include "content/public/browser/invalidate_type.h"
-#include "content/public/browser/navigation_entry.h"
-#include "content/public/browser/web_contents.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/gfx/codec/png_codec.h"
 #include "ui/gfx/favicon_size.h"
@@ -20,8 +16,6 @@
 class TestFaviconHandler;
 
 using content::FaviconURL;
-using content::NavigationEntry;
-using content::WebContents;
 
 namespace {
 
@@ -191,18 +185,31 @@ class TestFaviconClient : public FaviconClient {
 
 class TestFaviconDriver : public FaviconDriver {
  public:
-  TestFaviconDriver() {
-  }
+  TestFaviconDriver() : favicon_validity_(false) {}
 
   virtual ~TestFaviconDriver() {
   }
 
   virtual bool IsOffTheRecord() OVERRIDE { return false; }
 
-  virtual NavigationEntry* GetActiveEntry() OVERRIDE {
-    ADD_FAILURE() << "TestFaviconDriver::GetActiveEntry() "
-                  << "should never be called in tests.";
-    return NULL;
+  virtual const gfx::Image GetActiveFaviconImage() OVERRIDE { return image_; }
+
+  virtual const GURL GetActiveFaviconURL() OVERRIDE { return favicon_url_; }
+
+  virtual bool GetActiveFaviconValidity() OVERRIDE { return favicon_validity_; }
+
+  virtual const GURL GetActiveURL() OVERRIDE { return url_; }
+
+  virtual void SetActiveFaviconImage(gfx::Image image) OVERRIDE {
+    image_ = image;
+  }
+
+  virtual void SetActiveFaviconURL(GURL favicon_url) OVERRIDE {
+    favicon_url_ = favicon_url;
+  }
+
+  virtual void SetActiveFaviconValidity(bool favicon_validity) OVERRIDE {
+    favicon_validity_ = favicon_validity;
   }
 
   virtual int StartDownload(const GURL& url,
@@ -217,7 +224,13 @@ class TestFaviconDriver : public FaviconDriver {
                   << "should never be called in tests.";
   }
 
+  void SetActiveURL(GURL url) { url_ = url; }
+
  private:
+  GURL favicon_url_;
+  GURL url_;
+  gfx::Image image_;
+  bool favicon_validity_;
   DISALLOW_COPY_AND_ASSIGN(TestFaviconDriver);
 };
 
@@ -228,15 +241,13 @@ class TestFaviconHandler : public FaviconHandler {
  public:
   TestFaviconHandler(const GURL& page_url,
                      FaviconClient* client,
-                     FaviconDriver* driver,
+                     TestFaviconDriver* driver,
                      Type type,
                      bool download_largest_icon)
-                     : FaviconHandler(client, driver, type,
-                                      download_largest_icon),
-        entry_(NavigationEntry::Create()),
+      : FaviconHandler(client, driver, type, download_largest_icon),
         download_id_(0),
         num_favicon_updates_(0) {
-    entry_->SetURL(page_url);
+    driver->SetActiveURL(page_url);
     download_handler_.reset(new DownloadHandler(this));
   }
 
@@ -265,10 +276,6 @@ class TestFaviconHandler : public FaviconHandler {
   }
 
   // Methods to access favicon internals.
-  virtual NavigationEntry* GetEntry() OVERRIDE {
-    return entry_.get();
-  }
-
   const std::vector<FaviconURL>& urls() {
     return image_urls_;
   }
@@ -342,7 +349,6 @@ class TestFaviconHandler : public FaviconHandler {
   GURL page_url_;
 
  private:
-  scoped_ptr<NavigationEntry> entry_;
 
   // The unique id of a download request. It will be returned to a
   // FaviconHandler.
@@ -491,8 +497,8 @@ TEST_F(FaviconHandlerTest, GetFaviconFromHistory) {
   // Send history response.
   history_handler->InvokeCallback();
   // Verify FaviconHandler status
-  EXPECT_TRUE(helper.GetEntry()->GetFavicon().valid);
-  EXPECT_EQ(icon_url, helper.GetEntry()->GetFavicon().url);
+  EXPECT_TRUE(driver.GetActiveFaviconValidity());
+  EXPECT_EQ(icon_url, driver.GetActiveFaviconURL());
 
   // Simulates update favicon url.
   std::vector<FaviconURL> urls;
@@ -535,8 +541,8 @@ TEST_F(FaviconHandlerTest, DownloadFavicon) {
   // Send history response.
   history_handler->InvokeCallback();
   // Verify FaviconHandler status
-  EXPECT_TRUE(helper.GetEntry()->GetFavicon().valid);
-  EXPECT_EQ(icon_url, helper.GetEntry()->GetFavicon().url);
+  EXPECT_TRUE(driver.GetActiveFaviconValidity());
+  EXPECT_EQ(icon_url, driver.GetActiveFaviconURL());
 
   // Simulates update favicon url.
   std::vector<FaviconURL> urls;
@@ -572,11 +578,10 @@ TEST_F(FaviconHandlerTest, DownloadFavicon) {
   EXPECT_EQ(page_url, history_handler->page_url_);
 
   // Verify NavigationEntry.
-  content::FaviconStatus favicon_status = helper.GetEntry()->GetFavicon();
-  EXPECT_EQ(icon_url, favicon_status.url);
-  EXPECT_TRUE(favicon_status.valid);
-  EXPECT_FALSE(favicon_status.image.IsEmpty());
-  EXPECT_EQ(gfx::kFaviconSize, favicon_status.image.Width());
+  EXPECT_EQ(icon_url, driver.GetActiveFaviconURL());
+  EXPECT_TRUE(driver.GetActiveFaviconValidity());
+  EXPECT_FALSE(driver.GetActiveFaviconImage().IsEmpty());
+  EXPECT_EQ(gfx::kFaviconSize, driver.GetActiveFaviconImage().Width());
 }
 
 TEST_F(FaviconHandlerTest, UpdateAndDownloadFavicon) {
@@ -603,8 +608,8 @@ TEST_F(FaviconHandlerTest, UpdateAndDownloadFavicon) {
   // Send history response.
   history_handler->InvokeCallback();
   // Verify FaviconHandler status.
-  EXPECT_TRUE(helper.GetEntry()->GetFavicon().valid);
-  EXPECT_EQ(icon_url, helper.GetEntry()->GetFavicon().url);
+  EXPECT_TRUE(driver.GetActiveFaviconValidity());
+  EXPECT_EQ(icon_url, driver.GetActiveFaviconURL());
 
   // Reset the history_handler to verify whether new icon is requested from
   // history.
@@ -655,11 +660,10 @@ TEST_F(FaviconHandlerTest, UpdateAndDownloadFavicon) {
   EXPECT_EQ(page_url, history_handler->page_url_);
 
   // Verify NavigationEntry.
-  content::FaviconStatus favicon_status = helper.GetEntry()->GetFavicon();
-  EXPECT_EQ(new_icon_url, favicon_status.url);
-  EXPECT_TRUE(favicon_status.valid);
-  EXPECT_FALSE(favicon_status.image.IsEmpty());
-  EXPECT_EQ(gfx::kFaviconSize, favicon_status.image.Width());
+  EXPECT_EQ(new_icon_url, driver.GetActiveFaviconURL());
+  EXPECT_TRUE(driver.GetActiveFaviconValidity());
+  EXPECT_FALSE(driver.GetActiveFaviconImage().IsEmpty());
+  EXPECT_EQ(gfx::kFaviconSize, driver.GetActiveFaviconImage().Width());
 }
 
 TEST_F(FaviconHandlerTest, FaviconInHistoryInvalid) {
@@ -693,8 +697,8 @@ TEST_F(FaviconHandlerTest, FaviconInHistoryInvalid) {
   // Send history response.
   history_handler->InvokeCallback();
   // The NavigationEntry should not be set yet as the history data is invalid.
-  EXPECT_FALSE(helper.GetEntry()->GetFavicon().valid);
-  EXPECT_EQ(GURL(), helper.GetEntry()->GetFavicon().url);
+  EXPECT_FALSE(driver.GetActiveFaviconValidity());
+  EXPECT_EQ(GURL(), driver.GetActiveFaviconURL());
 
   // Reset the history_handler to verify whether new icon is requested from
   // history.
@@ -727,11 +731,10 @@ TEST_F(FaviconHandlerTest, FaviconInHistoryInvalid) {
   EXPECT_EQ(page_url, history_handler->page_url_);
 
   // Verify NavigationEntry.
-  content::FaviconStatus favicon_status = helper.GetEntry()->GetFavicon();
-  EXPECT_EQ(icon_url, favicon_status.url);
-  EXPECT_TRUE(favicon_status.valid);
-  EXPECT_FALSE(favicon_status.image.IsEmpty());
-  EXPECT_EQ(gfx::kFaviconSize, favicon_status.image.Width());
+  EXPECT_EQ(icon_url, driver.GetActiveFaviconURL());
+  EXPECT_TRUE(driver.GetActiveFaviconValidity());
+  EXPECT_FALSE(driver.GetActiveFaviconImage().IsEmpty());
+  EXPECT_EQ(gfx::kFaviconSize, driver.GetActiveFaviconImage().Width());
 }
 
 TEST_F(FaviconHandlerTest, UpdateFavicon) {
@@ -757,8 +760,8 @@ TEST_F(FaviconHandlerTest, UpdateFavicon) {
   // Send history response.
   history_handler->InvokeCallback();
   // Verify FaviconHandler status.
-  EXPECT_TRUE(helper.GetEntry()->GetFavicon().valid);
-  EXPECT_EQ(icon_url, helper.GetEntry()->GetFavicon().url);
+  EXPECT_TRUE(driver.GetActiveFaviconValidity());
+  EXPECT_EQ(icon_url, driver.GetActiveFaviconURL());
 
   // Reset the history_handler to verify whether new icon is requested from
   // history.
@@ -791,9 +794,9 @@ TEST_F(FaviconHandlerTest, UpdateFavicon) {
   EXPECT_FALSE(helper.download_handler()->HasDownload());
 
   // Verify the favicon status.
-  EXPECT_EQ(new_icon_url, helper.GetEntry()->GetFavicon().url);
-  EXPECT_TRUE(helper.GetEntry()->GetFavicon().valid);
-  EXPECT_FALSE(helper.GetEntry()->GetFavicon().image.IsEmpty());
+  EXPECT_EQ(new_icon_url, driver.GetActiveFaviconURL());
+  EXPECT_TRUE(driver.GetActiveFaviconValidity());
+  EXPECT_FALSE(driver.GetActiveFaviconImage().IsEmpty());
 }
 
 TEST_F(FaviconHandlerTest, Download2ndFaviconURLCandidate) {
@@ -820,8 +823,8 @@ TEST_F(FaviconHandlerTest, Download2ndFaviconURLCandidate) {
   // Send history response.
   history_handler->InvokeCallback();
   // Verify FaviconHandler status.
-  EXPECT_FALSE(helper.GetEntry()->GetFavicon().valid);
-  EXPECT_EQ(GURL(), helper.GetEntry()->GetFavicon().url);
+  EXPECT_FALSE(driver.GetActiveFaviconValidity());
+  EXPECT_EQ(GURL(), driver.GetActiveFaviconURL());
 
   // Reset the history_handler to verify whether new icon is requested from
   // history.
@@ -934,8 +937,8 @@ TEST_F(FaviconHandlerTest, UpdateDuringDownloading) {
   // Send history response.
   history_handler->InvokeCallback();
   // Verify FaviconHandler status.
-  EXPECT_FALSE(helper.GetEntry()->GetFavicon().valid);
-  EXPECT_EQ(GURL(), helper.GetEntry()->GetFavicon().url);
+  EXPECT_FALSE(driver.GetActiveFaviconValidity());
+  EXPECT_EQ(GURL(), driver.GetActiveFaviconURL());
 
   // Reset the history_handler to verify whether new icon is requested from
   // history.
@@ -1067,16 +1070,15 @@ TEST_F(FaviconHandlerTest, MultipleFavicons) {
                                 kSourceIconURLs + arraysize(kSizes1));
   DownloadTillDoneIgnoringHistory(&handler1, kPageURL, urls1, kSizes1);
 
-  content::FaviconStatus favicon_status1(handler1.GetEntry()->GetFavicon());
   EXPECT_EQ(0u, handler1.image_urls().size());
-  EXPECT_TRUE(favicon_status1.valid);
-  EXPECT_FALSE(favicon_status1.image.IsEmpty());
-  EXPECT_EQ(gfx::kFaviconSize, favicon_status1.image.Width());
+  EXPECT_TRUE(driver1.GetActiveFaviconValidity());
+  EXPECT_FALSE(driver1.GetActiveFaviconImage().IsEmpty());
+  EXPECT_EQ(gfx::kFaviconSize, driver1.GetActiveFaviconImage().Width());
 
   size_t expected_index = 2u;
   EXPECT_EQ(32, kSizes1[expected_index]);
   EXPECT_EQ(kSourceIconURLs[expected_index].icon_url,
-            handler1.GetEntry()->GetFavicon().url);
+            driver1.GetActiveFaviconURL());
 
   // 2) Test that if there are several single resolution favicons to choose
   // from, the exact match is preferred even if it results in upsampling.
@@ -1088,11 +1090,11 @@ TEST_F(FaviconHandlerTest, MultipleFavicons) {
   std::vector<FaviconURL> urls2(kSourceIconURLs,
                                 kSourceIconURLs + arraysize(kSizes2));
   DownloadTillDoneIgnoringHistory(&handler2, kPageURL, urls2, kSizes2);
-  EXPECT_TRUE(handler2.GetEntry()->GetFavicon().valid);
+  EXPECT_TRUE(driver2.GetActiveFaviconValidity());
   expected_index = 0u;
   EXPECT_EQ(16, kSizes2[expected_index]);
   EXPECT_EQ(kSourceIconURLs[expected_index].icon_url,
-            handler2.GetEntry()->GetFavicon().url);
+            driver2.GetActiveFaviconURL());
 
   // 3) Test that favicons which need to be upsampled a little or downsampled
   // a little are preferred over huge favicons.
@@ -1104,11 +1106,11 @@ TEST_F(FaviconHandlerTest, MultipleFavicons) {
   std::vector<FaviconURL> urls3(kSourceIconURLs,
                                 kSourceIconURLs + arraysize(kSizes3));
   DownloadTillDoneIgnoringHistory(&handler3, kPageURL, urls3, kSizes3);
-  EXPECT_TRUE(handler3.GetEntry()->GetFavicon().valid);
+  EXPECT_TRUE(driver3.GetActiveFaviconValidity());
   expected_index = 1u;
   EXPECT_EQ(48, kSizes3[expected_index]);
   EXPECT_EQ(kSourceIconURLs[expected_index].icon_url,
-            handler3.GetEntry()->GetFavicon().url);
+            driver3.GetActiveFaviconURL());
 
   TestFaviconDriver driver4;
   TestFaviconHandler handler4(
@@ -1118,11 +1120,11 @@ TEST_F(FaviconHandlerTest, MultipleFavicons) {
   std::vector<FaviconURL> urls4(kSourceIconURLs,
                                 kSourceIconURLs + arraysize(kSizes4));
   DownloadTillDoneIgnoringHistory(&handler4, kPageURL, urls4, kSizes4);
-  EXPECT_TRUE(handler4.GetEntry()->GetFavicon().valid);
+  EXPECT_TRUE(driver4.GetActiveFaviconValidity());
   expected_index = 0u;
   EXPECT_EQ(17, kSizes4[expected_index]);
   EXPECT_EQ(kSourceIconURLs[expected_index].icon_url,
-            handler4.GetEntry()->GetFavicon().url);
+            driver4.GetActiveFaviconURL());
 }
 
 #endif

chrome/browser/favicon/favicon_tab_helper.cc

@@ -135,10 +135,6 @@ void FaviconTabHelper::SaveFavicon() {
       entry->GetURL(), favicon.url, favicon_base::FAVICON, favicon.image);
 }
 
-NavigationEntry* FaviconTabHelper::GetActiveEntry() {
-  return web_contents()->GetController().GetActiveEntry();
-}
-
 int FaviconTabHelper::StartDownload(const GURL& url, int max_image_size) {
   FaviconService* favicon_service = FaviconServiceFactory::GetForProfile(
       profile_->GetOriginalProfile(), Profile::IMPLICIT_ACCESS);
@@ -167,6 +163,42 @@ bool FaviconTabHelper::IsOffTheRecord() {
   return web_contents()->GetBrowserContext()->IsOffTheRecord();
 }
 
+const gfx::Image FaviconTabHelper::GetActiveFaviconImage() {
+  return GetFaviconStatus().image;
+}
+
+const GURL FaviconTabHelper::GetActiveFaviconURL() {
+  return GetFaviconStatus().url;
+}
+
+bool FaviconTabHelper::GetActiveFaviconValidity() {
+  return GetFaviconStatus().valid;
+}
+
+const GURL FaviconTabHelper::GetActiveURL() {
+  NavigationEntry* entry = web_contents()->GetController().GetActiveEntry();
+  if (!entry || entry->GetURL().is_empty())
+    return GURL();
+  return entry->GetURL();
+}
+
+void FaviconTabHelper::SetActiveFaviconImage(gfx::Image image) {
+  GetFaviconStatus().image = image;
+}
+
+void FaviconTabHelper::SetActiveFaviconURL(GURL url) {
+  GetFaviconStatus().url = url;
+}
+
+void FaviconTabHelper::SetActiveFaviconValidity(bool validity) {
+  GetFaviconStatus().valid = validity;
+}
+
+content::FaviconStatus& FaviconTabHelper::GetFaviconStatus() {
+  DCHECK(web_contents()->GetController().GetActiveEntry());
+  return web_contents()->GetController().GetActiveEntry()->GetFavicon();
+}
+
 void FaviconTabHelper::DidStartNavigationToPendingEntry(
     const GURL& url,
     NavigationController::ReloadType reload_type) {

chrome/browser/favicon/favicon_tab_helper.h

@@ -19,6 +19,10 @@ namespace gfx {
 class Image;
 }
 
+namespace content {
+struct FaviconStatus;
+}
+
 class GURL;
 class FaviconHandler;
 class Profile;
@@ -68,10 +72,16 @@ class FaviconTabHelper : public content::WebContentsObserver,
   void SaveFavicon();
 
   // FaviconDriver methods.
-  virtual content::NavigationEntry* GetActiveEntry() OVERRIDE;
   virtual int StartDownload(const GURL& url, int max_bitmap_size) OVERRIDE;
   virtual void NotifyFaviconUpdated(bool icon_url_changed) OVERRIDE;
   virtual bool IsOffTheRecord() OVERRIDE;
+  virtual const gfx::Image GetActiveFaviconImage() OVERRIDE;
+  virtual const GURL GetActiveFaviconURL() OVERRIDE;
+  virtual bool GetActiveFaviconValidity() OVERRIDE;
+  virtual const GURL GetActiveURL() OVERRIDE;
+  virtual void SetActiveFaviconImage(gfx::Image image) OVERRIDE;
+  virtual void SetActiveFaviconURL(GURL url) OVERRIDE;
+  virtual void SetActiveFaviconValidity(bool validity) OVERRIDE;
 
   // Favicon download callback.
   void DidDownloadFavicon(
@@ -97,6 +107,9 @@ class FaviconTabHelper : public content::WebContentsObserver,
       const content::LoadCommittedDetails& details,
       const content::FrameNavigateParams& params) OVERRIDE;
 
+  // Helper method that returns the active navigation entry's favicon.
+  content::FaviconStatus& GetFaviconStatus();
+
   Profile* profile_;
 
   std::vector<content::FaviconURL> favicon_urls_;

components/favicon/core/favicon_driver.h

@@ -7,9 +7,8 @@
 
 class GURL;
 
-namespace content {
-// TODO(jif): Abstract the NavigationEntry (crbug.com/359598).
-class NavigationEntry;
+namespace gfx {
+class Image;
 }
 
 // Interface that allows Favicon core code to interact with its driver (i.e.,
@@ -17,9 +16,6 @@ class NavigationEntry;
 // implementation must be provided by the driver.
 class FaviconDriver {
  public:
-  // Returns the current NavigationEntry.
-  // TODO(jif): Abstract the NavigationEntry (crbug.com/359598).
-  virtual content::NavigationEntry* GetActiveEntry() = 0;
 
   // Starts the download for the given favicon. When finished, the driver
   // will call OnDidDownloadFavicon() with the results.
@@ -38,6 +34,33 @@ class FaviconDriver {
 
   // Returns whether the user is operating in an off-the-record context.
   virtual bool IsOffTheRecord() = 0;
+
+  // Returns the bitmap of the current page's favicon. Requires GetActiveURL()
+  // to be valid.
+  virtual const gfx::Image GetActiveFaviconImage() = 0;
+
+  // Returns the URL of the current page's favicon. Requires GetActiveURL() to
+  // be valid.
+  virtual const GURL GetActiveFaviconURL() = 0;
+
+  // Returns whether the page's favicon is valid (returns false if the default
+  // favicon is being used). Requires GetActiveURL() to be valid.
+  virtual bool GetActiveFaviconValidity() = 0;
+
+  // Returns the URL of the current page, if any. Returns an invalid
+  // URL otherwise.
+  virtual const GURL GetActiveURL() = 0;
+
+  // Sets the bitmap of the current page's favicon. Requires GetActiveURL() to
+  // be valid.
+  virtual void SetActiveFaviconImage(gfx::Image image) = 0;
+
+  // Sets the URL of the favicon's bitmap. Requires GetActiveURL() to be valid.
+  virtual void SetActiveFaviconURL(GURL url) = 0;
+
+  // Sets whether the page's favicon is valid (if false, the default favicon is
+  // being used). Requires GetActiveURL() to be valid.
+  virtual void SetActiveFaviconValidity(bool validity) = 0;
 };
 
 #endif  // COMPONENTS_FAVICON_CORE_FAVICON_DRIVER_H_