Add support to process favicons from Web Manifests

components/favicon/content/content_favicon_driver.h

Index: components/favicon/content/content_favicon_driver.h
diff --git a/components/favicon/content/content_favicon_driver.h b/components/favicon/content/content_favicon_driver.h
index 54a68884d81a94369bf52e3f7905004029f6fa29..4231372695f41c769eca8990edd395172f449a4f 100644
--- a/components/favicon/content/content_favicon_driver.h
+++ b/components/favicon/content/content_favicon_driver.h
@@ -6,6 +6,7 @@
 #define COMPONENTS_FAVICON_CONTENT_CONTENT_FAVICON_DRIVER_H_
 
 #include "base/macros.h"
+#include "base/optional.h"
 #include "components/favicon/core/favicon_driver_impl.h"
 #include "content/public/browser/reload_type.h"
 #include "content/public/browser/web_contents_observer.h"
@@ -36,7 +37,7 @@ class ContentFaviconDriver
 
   // Returns the current tab's favicon URLs. If this is empty,
   // DidUpdateFaviconURL has not yet been called for the current navigation.
-  const std::vector<content::FaviconURL>& favicon_urls() const {
+  const base::Optional<std::vector<content::FaviconURL>>& favicon_urls() const {
     return favicon_urls_;
   }

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

Can this function return a std::vector<content::FaviconURL>() ?

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

By reintroducing the ambiguity between not having received favicons and empty
favicon list? You'll see that the comment above is wrong and some code
(extensions) in fact seem to rely on that fact. I prefer an API without
ambiguities, but if you're feeling strongly, I'll revert.

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

The comment is correct for http page URLs because a default URL is passed if the
page has no <link rel="icon"> tags. I see that the comment is incorrect for non
http page URLs

Making this function return a base::Optional is useless given that the
base::Optional is stripped in FaviconDownloader::GetFaviconURLsFromWebContents()

[mastiz, 2017/05/15 14:07:00]

Reverted. It however returns by value, since returning a reference requires some
boilerplate (e.g. const member field with empty vector) due to base::optional. 

 
@@ -62,6 +63,8 @@ class ContentFaviconDriver
   int DownloadImage(const GURL& url,
                     int max_image_size,
                     ImageDownloadCallback callback) override;
+  void DownloadManifest(const GURL& url,
+                        ManifestDownloadCallback callback) override;
   bool IsOffTheRecord() override;
   void OnFaviconUpdated(const GURL& page_url,
                         FaviconDriverObserver::NotificationIconType icon_type,
@@ -72,13 +75,21 @@ class ContentFaviconDriver
   // content::WebContentsObserver implementation.
   void DidUpdateFaviconURL(
       const std::vector<content::FaviconURL>& candidates) override;
+  void DidUpdateWebManifestURL(
+      const base::Optional<GURL>& manifest_url) override;
   void DidStartNavigation(
       content::NavigationHandle* navigation_handle) override;
   void DidFinishNavigation(
       content::NavigationHandle* navigation_handle) override;
 
   GURL bypass_cache_page_url_;
-  std::vector<content::FaviconURL> favicon_urls_;
+  // nullopt until the actual list is reported via DidUpdateFaviconURL().
+  base::Optional<std::vector<content::FaviconURL>> favicon_urls_;
+  // Whether DidUpdateManifestURL() was called at least once after

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

I am probably missing something. Does DidUpdateWebManifestURL() get called if
the page has no Web Manifest?

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

I removed the comment. Wrt to your question, in general it won't, but it *could*
(via javascript). Hence, I think the comment was accurate (wrt to no
guarantees), but I can see how it is misleading so removed.

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

Thanks

+  // DidStartNavigation(). Note that this doesn't imply an actual manifest URL,
+  // i.e. the page might not have a web manifest.
+  bool received_manifest_url_;

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

|received_manifest_url_| seems unused?

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

Removed, thanks.

+  base::Optional<GURL> manifest_url_;
 
   DISALLOW_COPY_AND_ASSIGN(ContentFaviconDriver);
 };