Call WebStateObserver::FaviconUrlUpdated for same-document navigations.

WebStateImpl will cache favicon urls in OnFaviconUrlUpdated and will
call WebStateObserver::FaviconUrlUpdated in OnNavigationFinished for
same document navigations.

Also removed favicon urls caching in WebFaviconDriver introduced in
crrev.com/c/695761. This CL is not exactly a revert of
crrev.com/c/695761, because deprecated NavigationItemCommitted is
replaced with DidFinishNavigation. NavigationItemCommitted is not
called for push/replace state same-document navigaiton, which may
cause other bugs for favicons caching.


Bug: 789581
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I42922ab4d7812380787e350713640efe4d599680
Reviewed-on: https://chromium-review.googlesource.com/797072Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Commit-Queue: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521876}

components/favicon/ios/web_favicon_driver.h

@@ -60,8 +60,6 @@ class WebFaviconDriver : public web::WebStateObserver,
                    history::HistoryService* history_service);
 
   // web::WebStateObserver implementation.
-  void DidStartNavigation(web::WebState* web_state,
-                          web::NavigationContext* navigation_context) override;
   void DidFinishNavigation(web::WebState* web_state,
                            web::NavigationContext* navigation_context) override;
   void FaviconUrlUpdated(
@@ -76,9 +74,6 @@ class WebFaviconDriver : public web::WebStateObserver,
   // Image Fetcher used to fetch favicon.
   image_fetcher::IOSImageDataFetcherWrapper image_fetcher_;
 
-  // Caches the favicon URLs candidates for same-document navigations.
-  std::vector<favicon::FaviconURL> candidates_;
-
   // The WebState this instance is observing. Will be null after
   // WebStateDestroyed has been called.
   web::WebState* web_state_ = nullptr;

components/favicon/ios/web_favicon_driver.mm

@@ -117,13 +117,10 @@ void WebFaviconDriver::OnFaviconUpdated(
   // On iOS, 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 (GetActiveURL() != page_url && !page_url.is_empty()) {
-    return;
-  }
-
   web::NavigationItem* item =
       web_state_->GetNavigationManager()->GetVisibleItem();
-  DCHECK(item);
+  if (!item || item->GetURL() != page_url)
+    return;
 
   web::FaviconStatus& favicon_status = item->GetFavicon();
   favicon_status.valid = true;
@@ -168,32 +165,11 @@ WebFaviconDriver::~WebFaviconDriver() {
   DCHECK(!web_state_);
 }
 
-void WebFaviconDriver::DidStartNavigation(
-    web::WebState* web_state,
-    web::NavigationContext* navigation_context) {
-  DCHECK_EQ(web_state_, web_state);
-  SetFaviconOutOfDateForPage(navigation_context->GetUrl(),
-                             /*force_reload=*/false);
-}
-
 void WebFaviconDriver::DidFinishNavigation(
     web::WebState* web_state,
     web::NavigationContext* navigation_context) {
-  DCHECK_EQ(web_state_, web_state);
-  if (navigation_context->GetError())
-    return;
-
-  // Fetch the favicon for the new URL.
-  FetchFavicon(navigation_context->GetUrl(),
+  FetchFavicon(web_state->GetLastCommittedURL(),
                navigation_context->IsSameDocument());
-
-  if (navigation_context->IsSameDocument()) {
-    if (!candidates_.empty()) {
-      FaviconUrlUpdatedInternal(candidates_);
-    }
-  } else {
-    candidates_.clear();
-  }
 }
 
 void WebFaviconDriver::FaviconUrlUpdated(
@@ -201,8 +177,8 @@ void WebFaviconDriver::FaviconUrlUpdated(
     const std::vector<web::FaviconURL>& candidates) {
   DCHECK_EQ(web_state_, web_state);
   DCHECK(!candidates.empty());
-  candidates_ = FaviconURLsFromWebFaviconURLs(candidates);
-  FaviconUrlUpdatedInternal(candidates_);
+  OnUpdateCandidates(GetActiveURL(), FaviconURLsFromWebFaviconURLs(candidates),
+                     GURL());
 }
 
 void WebFaviconDriver::WebStateDestroyed(web::WebState* web_state) {
@@ -211,9 +187,4 @@ void WebFaviconDriver::WebStateDestroyed(web::WebState* web_state) {
   web_state_ = nullptr;
 }
 
-void WebFaviconDriver::FaviconUrlUpdatedInternal(
-    const std::vector<favicon::FaviconURL>& candidates) {
-  OnUpdateCandidates(GetActiveURL(), candidates, GURL());
-}
-
 }  // namespace favicon

ios/web/web_state/web_state_impl.h

@@ -79,7 +79,9 @@ class WebStateImpl : public WebState, public NavigationManagerDelegate {
   // Notifies the observers that a navigation has started.
   void OnNavigationStarted(web::NavigationContext* context);
 
-  // Notifies the observers that a navigation has finished.
+  // Notifies the observers that a navigation has finished. For same-document
+  // navigations notifies the observers about favicon URLs update using
+  // candidates received in OnFaviconUrlUpdated.
   void OnNavigationFinished(web::NavigationContext* context);
 
   // Called when page title was changed.
@@ -378,6 +380,12 @@ class WebStateImpl : public WebState, public NavigationManagerDelegate {
   // history into WKWebView when web usage is re-enabled.
   base::scoped_nsobject<CRWSessionStorage> cached_session_storage_;
 
+  // Favicons URLs received in OnFaviconUrlUpdated.
+  // WebStateObserver:FaviconUrlUpdated must be called for same-document
+  // navigations, so this cache will be used to avoid running expensive favicon
+  // fetching JavaScript.
+  std::vector<web::FaviconURL> cached_favicon_urls_;
+
   DISALLOW_COPY_AND_ASSIGN(WebStateImpl);
 };
 

ios/web/web_state/web_state_impl.mm

@@ -261,6 +261,7 @@ void WebStateImpl::OnFormActivityRegistered(const FormActivityParams& params) {
 
 void WebStateImpl::OnFaviconUrlUpdated(
     const std::vector<FaviconURL>& candidates) {
+  cached_favicon_urls_ = candidates;
   for (auto& observer : observers_)
     observer.FaviconUrlUpdated(this, candidates);
 }
@@ -736,6 +737,19 @@ void WebStateImpl::OnNavigationStarted(web::NavigationContext* context) {
 void WebStateImpl::OnNavigationFinished(web::NavigationContext* context) {
   for (auto& observer : observers_)
     observer.DidFinishNavigation(this, context);
+
+  // Update cached_favicon_urls_.
+  if (!context->IsSameDocument()) {
+    // Favicons are not valid after document change. Favicon URLs will be
+    // refetched by CRWWebController and passed to OnFaviconUrlUpdated.
+    cached_favicon_urls_.clear();
+  } else if (!cached_favicon_urls_.empty()) {
+    // For same-document navigations favicon urls will not be refetched and
+    // WebStateObserver:FaviconUrlUpdated must use the cached results.
+    for (auto& observer : observers_) {
+      observer.FaviconUrlUpdated(this, cached_favicon_urls_);
+    }
+  }
 }
 
 #pragma mark - NavigationManagerDelegate implementation

ios/web/web_state/web_state_impl_unittest.mm

@@ -18,6 +18,7 @@
 #import "base/test/ios/wait_util.h"
 #import "ios/web/public/java_script_dialog_presenter.h"
 #include "ios/web/public/load_committed_details.h"
+#import "ios/web/public/test/fakes/fake_navigation_context.h"
 #include "ios/web/public/test/fakes/test_browser_state.h"
 #import "ios/web/public/test/fakes/test_web_state_delegate.h"
 #import "ios/web/public/test/fakes/test_web_state_observer.h"
@@ -772,4 +773,53 @@ TEST_F(WebStateImplTest, CreatedWithOpener) {
   EXPECT_TRUE(web_state_with_opener->HasOpener());
 }
 
+// Tests that WebStateObserver::FaviconUrlUpdated is called for same-document
+// navigations.
+TEST_F(WebStateImplTest, FaviconUpdateForSameDocumentNavigations) {
+  auto observer = std::make_unique<TestWebStateObserver>(web_state_.get());
+
+  // No callback if icons has not been fetched yet.
+  FakeNavigationContext context;
+  context.SetIsSameDocument(true);
+  web_state_->OnNavigationFinished(&context);
+  EXPECT_FALSE(observer->update_favicon_url_candidates_info());
+
+  // Callback is called when icons were fetched.
+  observer = base::MakeUnique<TestWebStateObserver>(web_state_.get());
+  web::FaviconURL favicon_url(GURL("https://chromium.test/"),
+                              web::FaviconURL::IconType::kTouchIcon,
+                              {gfx::Size(5, 6)});
+  web_state_->OnFaviconUrlUpdated({favicon_url});
+  EXPECT_TRUE(observer->update_favicon_url_candidates_info());
+
+  // Callback is now called after same-document navigation.
+  observer = std::make_unique<TestWebStateObserver>(web_state_.get());
+  web_state_->OnNavigationFinished(&context);
+  ASSERT_TRUE(observer->update_favicon_url_candidates_info());
+  ASSERT_EQ(1U,
+            observer->update_favicon_url_candidates_info()->candidates.size());
+  const web::FaviconURL& actual_favicon_url =
+      observer->update_favicon_url_candidates_info()->candidates[0];
+  EXPECT_EQ(favicon_url.icon_url, actual_favicon_url.icon_url);
+  EXPECT_EQ(favicon_url.icon_type, actual_favicon_url.icon_type);
+  ASSERT_EQ(favicon_url.icon_sizes.size(),
+            actual_favicon_url.icon_sizes.size());
+  EXPECT_EQ(favicon_url.icon_sizes[0].width(),
+            actual_favicon_url.icon_sizes[0].width());
+  EXPECT_EQ(favicon_url.icon_sizes[0].height(),
+            actual_favicon_url.icon_sizes[0].height());
+
+  // Document change navigation does not call callback.
+  observer = std::make_unique<TestWebStateObserver>(web_state_.get());
+  context.SetIsSameDocument(false);
+  web_state_->OnNavigationFinished(&context);
+  EXPECT_FALSE(observer->update_favicon_url_candidates_info());
+
+  // Previous candidates were invalidated by the document change. No callback
+  // if icons has not been fetched yet.
+  context.SetIsSameDocument(true);
+  web_state_->OnNavigationFinished(&context);
+  EXPECT_FALSE(observer->update_favicon_url_candidates_info());
+}
+
 }  // namespace web