Extend LargeIconService to fetch missing favicons from a Google server

components/favicon/core/large_icon_service.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_CORE_LARGE_ICON_SERVICE_H_
 #define COMPONENTS_FAVICON_CORE_LARGE_ICON_SERVICE_H_
 
 #include <vector>
 
 #include "base/macros.h"
 #include "base/task/cancelable_task_tracker.h"
 #include "components/favicon_base/favicon_callback.h"
 #include "components/keyed_service/core/keyed_service.h"
 
 class GURL;
 
 namespace base {
 class TaskRunner;
 }
 
+namespace image_fetcher {
+class ImageFetcher;
+}
+
 namespace favicon {
 
 class FaviconService;
 
 // The large icon service provides methods to access large icons. It relies on
 // the favicon service.
 class LargeIconService : public KeyedService {
  public:
   LargeIconService(
       FaviconService* favicon_service,
-      const scoped_refptr<base::TaskRunner>& background_task_runner);
+      const scoped_refptr<base::TaskRunner>& background_task_runner,
+      std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher);
   ~LargeIconService() override;
 
   // Requests the best large icon for the page at |page_url|.
   // Case 1. An icon exists whose size is >= |min_source_size_in_pixel|:
   // - If |desired_size_in_pixel| == 0: returns icon as is.
   // - Else: returns the icon resized to |desired_size_in_pixel|.
   // Case 2. An icon exists whose size is < |min_source_size_in_pixel|:
   // - Extracts dominant color of smaller image, returns a fallback icon style
   //   that has a matching background.
   // Case 3. No icon exists.
   // - Returns the default fallback icon style.
   // For cases 2 and 3, this function returns the style of the fallback icon
   // instead of rendering an icon so clients can render the icon themselves.
   base::CancelableTaskTracker::TaskId GetLargeIconOrFallbackStyle(
     const GURL& page_url,
     int min_source_size_in_pixel,
     int desired_size_in_pixel,
     const favicon_base::LargeIconCallback& callback,
     base::CancelableTaskTracker* tracker);
 
+  // Fetches the best large icon for the page at |page_url| from a Google
+  // favicon server and stores the result in the FaviconService cache

[pkotwicz, 2017/03/21 18:20:29]

Nit: cache -> database

[mastiz, 2017/03/22 09:46:53]

Done. I'm a bit surprised that, besides comments, you don't suggest renaming the
function.

+  // (implemented in HistoryService). The write will be a no-op if the local
+  // favicon cache contains an icon for |page_url|, so clients are encouraged to

[pkotwicz, 2017/03/21 18:20:29]

cache -> database

[pkotwicz, 2017/03/21 18:20:29]

cache -> database

[mastiz, 2017/03/22 09:46:53]

Done.

[mastiz, 2017/03/22 09:46:53]

Done.

+  // use GetLargeIconOrFallbackStyle() first.
+  //
+  // A minimum size |min_source_size_in_pixel| can be specified as constraint.

[pkotwicz, 2017/03/21 18:20:29]

"as constraint" -> "as a constraint"

[mastiz, 2017/03/22 09:46:53]

Done.

+  //
+  // The callback is triggered when the operation finishes, where |success|
+  // tells whether the fetch actually managed to cache a new icon in the
+  // FaviconService.

[pkotwicz, 2017/03/08 05:38:16]

The boolean in the callback is not really accurate. The callback might return
true even if the icon was not stored because there is already a favicon stored
for |page_url| in the favicon database.

Should the callback be void(gfx::Image image) instead? I think that callers care
mostly about the fetched gfx::Image.

[mastiz, 2017/03/08 16:37:33]

Fixed to return a proper value for the bool. I want to avoid returning the image
because this adds complexity to the API (min original size, resampling needs,
etc.) for something we'll likely not need (otherwise we can change the code
later).

[pkotwicz, 2017/03/21 18:20:29]

I am skeptical of the usefulness of the bool callback value but if you think it
is useful ok

[mastiz, 2017/03/22 09:46:53]

We can change it later if it turns out to the useless. For now, I anticipate it
will be used, and meanwhile it's useful for testing too.

+  //
+  // WARNING: This function will share the |page_url| with a Google server. This
+  // Can be used only for urls that are not privacy sensitive or for users that
+  // sync their history with Google servers.
+  void GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
+      const GURL& page_url,
+      int min_source_size_in_pixel,
+      const base::Callback<void(bool success)>& callback);
+
  private:
   FaviconService* favicon_service_;
   scoped_refptr<base::TaskRunner> background_task_runner_;
 
   // A pre-populated list of icon types to consider when looking for large
   // icons. This is an optimization over populating an icon type vector on each
   // request.
   std::vector<int> large_icon_types_;
 
+  std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher_;
+
   DISALLOW_COPY_AND_ASSIGN(LargeIconService);
 };
 
 }  // namespace favicon
 
 #endif  // COMPONENTS_FAVICON_CORE_LARGE_ICON_SERVICE_H_