Add support to process favicons from Web Manifests

components/favicon/content/content_favicon_driver.h

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef COMPONENTS_FAVICON_CONTENT_CONTENT_FAVICON_DRIVER_H_
 #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"
 #include "content/public/browser/web_contents_user_data.h"
 #include "url/gurl.h"
 
 namespace content {
 struct FaviconURL;
 class WebContents;
 }
@@ skipping 10 matching lines @@
  public:
   ~ContentFaviconDriver() override;
 
   static void CreateForWebContents(content::WebContents* web_contents,
                                    FaviconService* favicon_service,
                                    history::HistoryService* history_service,
                                    bookmarks::BookmarkModel* bookmark_model);
 
   // 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. 

 
   // Saves the favicon for the last committed navigation entry to the thumbnail
   // database.
   void SaveFavicon();
 
   // FaviconDriver implementation.
   gfx::Image GetFavicon() const override;
   bool FaviconIsValid() const override;
   GURL GetActiveURL() override;
 
  protected:
   ContentFaviconDriver(content::WebContents* web_contents,
                        FaviconService* favicon_service,
                        history::HistoryService* history_service,
                        bookmarks::BookmarkModel* bookmark_model);
 
  private:
   friend class content::WebContentsUserData<ContentFaviconDriver>;
 
   // FaviconHandler::Delegate implementation.
   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,
                         const GURL& icon_url,
                         bool icon_url_changed,
                         const gfx::Image& image) override;
 
   // 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);
 };
 
 }  // namespace favicon
 
 #endif  // COMPONENTS_FAVICON_CONTENT_CONTENT_FAVICON_DRIVER_H_