Extend LargeIconService to fetch missing favicons from a Google server

components/favicon/core/large_icon_service.h

Index: components/favicon/core/large_icon_service.h
diff --git a/components/favicon/core/large_icon_service.h b/components/favicon/core/large_icon_service.h
index 73a2698448c9182cb2ac43fd9e7103ba14ad49f2..8fa2f1451336d3466fe24663e2466c26f6fb8dff 100644
--- a/components/favicon/core/large_icon_service.h
+++ b/components/favicon/core/large_icon_service.h
@@ -18,6 +18,10 @@ namespace base {
 class TaskRunner;
 }
 
+namespace image_fetcher {
+class ImageFetcher;
+}
+
 namespace favicon {
 
 class FaviconService;
@@ -28,7 +32,8 @@ 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|.
@@ -49,6 +54,26 @@ class LargeIconService : public KeyedService {
     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_;
@@ -58,6 +83,8 @@ class LargeIconService : public KeyedService {
   // request.
   std::vector<int> large_icon_types_;
 
+  std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher_;
+
   DISALLOW_COPY_AND_ASSIGN(LargeIconService);
 };