Remove useless FaviconHandler::PageChangedSinceFaviconWasRequested()

Remove FaviconHandler::PageChangedSinceFaviconWasRequested() since both
- FaviconHandler::url_ changes (FaviconHandler::FetchFavicon())
- The NavigationController's last committed URL changes
  (NavigationControllerImpl::RendererDidNavigate())
only as a result of NavigatorImpl::DidNavigate()

BUG=517089
TEST=None

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

Cr-Commit-Position: refs/heads/master@{#351113}

components/favicon/content/content_favicon_driver.cc

@@ -161,11 +161,14 @@ void ContentFaviconDriver::DidUpdateFaviconURL(
 
   // Ignore the update if there is no last committed navigation entry. This can
   // occur when loading an initially blank page.
-  if (!web_contents()->GetController().GetLastCommittedEntry())
+  content::NavigationEntry* entry =
+      web_contents()->GetController().GetLastCommittedEntry();
+  if (!entry)
     return;
 
   favicon_urls_ = candidates;
-  OnUpdateFaviconURL(FaviconURLsFromContentFaviconURLs(candidates));
+  OnUpdateFaviconURL(entry->GetURL(),
+                     FaviconURLsFromContentFaviconURLs(candidates));
 }
 
 void ContentFaviconDriver::DidStartNavigationToPendingEntry(

components/favicon/core/favicon_driver.h

@@ -62,15 +62,14 @@ class FaviconDriver {
   virtual GURL GetActiveURL() = 0;
 
   // Returns whether the page's favicon is valid (returns false if the default
-  // favicon is being used). Requires GetActiveURL() to be valid.
+  // favicon is being used).
   virtual bool GetActiveFaviconValidity() = 0;
 
   // Sets whether the page's favicon is valid (if false, the default favicon is
   // being used).
   virtual void SetActiveFaviconValidity(bool valid) = 0;
 
-  // Returns the URL of the current page's favicon. Requires GetActiveURL() to
-  // be valid.
+  // Returns the URL of the current page's favicon.
   virtual GURL GetActiveFaviconURL() = 0;
 
   // Sets the URL of the favicon's bitmap.
@@ -83,8 +82,9 @@ class FaviconDriver {
   // necessarily 16x16. |icon_url| is the url the image is from. If
   // |is_active_favicon| is true the image corresponds to the favicon
   // (possibly empty) of the page.
-  virtual void OnFaviconAvailable(const gfx::Image& image,
+  virtual void OnFaviconAvailable(const GURL& page_url,
                                   const GURL& icon_url,
+                                  const gfx::Image& image,
                                   bool is_active_favicon) = 0;
 
   // Returns whether the driver is waiting for a download to complete or for

components/favicon/core/favicon_driver_impl.cc

@@ -98,9 +98,17 @@ bool FaviconDriverImpl::IsBookmarked(const GURL& url) {
   return bookmark_model_ && bookmark_model_->IsBookmarked(url);
 }
 
-void FaviconDriverImpl::OnFaviconAvailable(const gfx::Image& image,
+void FaviconDriverImpl::OnFaviconAvailable(const GURL& page_url,
                                            const GURL& icon_url,
+                                           const gfx::Image& image,
                                            bool is_active_favicon) {
+  // Check whether the active URL has changed since FetchFavicon() was called.
+  // On iOS only, the active URL can change between calls to FetchFavicon().
+  // For instance, FetchFavicon() is not synchronously called when the active
+  // URL changes as a result of CRWSessionController::goToEntry().
+  if (page_url != GetActiveURL())
+    return;
+
   if (is_active_favicon) {
     bool icon_url_changed = GetActiveFaviconURL() != icon_url;
     // No matter what happens, we need to mark the favicon as being set.
@@ -141,13 +149,14 @@ void FaviconDriverImpl::SetFaviconOutOfDateForPage(const GURL& url,
 }
 
 void FaviconDriverImpl::OnUpdateFaviconURL(
+    const GURL& page_url,
     const std::vector<FaviconURL>& candidates) {
   DCHECK(!candidates.empty());
-  favicon_handler_->OnUpdateFaviconURL(candidates);
+  favicon_handler_->OnUpdateFaviconURL(page_url, candidates);
   if (touch_icon_handler_.get())
-    touch_icon_handler_->OnUpdateFaviconURL(candidates);
+    touch_icon_handler_->OnUpdateFaviconURL(page_url, candidates);
   if (large_icon_handler_.get())
-    large_icon_handler_->OnUpdateFaviconURL(candidates);
+    large_icon_handler_->OnUpdateFaviconURL(page_url, candidates);
 }
 
 }  // namespace favicon

components/favicon/core/favicon_driver_impl.h

@@ -53,8 +53,9 @@ class FaviconDriverImpl : public FaviconDriver {
   // FaviconDriver implementation.
   void FetchFavicon(const GURL& url) override;
   bool IsBookmarked(const GURL& url) override;
-  void OnFaviconAvailable(const gfx::Image& image,
+  void OnFaviconAvailable(const GURL& page_url,
                           const GURL& icon_url,
+                          const gfx::Image& image,
                           bool is_active_favicon) override;
   bool HasPendingTasksForTest() override;
 
@@ -73,7 +74,8 @@ class FaviconDriverImpl : public FaviconDriver {
   void SetFaviconOutOfDateForPage(const GURL& url, bool force_reload);
 
   // Broadcasts new favicon URL candidates to FaviconHandlers.
-  void OnUpdateFaviconURL(const std::vector<FaviconURL>& candidates);
+  void OnUpdateFaviconURL(const GURL& page_url,
+                          const std::vector<FaviconURL>& candidates);
 
  protected:
   history::HistoryService* history_service() { return history_service_; }

components/favicon/core/favicon_handler.h

@@ -94,7 +94,8 @@ class FaviconHandler {
 
   // Message Handler.  Must be public, because also called from
   // PrerenderContents. Collects the |image_urls| list.
-  void OnUpdateFaviconURL(const std::vector<favicon::FaviconURL>& candidates);
+  void OnUpdateFaviconURL(const GURL& page_url,
+                          const std::vector<favicon::FaviconURL>& candidates);
 
   // Processes the current image_urls_ entry, requesting the image from the
   // history / download service.
@@ -153,7 +154,7 @@ class FaviconHandler {
                                   const gfx::Image& image);
 
   // Returns true if the favicon should be saved.
-  virtual bool ShouldSaveFavicon(const GURL& url);
+  virtual bool ShouldSaveFavicon();
 
  private:
   // For testing:
@@ -164,11 +165,8 @@ class FaviconHandler {
     DownloadRequest();
     ~DownloadRequest();
 
-    DownloadRequest(const GURL& url,
-                    const GURL& image_url,
-                    favicon_base::IconType icon_type);
+    DownloadRequest(const GURL& image_url, favicon_base::IconType icon_type);
 
-    GURL url;
     GURL image_url;
     favicon_base::IconType icon_type;
   };
@@ -178,13 +176,11 @@ class FaviconHandler {
     FaviconCandidate();
     ~FaviconCandidate();
 
-    FaviconCandidate(const GURL& url,
-                     const GURL& image_url,
+    FaviconCandidate(const GURL& image_url,
                      const gfx::Image& image,
                      float score,
                      favicon_base::IconType icon_type);
 
-    GURL url;
     GURL image_url;
     gfx::Image image;
     float score;
@@ -202,8 +198,7 @@ class FaviconHandler {
   // If the favicon has expired, asks the renderer to download the favicon.
   // Otherwise asks history to update the mapping between page url and icon
   // url with a callback to OnFaviconData when done.
-  void DownloadFaviconOrAskFaviconService(const GURL& page_url,
-                                          const GURL& icon_url,
+  void DownloadFaviconOrAskFaviconService(const GURL& icon_url,
                                           favicon_base::IconType icon_type);
 
   // See description above class for details.
@@ -212,20 +207,17 @@ class FaviconHandler {
 
   // Schedules a download for the specified entry. This adds the request to
   // download_requests_.
-  void ScheduleDownload(const GURL& url,
-                        const GURL& image_url,
+  void ScheduleDownload(const GURL& image_url,
                         favicon_base::IconType icon_type);
 
   // Updates |favicon_candidate_| and returns true if it is an exact match.
-  bool UpdateFaviconCandidate(const GURL& url,
-                              const GURL& image_url,
+  bool UpdateFaviconCandidate(const GURL& image_url,
                               const gfx::Image& image,
                               float score,
                               favicon_base::IconType icon_type);
 
   // Sets the image data for the favicon.
-  void SetFavicon(const GURL& url,
-                  const GURL& icon_url,
+  void SetFavicon(const GURL& icon_url,
                   const gfx::Image& image,
                   favicon_base::IconType icon_type);
 
@@ -242,9 +234,6 @@ class FaviconHandler {
     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 {

components/favicon/core/favicon_handler_unittest.cc

@@ -247,8 +247,9 @@ class TestFaviconDriver : public FaviconDriver {
     image_ = image;
   }
 
-  void OnFaviconAvailable(const gfx::Image& image,
+  void OnFaviconAvailable(const GURL& page_url,
                           const GURL& icon_url,
+                          const gfx::Image& image,
                           bool update_active_favicon) override {
     ++num_favicon_available_;
     available_image_ = image;
@@ -398,7 +399,7 @@ class TestFaviconHandler : public FaviconHandler {
         page_url, icon_url, icon_type, bitmap_data, image.Size()));
   }
 
-  bool ShouldSaveFavicon(const GURL& url) override { return true; }
+  bool ShouldSaveFavicon() override { return true; }
 
   GURL page_url_;
 
@@ -502,7 +503,7 @@ class FaviconHandlerTest : public testing::Test {
     favicon_handler->FetchFavicon(page_url);
     favicon_handler->history_handler()->InvokeCallback();
 
-    favicon_handler->OnUpdateFaviconURL(candidate_icons);
+    favicon_handler->OnUpdateFaviconURL(page_url, candidate_icons);
   }
 
   void SetUp() override {
@@ -551,7 +552,7 @@ TEST_F(FaviconHandlerTest, GetFaviconFromHistory) {
   std::vector<FaviconURL> urls;
   urls.push_back(
       FaviconURL(icon_url, favicon_base::FAVICON, std::vector<gfx::Size>()));
-  helper.OnUpdateFaviconURL(urls);
+  helper.OnUpdateFaviconURL(page_url, urls);
 
   // Verify FaviconHandler status
   EXPECT_EQ(1U, helper.urls().size());
@@ -593,7 +594,7 @@ TEST_F(FaviconHandlerTest, DownloadFavicon) {
   std::vector<FaviconURL> urls;
   urls.push_back(
       FaviconURL(icon_url, favicon_base::FAVICON, std::vector<gfx::Size>()));
-  helper.OnUpdateFaviconURL(urls);
+  helper.OnUpdateFaviconURL(page_url, urls);
 
   // Verify FaviconHandler status
   EXPECT_EQ(1U, helper.urls().size());
@@ -662,7 +663,7 @@ TEST_F(FaviconHandlerTest, UpdateAndDownloadFavicon) {
   std::vector<FaviconURL> urls;
   urls.push_back(FaviconURL(
       new_icon_url, favicon_base::FAVICON, std::vector<gfx::Size>()));
-  helper.OnUpdateFaviconURL(urls);
+  helper.OnUpdateFaviconURL(page_url, urls);
 
   // Verify FaviconHandler status.
   EXPECT_EQ(1U, helper.urls().size());
@@ -749,7 +750,7 @@ TEST_F(FaviconHandlerTest, FaviconInHistoryInvalid) {
   std::vector<FaviconURL> urls;
   urls.push_back(
       FaviconURL(icon_url, favicon_base::FAVICON, std::vector<gfx::Size>()));
-  helper.OnUpdateFaviconURL(urls);
+  helper.OnUpdateFaviconURL(page_url, urls);
 
   // A download for the favicon should be requested, and we should not do
   // another history request.
@@ -810,7 +811,7 @@ TEST_F(FaviconHandlerTest, UpdateFavicon) {
   std::vector<FaviconURL> urls;
   urls.push_back(FaviconURL(
       new_icon_url, favicon_base::FAVICON, std::vector<gfx::Size>()));
-  helper.OnUpdateFaviconURL(urls);
+  helper.OnUpdateFaviconURL(page_url, urls);
 
   // Verify FaviconHandler status.
   EXPECT_EQ(1U, helper.urls().size());
@@ -879,7 +880,7 @@ TEST_F(FaviconHandlerTest, Download2ndFaviconURLCandidate) {
       new_icon_url, favicon_base::TOUCH_ICON, std::vector<gfx::Size>()));
   urls.push_back(FaviconURL(
       new_icon_url, favicon_base::FAVICON, std::vector<gfx::Size>()));
-  helper.OnUpdateFaviconURL(urls);
+  helper.OnUpdateFaviconURL(page_url, urls);
 
   // Verify FaviconHandler status.
   EXPECT_EQ(2U, helper.urls().size());
@@ -990,7 +991,7 @@ TEST_F(FaviconHandlerTest, UpdateDuringDownloading) {
       new_icon_url, favicon_base::TOUCH_ICON, std::vector<gfx::Size>()));
   urls.push_back(FaviconURL(
       new_icon_url, favicon_base::FAVICON, std::vector<gfx::Size>()));
-  helper.OnUpdateFaviconURL(urls);
+  helper.OnUpdateFaviconURL(page_url, urls);
 
   // Verify FaviconHandler status.
   EXPECT_EQ(2U, helper.urls().size());
@@ -1024,7 +1025,7 @@ TEST_F(FaviconHandlerTest, UpdateDuringDownloading) {
   std::vector<FaviconURL> latest_urls;
   latest_urls.push_back(FaviconURL(
       latest_icon_url, favicon_base::TOUCH_ICON, std::vector<gfx::Size>()));
-  helper.OnUpdateFaviconURL(latest_urls);
+  helper.OnUpdateFaviconURL(page_url, latest_urls);
 
   EXPECT_EQ(1U, helper.urls().size());
   EXPECT_EQ(latest_icon_url, helper.current_candidate()->icon_url);

components/favicon/ios/web_favicon_driver.cc

@@ -32,6 +32,11 @@ void WebFaviconDriver::CreateForWebState(
                                               history_service, bookmark_model));
 }
 
+void WebFaviconDriver::FetchFavicon(const GURL& url) {
+  fetch_favicon_url_ = url;
+  FaviconDriverImpl::FetchFavicon(url);
+}
+
 gfx::Image WebFaviconDriver::GetFavicon() const {
   web::NavigationItem* item =
       web_state()->GetNavigationManager()->GetLastCommittedItem();
@@ -68,7 +73,7 @@ GURL WebFaviconDriver::GetActiveURL() {
 }
 
 bool WebFaviconDriver::GetActiveFaviconValidity() {
-  return GetFaviconStatus().valid;
+  return !ActiveURLChangedSinceFetchFavicon() && GetFaviconStatus().valid;
 }
 
 void WebFaviconDriver::SetActiveFaviconValidity(bool validity) {
@@ -76,7 +81,7 @@ void WebFaviconDriver::SetActiveFaviconValidity(bool validity) {
 }
 
 GURL WebFaviconDriver::GetActiveFaviconURL() {
-  return GetFaviconStatus().url;
+  return ActiveURLChangedSinceFetchFavicon() ? GURL() : GetFaviconStatus().url;
 }
 
 void WebFaviconDriver::SetActiveFaviconURL(const GURL& url) {
@@ -87,8 +92,17 @@ void WebFaviconDriver::SetActiveFaviconImage(const gfx::Image& image) {
   GetFaviconStatus().image = image;
 }
 
+bool WebFaviconDriver::ActiveURLChangedSinceFetchFavicon() {
+  // On iOS the active URL can change in between calls to FetchFavicon(). For
+  // instance, FetchFavicon() is not synchronously called when the active URL
+  // changes as a result of CRWSessionController::goToEntry().
+  // TODO(stuartmorgan): Remove this once iOS always triggers favicon fetches
+  // synchronously after active URL changes.
+  return GetActiveURL() != fetch_favicon_url_;
+}
+
 web::FaviconStatus& WebFaviconDriver::GetFaviconStatus() {
-  DCHECK(web_state()->GetNavigationManager()->GetVisibleItem());
+  DCHECK(!ActiveURLChangedSinceFetchFavicon());
   return web_state()->GetNavigationManager()->GetVisibleItem()->GetFavicon();
 }
 
@@ -106,7 +120,7 @@ WebFaviconDriver::~WebFaviconDriver() {
 void WebFaviconDriver::FaviconUrlUpdated(
     const std::vector<web::FaviconURL>& candidates) {
   DCHECK(!candidates.empty());
-  OnUpdateFaviconURL(FaviconURLsFromWebFaviconURLs(candidates));
+  OnUpdateFaviconURL(GetActiveURL(), FaviconURLsFromWebFaviconURLs(candidates));
 }
 
 }  // namespace favicon

components/favicon/ios/web_favicon_driver.h

@@ -29,6 +29,7 @@ class WebFaviconDriver : public web::WebStateObserver,
                                 bookmarks::BookmarkModel* bookmark_model);
 
   // FaviconDriver implementation.
+  void FetchFavicon(const GURL& url) override;
   gfx::Image GetFavicon() const override;
   bool FaviconIsValid() const override;
   int StartDownload(const GURL& url, int max_bitmap_size) override;
@@ -49,6 +50,9 @@ class WebFaviconDriver : public web::WebStateObserver,
                    bookmarks::BookmarkModel* bookmark_model);
   ~WebFaviconDriver() override;
 
+  // Returns whether the active URL has changed since FetchFavicon() was called.
+  bool ActiveURLChangedSinceFetchFavicon();
+
   // web::WebStateObserver implementation.
   void FaviconUrlUpdated(
       const std::vector<web::FaviconURL>& candidates) override;
@@ -56,6 +60,9 @@ class WebFaviconDriver : public web::WebStateObserver,
   // Returns the active navigation entry's favicon.
   web::FaviconStatus& GetFaviconStatus();
 
+  // The URL passed to FetchFavicon().
+  GURL fetch_favicon_url_;
+
   DISALLOW_COPY_AND_ASSIGN(WebFaviconDriver);
 };