favicons: adds CoreFaviconService

CoreFaviconService contains the methods needed by the
favicon implementation. FaviconService now extends
CoreFaviconService.

BUG=1076463
TEST=covered by tests

Change-Id: Ic352fe1430176843e324f70f21f5448c7dc76d35
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2313098
Commit-Queue: Scott Violet <sky@chromium.org>
Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#791108}

components/favicon/content/content_favicon_driver.cc

@@ -38,7 +38,7 @@ void ExtractManifestIcons(
 // static
 void ContentFaviconDriver::CreateForWebContents(
     content::WebContents* web_contents,
-    FaviconService* favicon_service) {
+    CoreFaviconService* favicon_service) {
   if (FromWebContents(web_contents))
     return;
 
@@ -75,7 +75,7 @@ GURL ContentFaviconDriver::GetActiveURL() {
 }
 
 ContentFaviconDriver::ContentFaviconDriver(content::WebContents* web_contents,
-                                           FaviconService* favicon_service)
+                                           CoreFaviconService* favicon_service)
     : content::WebContentsObserver(web_contents),
       FaviconDriverImpl(favicon_service),
       document_on_load_completed_(false) {}

components/favicon/content/content_favicon_driver.h

@@ -18,6 +18,8 @@
 
 namespace favicon {
 
+class CoreFaviconService;
+
 // ContentFaviconDriver is an implementation of FaviconDriver that listens to
 // WebContents events to start download of favicons and to get informed when the
 // favicon download has completed.
@@ -29,7 +31,7 @@ class ContentFaviconDriver
   ~ContentFaviconDriver() override;
 
   static void CreateForWebContents(content::WebContents* web_contents,
-                                   FaviconService* favicon_service);
+                                   CoreFaviconService* favicon_service);
 
   // Returns the current tab's favicon URLs. If this is empty,
   // DidUpdateFaviconURL has not yet been called for the current navigation.
@@ -44,7 +46,7 @@ class ContentFaviconDriver
 
  protected:
   ContentFaviconDriver(content::WebContents* web_contents,
-                       FaviconService* favicon_service);
+                       CoreFaviconService* favicon_service);
 
  private:
   friend class content::WebContentsUserData<ContentFaviconDriver>;

components/favicon/core/BUILD.gn

