Web Contents: expose the candidate favicon URLs in WebContents.

This will avoid WebContentsObserver to listen to the observer and
footgun themselves thinking that they will be aware of everything while
they should only expect updates.

WebContentsObserver documentation has been updated accordingly.

ContentFaviconDriver no longer exposes this information and the only
user is now using WebContents directly.

Bug: 1030540
Change-Id: I91ab4c193ef1ccbd3e4568250631029cb711f61c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1959334Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Alexey Baskakov <loyso@chromium.org>
Commit-Queue: Mounir Lamouri <mlamouri@chromium.org>
Cr-Commit-Position: refs/heads/master@{#727166}

chrome/browser/web_applications/components/BUILD.gn

@@ -69,9 +69,9 @@ source_set("components") {
     # //chrome/browser/web_applications/components to a stand-alone
     # //components/web_app_icon_downloader?
     #
-    # There's also //components/favicon, //components/image_fetcher as well as
-    # code split between //content/public/browser/manifest_icon_downloader.h
-    # and //content/browser/manfest/manifest_icon_downloader.cc. Some or all of
+    # There's //components/image_fetcher as well as code split between
+    # //content/public/browser/manifest_icon_downloader.h and
+    # //content/browser/manfest/manifest_icon_downloader.cc. Some or all of
     # those might be similar enough to merge.
     "web_app_icon_downloader.cc",
     "web_app_icon_downloader.h",
@@ -114,7 +114,6 @@ source_set("components") {
     "//chrome/common",
     "//components/content_settings/core/browser",
     "//components/crx_file",
-    "//components/favicon/content",
     "//components/keyed_service/content",
     "//components/pref_registry",
     "//components/services/app_service/public/mojom",

chrome/browser/web_applications/components/web_app_icon_downloader.cc

@@ -8,7 +8,6 @@
 #include "base/metrics/histogram_functions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/web_applications/components/web_app_helpers.h"
-#include "components/favicon/content/content_favicon_driver.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/favicon_url.h"
@@ -44,11 +43,13 @@ void WebAppIconDownloader::Start() {
   FetchIcons(extra_favicon_urls_);
 
   if (need_favicon_urls_) {
-    std::vector<content::FaviconURL> favicon_tab_helper_urls =
+    // The call to `GetFaviconURLsFromWebContents()` is to allow this method to
+    // be mocked by unit tests.
+    const std::vector<content::FaviconURL> favicon_urls =
         GetFaviconURLsFromWebContents();
-    if (!favicon_tab_helper_urls.empty()) {
+    if (!favicon_urls.empty()) {
       need_favicon_urls_ = false;
-      FetchIcons(favicon_tab_helper_urls);
+      FetchIcons(favicon_urls);
     }
   }
 }
@@ -66,14 +67,7 @@ int WebAppIconDownloader::DownloadImage(const GURL& url) {
 
 std::vector<content::FaviconURL>
 WebAppIconDownloader::GetFaviconURLsFromWebContents() {
-  favicon::ContentFaviconDriver* content_favicon_driver =
-      web_contents()
-          ? favicon::ContentFaviconDriver::FromWebContents(web_contents())
-          : nullptr;
-  // If favicon_urls() is empty, we are guaranteed that DidUpdateFaviconURLs has
-  // not yet been called for the current page's navigation.
-  return content_favicon_driver ? content_favicon_driver->favicon_urls()
-                                : std::vector<content::FaviconURL>();
+  return web_contents()->GetFaviconURLs();
 }
 
 void WebAppIconDownloader::FetchIcons(

chrome/browser/web_applications/components/web_app_icon_downloader.h

@@ -65,7 +65,7 @@ class WebAppIconDownloader : public content::WebContentsObserver {
   // This is overridden in testing.
   virtual int DownloadImage(const GURL& url);
 
-  // Queries FaviconDriver for the page's current favicon URLs.
+  // Queries WebContents for the page's current favicon URLs.
   // This is overridden in testing.
   virtual std::vector<content::FaviconURL> GetFaviconURLsFromWebContents();
 

content/browser/web_contents/web_contents_impl.cc

@@ -4362,6 +4362,8 @@ void WebContentsImpl::SetFocusToLocationBar() {
 void WebContentsImpl::DidStartNavigation(NavigationHandle* navigation_handle) {
   TRACE_EVENT1("navigation", "WebContentsImpl::DidStartNavigation",
                "navigation_handle", navigation_handle);
+  favicon_urls_.clear();
+
   for (auto& observer : observers_)
     observer.DidStartNavigation(navigation_handle);
 
@@ -5117,6 +5119,8 @@ void WebContentsImpl::OnUpdateFaviconURL(
   if (!source->IsCurrent())
     return;
 
+  favicon_urls_ = candidates;
+
   for (auto& observer : observers_)
     observer.DidUpdateFaviconURL(candidates);
 }
@@ -6742,6 +6746,10 @@ ukm::SourceId WebContentsImpl::GetLastCommittedSourceId() {
   return last_committed_source_id_;
 }
 
+std::vector<FaviconURL> WebContentsImpl::GetFaviconURLs() {
+  return favicon_urls_;
+}
+
 BrowserPluginEmbedder* WebContentsImpl::GetBrowserPluginEmbedder() const {
   return browser_plugin_embedder_.get();
 }

content/browser/web_contents/web_contents_impl.h

@@ -493,6 +493,7 @@ class CONTENT_EXPORT WebContentsImpl : public WebContents,
   base::UnguessableToken GetAudioGroupId() override;
   bool CompletedFirstVisuallyNonEmptyPaint() override;
   ukm::SourceId GetLastCommittedSourceId() override;
+  std::vector<FaviconURL> GetFaviconURLs() override;
 
 #if defined(OS_ANDROID)
   base::android::ScopedJavaLocalRef<jobject> GetJavaWebContents() override;
@@ -1148,6 +1149,9 @@ class CONTENT_EXPORT WebContentsImpl : public WebContents,
   FRIEND_TEST_ALL_PREFIXES(WebContentsImplTest,
                            ResetJavaScriptDialogOnUserNavigate);
   FRIEND_TEST_ALL_PREFIXES(WebContentsImplTest, ParseDownloadHeaders);
+  FRIEND_TEST_ALL_PREFIXES(WebContentsImplTest, FaviconURLsSet);
+  FRIEND_TEST_ALL_PREFIXES(WebContentsImplTest, FaviconURLsResetWithNavigation);
+  FRIEND_TEST_ALL_PREFIXES(WebContentsImplTest, FaviconURLsUpdateDelay);
   FRIEND_TEST_ALL_PREFIXES(WebContentsImplBrowserTest,
                            NotifyFullscreenAcquired);
   FRIEND_TEST_ALL_PREFIXES(WebContentsImplBrowserTest,
@@ -1957,6 +1961,10 @@ class CONTENT_EXPORT WebContentsImpl : public WebContents,
   // successful navigation in this WebContents.
   bool first_navigation_completed_ = false;
 
+  // Represents the favicon urls candidates from the page.
+  // Empty std::vector until the first update from the renderer.
+  std::vector<FaviconURL> favicon_urls_;
+
   base::WeakPtrFactory<WebContentsImpl> loading_weak_factory_{this};
   base::WeakPtrFactory<WebContentsImpl> weak_factory_{this};
 

content/browser/web_contents/web_contents_impl_unittest.cc

@@ -3614,4 +3614,36 @@ TEST_F(WebContentsImplTest, Bluetooth) {
   EXPECT_FALSE(contents()->IsConnectedToBluetoothDevice());
 }
 
+TEST_F(WebContentsImplTest, FaviconURLsSet) {
+  const FaviconURL kFavicon(GURL("https://example.com/favicon.ico"),
+                            FaviconURL::IconType::kFavicon, {});
+
+  contents()->NavigateAndCommit(GURL("https://example.com"));
+  EXPECT_EQ(0u, contents()->GetFaviconURLs().size());
+
+  contents()->OnUpdateFaviconURL(contents()->GetMainFrame(), {kFavicon});
+  EXPECT_EQ(1u, contents()->GetFaviconURLs().size());
+
+  contents()->OnUpdateFaviconURL(contents()->GetMainFrame(),
+                                 {kFavicon, kFavicon});
+  EXPECT_EQ(2u, contents()->GetFaviconURLs().size());
+
+  contents()->OnUpdateFaviconURL(contents()->GetMainFrame(), {kFavicon});
+  EXPECT_EQ(1u, contents()->GetFaviconURLs().size());
+}
+
+TEST_F(WebContentsImplTest, FaviconURLsResetWithNavigation) {
+  const FaviconURL kFavicon(GURL("https://example.com/favicon.ico"),
+                            FaviconURL::IconType::kFavicon, {});
+
+  contents()->NavigateAndCommit(GURL("https://example.com"));
+  EXPECT_EQ(0u, contents()->GetFaviconURLs().size());
+
+  contents()->OnUpdateFaviconURL(contents()->GetMainFrame(), {kFavicon});
+  EXPECT_EQ(1u, contents()->GetFaviconURLs().size());
+
+  contents()->NavigateAndCommit(GURL("https://example.com/navigation.html"));
+  EXPECT_EQ(0u, contents()->GetFaviconURLs().size());
+}
+
 }  // namespace content

content/public/browser/web_contents.h

@@ -84,6 +84,7 @@ class RenderWidgetHostView;
 class WebContentsDelegate;
 struct CustomContextMenuContext;
 struct DropData;
+struct FaviconURL;
 struct MHTMLGenerationParams;
 
 // WebContents is the core class in content/. A WebContents renders web content
@@ -1036,6 +1037,12 @@ class WebContents : public PageNavigator,
   // The source ID of the last committed navigation.
   virtual ukm::SourceId GetLastCommittedSourceId() = 0;
 
+  // Returns the raw list of favicon candidates as reported to observers via
+  // WebContentsObserver::DidUpdateFaviconURL() since the last navigation start.
+  // Consider using FaviconDriver in components/favicon if possible for more
+  // reliable favicon-related state.
+  virtual std::vector<FaviconURL> GetFaviconURLs() = 0;
+
  private:
   // This interface should only be implemented inside content.
   friend class WebContentsImpl;

content/public/browser/web_contents_observer.h

@@ -443,7 +443,9 @@ class CONTENT_EXPORT WebContentsObserver : public IPC::Listener {
   virtual void UserAgentOverrideSet(const std::string& user_agent) {}
 
   // Invoked when new FaviconURL candidates are received from the renderer
-  // process.
+  // process. If the instance is created after the page is loaded, it is
+  // recommended to call WebContents::GetFaviconURLs() to get the current list
+  // as this callback will not be executed unless there is an update.
   virtual void DidUpdateFaviconURL(const std::vector<FaviconURL>& candidates) {}
 
   // Called when an audio change occurs.