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_;
   }
 
   // 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_;

[pkotwicz, 2017/05/11 06:12:57]

Can this be a normal std::vector? I think that we treat an empty vector just as
if ContentFaviconDriver::DidUpdateFaviconURL() has not been called

[mastiz, 2017/05/11 08:45:18]

The condition in content_favicon_driver.cc:199 needs to distinguish the two
cases. When a web manifest URL is received, you need to know whether favicons
(including an empty list) have already been received (e.g. manifest URL updated
via Javascript for a page that doesn't contain linked favicons in the HTML).

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

I see. Thank you for pointing this out

+  // Whether DidUpdateManifestURL() was called at least once after
+  // 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_;
+  base::Optional<GURL> manifest_url_;

[pkotwicz, 2017/05/11 06:12:57]

Can this just be a GURL? I think that we treat an empty |manifest_url_| just as
if DidUpdateWebManifestURL() has not been called

[mastiz, 2017/05/11 08:45:18]

This is feasible, but note that WebContents' API exposes an optional (I've
pointed out your preference during code review but the suggestion was not
followed). So the question is: do you prefer consistency with the WebContents
API or not? I'm fine either way.

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

In my opinion being consistent with the WebContents API is not very important.
Passing a raw GURL makes things simpler I think

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

Done.

 
   DISALLOW_COPY_AND_ASSIGN(ContentFaviconDriver);
 };
 
 }  // namespace favicon
 
 #endif  // COMPONENTS_FAVICON_CONTENT_CONTENT_FAVICON_DRIVER_H_