@@ -4,6 +4,7 @@
 
 static_library("core") {
   sources = [
+    "core_favicon_service.h",
     "fallback_url_util.cc",
     "fallback_url_util.h",
     "favicon_client.h",

components/favicon/core/core_favicon_service.h

+// Copyright 2020 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_CORE_CORE_FAVICON_SERVICE_H_
+#define COMPONENTS_FAVICON_CORE_CORE_FAVICON_SERVICE_H_
+
+#include "base/callback.h"
+#include "base/containers/flat_set.h"
+#include "base/task/cancelable_task_tracker.h"
+#include "components/favicon_base/favicon_callback.h"
+#include "components/favicon_base/favicon_types.h"
+#include "components/favicon_base/favicon_usage_data.h"
+#include "components/keyed_service/core/keyed_service.h"
+
+class GURL;
+
+namespace favicon {
+
+// CoreFaviconService defines the API needed to persist and restore favicons.
+class CoreFaviconService : public KeyedService {
+ public:
+  CoreFaviconService() = default;
+
+  // Gets the favicons mapped to |page_url| for |icon_types| whose edge sizes
+  // most closely match |desired_size_in_dip|. A value of 0 gets the
+  // largest bitmap for |icon_types|. The returned FaviconBitmapResults will
+  // have at most one result for each entry in |desired_sizes|. If a bitmap is
+  // determined to be the best candidate for multiple |desired_sizes| there
+  // will be fewer results.
+  virtual base::CancelableTaskTracker::TaskId GetFaviconForPageURL(
+      const GURL& page_url,
+      const favicon_base::IconTypeSet& icon_types,
+      int desired_size_in_dip,
+      favicon_base::FaviconResultsCallback callback,
+      base::CancelableTaskTracker* tracker) = 0;
+
+  // Marks all types of favicon for the page as being out of date.
+  virtual void SetFaviconOutOfDateForPage(const GURL& page_url) = 0;
+
+  // Set the favicon for all URLs in |page_urls| for |icon_type| in the
+  // thumbnail database. |icon_url| is the single favicon to map to. Mappings
+  // from page URLs to favicons at different icon URLs will be deleted.
+  // A favicon bitmap is added for each image rep in |image|. Any preexisting
+  // bitmap data for |icon_url| is deleted. It is important that |image|
+  // contains image reps for all of ui::GetSupportedScaleFactors(). Use
+  // MergeFavicon() if it does not.
+  // TODO(pkotwicz): Save unresized favicon bitmaps to the database.
+  // TODO(pkotwicz): Support adding favicons for multiple icon URLs to the
+  // thumbnail database.
+  virtual void SetFavicons(const base::flat_set<GURL>& page_urls,
+                           const GURL& icon_url,
+                           favicon_base::IconType icon_type,
+                           const gfx::Image& image) = 0;
+
+  // Causes each page in |page_urls_to_write| to be associated to the same
+  // icon as the page |page_url_to_read| for icon types matching |icon_types|.
+  // No-op if |page_url_to_read| has no mappings for |icon_types|.
+  virtual void CloneFaviconMappingsForPages(
+      const GURL& page_url_to_read,
+      const favicon_base::IconTypeSet& icon_types,
+      const base::flat_set<GURL>& page_urls_to_write) = 0;
+
+  // The first argument for |callback| is the set of bitmaps for the passed in
+  // URL and icon types whose pixel sizes best match the passed in
+  // |desired_size_in_dip| at the resource scale factors supported by the
+  // current platform (eg MacOS) in addition to 1x. The vector has at most one
+  // result for each of the resource scale factors. There are less entries if a
+  // single/ result is the best bitmap to use for several resource scale
+  // factors.
+  virtual base::CancelableTaskTracker::TaskId GetFavicon(
+      const GURL& icon_url,
+      favicon_base::IconType icon_type,
+      int desired_size_in_dip,
+      favicon_base::FaviconResultsCallback callback,
+      base::CancelableTaskTracker* tracker) = 0;
+
+  // Maps |page_urls| to the favicon at |icon_url| if there is an entry in the
+  // database for |icon_url| and |icon_type|. This occurs when there is a
+  // mapping from a different page URL to |icon_url|. The favicon bitmaps whose
+  // edge sizes most closely match |desired_size_in_dip| from the favicons which
+  // were just mapped to |page_urls| are returned. If |desired_size_in_dip| has
+  // a '0' entry, the largest favicon bitmap is returned.
+  virtual base::CancelableTaskTracker::TaskId UpdateFaviconMappingsAndFetch(
+      const base::flat_set<GURL>& page_urls,
+      const GURL& icon_url,
+      favicon_base::IconType icon_type,
+      int desired_size_in_dip,
+      favicon_base::FaviconResultsCallback callback,
+      base::CancelableTaskTracker* tracker) = 0;
+
+  // Deletes favicon mappings for each URL in |page_urls| and their redirects.
+  virtual void DeleteFaviconMappings(const base::flat_set<GURL>& page_urls,
+                                     favicon_base::IconType icon_type) = 0;
+
+  // Avoid repeated requests to download missing favicon.
+  virtual void UnableToDownloadFavicon(const GURL& icon_url) = 0;
+  virtual void ClearUnableToDownloadFavicons() = 0;
+  virtual bool WasUnableToDownloadFavicon(const GURL& icon_url) const = 0;
+};
+
+}  // namespace favicon
+
+#endif  // COMPONENTS_FAVICON_CORE_CORE_FAVICON_SERVICE_H_

components/favicon/core/favicon_driver_impl.cc

@@ -9,9 +9,9 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_util.h"
 #include "build/build_config.h"
+#include "components/favicon/core/core_favicon_service.h"
 #include "components/favicon/core/favicon_driver_observer.h"
 #include "components/favicon/core/favicon_handler.h"
-#include "components/favicon/core/favicon_service.h"
 #include "components/favicon/core/favicon_url.h"
 
 namespace favicon {
@@ -25,7 +25,7 @@ const bool kEnableTouchIcon = false;
 
 }  // namespace
 
-FaviconDriverImpl::FaviconDriverImpl(FaviconService* favicon_service)
+FaviconDriverImpl::FaviconDriverImpl(CoreFaviconService* favicon_service)
     : favicon_service_(favicon_service) {
   if (!favicon_service_)
     return;

components/favicon/core/favicon_driver_impl.h

@@ -16,7 +16,7 @@ class GURL;
 
 namespace favicon {
 
-class FaviconService;
+class CoreFaviconService;
 struct FaviconURL;
 
 // FaviconDriverImpl is a partial implementation of FaviconDriver that allow
@@ -33,14 +33,14 @@ class FaviconDriverImpl : public FaviconDriver,
   void FetchFavicon(const GURL& page_url, bool is_same_document) override;
 
   // Returns whether the driver is waiting for a download to complete or for
-  // data from the FaviconService. Reserved for testing.
+  // data from the CoreFaviconService. Reserved for testing.
   bool HasPendingTasksForTest();
 
  protected:
-  explicit FaviconDriverImpl(FaviconService* favicon_service);
+  explicit FaviconDriverImpl(CoreFaviconService* favicon_service);
   ~FaviconDriverImpl() override;
 
-  // Informs FaviconService that the favicon for |url| is out of date. If
+  // Informs CoreFaviconService that the favicon for |url| is out of date. If
   // |force_reload| is true, then discard information about favicon download
   // failures.
   void SetFaviconOutOfDateForPage(const GURL& url, bool force_reload);
@@ -51,12 +51,12 @@ class FaviconDriverImpl : public FaviconDriver,
                           const GURL& manifest_url);
 
  protected:
-  FaviconService* favicon_service() { return favicon_service_; }
+  CoreFaviconService* favicon_service() { return favicon_service_; }
 
  private:
   // KeyedService used by FaviconDriverImpl. It may be null during testing,
   // but if it is defined, it must outlive the FaviconDriverImpl.
-  FaviconService* favicon_service_;
+  CoreFaviconService* favicon_service_;
 
   // FaviconHandlers used to download the different kind of favicons.
   std::vector<std::unique_ptr<FaviconHandler>> handlers_;

components/favicon/core/favicon_handler.cc

@@ -17,7 +17,7 @@
 #include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
 #include "build/build_config.h"
-#include "components/favicon/core/favicon_service.h"
+#include "components/favicon/core/core_favicon_service.h"
 #include "components/favicon/core/features.h"
 #include "components/favicon_base/favicon_util.h"
 #include "components/favicon_base/select_favicon_frames.h"
@@ -152,7 +152,7 @@ FaviconHandler::FaviconCandidate::FromFaviconURL(
 ////////////////////////////////////////////////////////////////////////////////
 
 FaviconHandler::FaviconHandler(
-    FaviconService* service,
+    CoreFaviconService* service,
     Delegate* delegate,
     FaviconDriverObserver::NotificationIconType handler_type)
     : handler_type_(handler_type),

components/favicon/core/favicon_handler.h

@@ -27,7 +27,7 @@ class SkBitmap;
 
 namespace favicon {
 
-class FaviconService;
+class CoreFaviconService;
 
 // FaviconHandler works with FaviconDriver to fetch the specific type of
 // favicon.
@@ -128,7 +128,7 @@ class FaviconHandler {
   };
 
   // |service| and |delegate| must not be nullptr and must outlive this class.
-  FaviconHandler(FaviconService* service,
+  FaviconHandler(CoreFaviconService* service,
                  Delegate* delegate,
                  FaviconDriverObserver::NotificationIconType handler_type);
   ~FaviconHandler();
@@ -152,7 +152,7 @@ class FaviconHandler {
   const std::vector<GURL> GetIconURLs() const;
 
   // Returns whether the handler is waiting for a download to complete or for
-  // data from the FaviconService. Reserved for testing.
+  // data from the CoreFaviconService. Reserved for testing.
   bool HasPendingTasksForTest();
 
   // Get the maximal icon size in pixels for a handler of type |handler_type|.
@@ -293,7 +293,7 @@ class FaviconHandler {
   // triggered in FetchFavicon().
   base::CancelableTaskTracker cancelable_task_tracker_for_page_url_;
 
-  // Used for various FaviconService methods triggered while processing
+  // Used for various CoreFaviconService methods triggered while processing
   // candidates.
   base::CancelableTaskTracker cancelable_task_tracker_for_candidates_;
 
@@ -305,7 +305,7 @@ class FaviconHandler {
   // The last page URL reported via FetchFavicon().
   GURL last_page_url_;
 
-  // Whether we got data back for the initial request to the FaviconService.
+  // Whether we got data back for the initial request to the CoreFaviconService.
   bool got_favicon_from_history_;
 
   // Whether the history data returned in
@@ -359,9 +359,9 @@ class FaviconHandler {
   GURL notification_icon_url_;
   favicon_base::IconType notification_icon_type_;
 
-  // The FaviconService which implements favicon operations. May be null during
-  // testing.
-  FaviconService* service_;
+  // The CoreFaviconService which implements favicon operations. May be null
+  // during testing.
+  CoreFaviconService* service_;
 
   // This handler's delegate.
   Delegate* delegate_;

components/favicon/core/favicon_service.h

@@ -5,24 +5,20 @@
 #ifndef COMPONENTS_FAVICON_CORE_FAVICON_SERVICE_H_
 #define COMPONENTS_FAVICON_CORE_FAVICON_SERVICE_H_
 
-#include <stdint.h>
-
-#include <memory>
 #include <vector>
 
 #include "base/callback.h"
-#include "base/containers/flat_set.h"
 #include "base/task/cancelable_task_tracker.h"
+#include "components/favicon/core/core_favicon_service.h"
 #include "components/favicon_base/favicon_callback.h"
 #include "components/favicon_base/favicon_types.h"
 #include "components/favicon_base/favicon_usage_data.h"
-#include "components/keyed_service/core/keyed_service.h"
 
 class GURL;
 
 namespace favicon {
 
-class FaviconService : public KeyedService {
+class FaviconService : public CoreFaviconService {
  public:
   //////////////////////////////////////////////////////////////////////////////
   // Methods to request favicon bitmaps from the history backend for |icon_url|.
@@ -51,20 +47,6 @@ class FaviconService : public KeyedService {
       favicon_base::FaviconRawBitmapCallback callback,
       base::CancelableTaskTracker* tracker) = 0;
 
-  // The first argument for |callback| is the set of bitmaps for the passed in
-  // URL and icon types whose pixel sizes best match the passed in
-  // |desired_size_in_dip| at the resource scale factors supported by the
-  // current platform (eg MacOS) in addition to 1x. The vector has at most one
-  // result for each of the resource scale factors. There are less entries if a
-  // single/ result is the best bitmap to use for several resource scale
-  // factors.
-  virtual base::CancelableTaskTracker::TaskId GetFavicon(
-      const GURL& icon_url,
-      favicon_base::IconType icon_type,
-      int desired_size_in_dip,
-      favicon_base::FaviconResultsCallback callback,
-      base::CancelableTaskTracker* tracker) = 0;
-
   //////////////////////////////////////////////////////////////////////////////
   // Methods to request favicon bitmaps from the history backend for |page_url|.
   // |page_url| is the web page the favicon is associated with.
@@ -107,31 +89,6 @@ class FaviconService : public KeyedService {
       favicon_base::FaviconRawBitmapCallback callback,
       base::CancelableTaskTracker* tracker) = 0;
 
-  virtual base::CancelableTaskTracker::TaskId GetFaviconForPageURL(
-      const GURL& page_url,
-      const favicon_base::IconTypeSet& icon_types,
-      int desired_size_in_dip,
-      favicon_base::FaviconResultsCallback callback,
-      base::CancelableTaskTracker* tracker) = 0;
-
-  // Maps |page_urls| to the favicon at |icon_url| if there is an entry in the
-  // database for |icon_url| and |icon_type|. This occurs when there is a
-  // mapping from a different page URL to |icon_url|. The favicon bitmaps whose
-  // edge sizes most closely match |desired_size_in_dip| from the favicons which
-  // were just mapped to |page_urls| are returned. If |desired_size_in_dip| has
-  // a '0' entry, the largest favicon bitmap is returned.
-  virtual base::CancelableTaskTracker::TaskId UpdateFaviconMappingsAndFetch(
-      const base::flat_set<GURL>& page_urls,
-      const GURL& icon_url,
-      favicon_base::IconType icon_type,
-      int desired_size_in_dip,
-      favicon_base::FaviconResultsCallback callback,
-      base::CancelableTaskTracker* tracker) = 0;
-
-  // Deletes favicon mappings for each URL in |page_urls| and their redirects.
-  virtual void DeleteFaviconMappings(const base::flat_set<GURL>& page_urls,
-                                     favicon_base::IconType icon_type) = 0;
-
   // Used to request a bitmap for the favicon with |favicon_id| which is not
   // resized from the size it is stored at in the database. If there are
   // multiple favicon bitmaps for |favicon_id|, the largest favicon bitmap is
@@ -141,9 +98,6 @@ class FaviconService : public KeyedService {
       favicon_base::FaviconRawBitmapCallback callback,
       base::CancelableTaskTracker* tracker) = 0;
 
-  // Marks all types of favicon for the page as being out of date.
-  virtual void SetFaviconOutOfDateForPage(const GURL& page_url) = 0;
-
   // Mark that the on-demand favicon at |icon_url| was requested now. This
   // postpones the automatic eviction of the favicon from the database. Not all
   // calls end up in a write into the DB:
@@ -175,29 +129,6 @@ class FaviconService : public KeyedService {
                             scoped_refptr<base::RefCountedMemory> bitmap_data,
                             const gfx::Size& pixel_size) = 0;
 
-  // Set the favicon for all URLs in |page_urls| for |icon_type| in the
-  // thumbnail database. |icon_url| is the single favicon to map to. Mappings
-  // from page URLs to favicons at different icon URLs will be deleted.
-  // A favicon bitmap is added for each image rep in |image|. Any preexisting
-  // bitmap data for |icon_url| is deleted. It is important that |image|
-  // contains image reps for all of ui::GetSupportedScaleFactors(). Use
-  // MergeFavicon() if it does not.
-  // TODO(pkotwicz): Save unresized favicon bitmaps to the database.
-  // TODO(pkotwicz): Support adding favicons for multiple icon URLs to the
-  // thumbnail database.
-  virtual void SetFavicons(const base::flat_set<GURL>& page_urls,
-                           const GURL& icon_url,
-                           favicon_base::IconType icon_type,
-                           const gfx::Image& image) = 0;
-
-  // Causes each page in |page_urls_to_write| to be associated to the same
-  // icon as the page |page_url_to_read| for icon types matching |icon_types|.
-  // No-op if |page_url_to_read| has no mappings for |icon_types|.
-  virtual void CloneFaviconMappingsForPages(
-      const GURL& page_url_to_read,
-      const favicon_base::IconTypeSet& icon_types,
-      const base::flat_set<GURL>& page_urls_to_write) = 0;
-
   // Figures out whether an on-demand favicon can be written for provided
   // |page_url| and returns the result via |callback|. The result is false if
   // there is an existing cached favicon for |icon_type| or if there is a
@@ -229,11 +160,6 @@ class FaviconService : public KeyedService {
                                    favicon_base::IconType icon_type,
                                    const gfx::Image& image,
                                    base::OnceCallback<void(bool)> callback) = 0;
-
-  // Avoid repeated requests to download missing favicon.
-  virtual void UnableToDownloadFavicon(const GURL& icon_url) = 0;
-  virtual bool WasUnableToDownloadFavicon(const GURL& icon_url) const = 0;
-  virtual void ClearUnableToDownloadFavicons() = 0;
 };
 
 }  // namespace favicon

components/favicon/ios/web_favicon_driver.h

@@ -17,6 +17,8 @@ class WebState;
 
 namespace favicon {
 
+class CoreFaviconService;
+
 // WebFaviconDriver is an implementation of FaviconDriver that listen to
 // WebState events to start download of favicons and to get informed when the
 // favicon download has completed.
@@ -27,7 +29,7 @@ class WebFaviconDriver : public web::WebStateObserver,
   ~WebFaviconDriver() override;
 
   static void CreateForWebState(web::WebState* web_state,
-                                FaviconService* favicon_service);
+                                CoreFaviconService* favicon_service);
 
   // FaviconDriver implementation.
   gfx::Image GetFavicon() const override;
@@ -54,7 +56,8 @@ class WebFaviconDriver : public web::WebStateObserver,
  private:
   friend class web::WebStateUserData<WebFaviconDriver>;
 
-  WebFaviconDriver(web::WebState* web_state, FaviconService* favicon_service);
+  WebFaviconDriver(web::WebState* web_state,
+                   CoreFaviconService* favicon_service);
 
   // web::WebStateObserver implementation.
   void DidFinishNavigation(web::WebState* web_state,

components/favicon/ios/web_favicon_driver.mm

@@ -27,7 +27,7 @@ namespace favicon {
 
 // static
 void WebFaviconDriver::CreateForWebState(web::WebState* web_state,
-                                         FaviconService* favicon_service) {
+                                         CoreFaviconService* favicon_service) {
   if (FromWebState(web_state))
     return;
 
@@ -139,7 +139,7 @@ void WebFaviconDriver::OnFaviconDeleted(
 }
 
 WebFaviconDriver::WebFaviconDriver(web::WebState* web_state,
-                                   FaviconService* favicon_service)
+                                   CoreFaviconService* favicon_service)
     : FaviconDriverImpl(favicon_service),
       image_fetcher_(web_state->GetBrowserState()->GetSharedURLLoaderFactory()),
       web_state_(web_state) {

ios/chrome/browser/reading_list/favicon_web_state_dispatcher_impl.mm

@@ -4,6 +4,7 @@
 
 #include "ios/chrome/browser/reading_list/favicon_web_state_dispatcher_impl.h"
 
+#include "components/favicon/core/favicon_service.h"
 #include "components/favicon/ios/web_favicon_driver.h"
 #include "components/keyed_service/core/service_access_type.h"
 #include "ios/chrome/browser/browser_state/chrome_browser_state.h"

ios/chrome/browser/search_engines/BUILD.gn

@@ -83,6 +83,7 @@ source_set("unit_tests") {
     ":search_engines",
     "//base:base",
     "//base/test:test_support",
+    "//components/favicon/core",
     "//components/favicon/ios",
     "//components/search_engines",
     "//ios/chrome/browser/browser_state",

ios/chrome/browser/search_engines/search_engine_tab_helper_unittest.mm

@@ -7,6 +7,7 @@
 #include "base/bind.h"
 #include "base/strings/utf_string_conversions.h"
 #import "base/test/ios/wait_util.h"
+#include "components/favicon/core/favicon_service.h"
 #include "components/favicon/ios/web_favicon_driver.h"
 #include "components/keyed_service/core/service_access_type.h"
 #include "components/search_engines/template_url_service.h"

ios/chrome/browser/tabs/BUILD.gn

@@ -43,6 +43,7 @@ source_set("tabs_internal") {
   deps = [
     ":tabs",
     "//base",
+    "//components/favicon/core",
     "//components/favicon/ios",
     "//components/history/core/browser",
     "//components/history/ios/browser",

ios/chrome/browser/tabs/tab_helper_util.mm

@@ -9,6 +9,7 @@
 #endif
 
 #include "base/feature_list.h"
+#include "components/favicon/core/favicon_service.h"
 #import "components/favicon/ios/web_favicon_driver.h"
 #include "components/history/core/browser/top_sites.h"
 #import "components/history/ios/browser/web_state_top_sites_observer.h"