Add support to process favicons from Web Manifests

components/favicon/content/content_favicon_driver.cc

Index: components/favicon/content/content_favicon_driver.cc
diff --git a/components/favicon/content/content_favicon_driver.cc b/components/favicon/content/content_favicon_driver.cc
index e5935ffcc381da3ab051b5bf3ac46f35ad66a9fd..fe8627f12f1bf70a0cb1a828938c85ade384e327 100644
--- a/components/favicon/content/content_favicon_driver.cc
+++ b/components/favicon/content/content_favicon_driver.cc
@@ -17,11 +17,26 @@
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/common/favicon_url.h"
+#include "content/public/common/manifest.h"
 #include "ui/gfx/image/image.h"
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(favicon::ContentFaviconDriver);
 
 namespace favicon {
+namespace {
+
+void ExtractManifestIcons(
+    ContentFaviconDriver::ManifestDownloadCallback callback,
+    const GURL& manifest_url,
+    const content::Manifest& manifest) {
+  std::vector<FaviconURL> candidates;
+  for (const content::Manifest::Icon& icon : manifest.icons) {
+    candidates.emplace_back(icon.src, favicon_base::FAVICON, icon.sizes);
+  }
+  callback.Run(candidates);
+}
+
+}  // namespace

[pkotwicz, 2017/05/12 06:13:28]

Nit: "namespace" -> "anonymous namespace"

[mastiz, 2017/05/12 13:31:32]

Code search (//\ namespace$) says otherwise (21K vs 1K), and besides the term
anonymous is not accurate, should be unnamed.

[pkotwicz, 2017/05/12 15:37:41]

Fair enough

 
 // static
 void ContentFaviconDriver::CreateForWebContents(
@@ -118,6 +133,11 @@ int ContentFaviconDriver::DownloadImage(const GURL& url,
                                        callback);
 }
 
+void ContentFaviconDriver::DownloadManifest(const GURL& url,
+                                            ManifestDownloadCallback callback) {
+  web_contents()->GetManifest(base::Bind(&ExtractManifestIcons, callback));
+}
+
 bool ContentFaviconDriver::IsOffTheRecord() {
   DCHECK(web_contents());
   return web_contents()->GetBrowserContext()->IsOffTheRecord();
@@ -156,8 +176,31 @@ void ContentFaviconDriver::DidUpdateFaviconURL(
     return;
 
   favicon_urls_ = candidates;
+
   OnUpdateCandidates(entry->GetURL(),
-                     FaviconURLsFromContentFaviconURLs(candidates));
+                     FaviconURLsFromContentFaviconURLs(candidates),
+                     manifest_url_);
+}
+
+void ContentFaviconDriver::DidUpdateWebManifestURL(
+    const base::Optional<GURL>& manifest_url) {
+  // Ignore the update if there is no last committed navigation entry. This can
+  // occur when loading an initially blank page.
+  content::NavigationEntry* entry =
+      web_contents()->GetController().GetLastCommittedEntry();
+  if (!entry)
+    return;
+
+  manifest_url_ = manifest_url;
+
+  // On regular page loads, DidUpdateManifestURL() is guaranteed to be called
+  // before DidUpdateFaviconURL(). However, a page can update the favicons via
+  // javascript.
+  if (favicon_urls_.has_value()) {
+    OnUpdateCandidates(entry->GetURL(),
+                       FaviconURLsFromContentFaviconURLs(*favicon_urls_),
+                       manifest_url);
+  }
 }
 
 void ContentFaviconDriver::DidStartNavigation(
@@ -165,6 +208,9 @@ void ContentFaviconDriver::DidStartNavigation(
   if (!navigation_handle->IsInMainFrame())
     return;
 
+  favicon_urls_.reset();
+  manifest_url_.reset();

[pkotwicz, 2017/05/12 06:13:28]

Can this code be in DidFinishNavigation() instead?

I am wondering whether we can move all of the code from DidStartNavigation() to
DidFinishNavigation() (in a separate CL). The reason for the split is that
originally only ContentFaviconDriver::DidStartNavigation() had access to the
reload type. See ContentFaviconDriver::DidStartNavigationToPendingEntry() in
https://chromium.googlesource.com/chromium/src/+blame/0df1d3a01bed974dfa51c33...

[mastiz, 2017/05/12 13:31:32]

I assume a navigation can be interrupted and hence no DidFinishNavigation called
in that case. Hence, we need to reset this in DidStartNavigation.

[pkotwicz, 2017/05/12 15:37:41]

My understanding is that DidFinishNavigation() should preceed calls to
DidUpdateFaviconURL() and DidUpdateWebManifestURL()

DidFinishNavigation() is called really early in the navigation. I think that it
is called when the renderer gets the initial data for the page (prior to the web
page having finished parsing and prior to window.onload() being called)

[mastiz, 2017/05/15 14:06:59]

The question is whether it's guaranteed to be called. I think the code is more
robust by handling the reset during start, and I don't see an immediate upside.

+
   content::ReloadType reload_type = navigation_handle->GetReloadType();
   if (reload_type == content::ReloadType::NONE || IsOffTheRecord())
     return;
@@ -183,8 +229,6 @@ void ContentFaviconDriver::DidFinishNavigation(
     return;
   }
 
-  favicon_urls_.clear();
-
   // Wait till the user navigates to a new URL to start checking the cache
   // again. The cache may be ignored for non-reload navigations (e.g.
   // history.replace() in-page navigation). This is allowed to increase the