Extend FaviconService to support fetching favicons from a Google server

components/favicon/core/favicon_service.h

Index: components/favicon/core/favicon_service.h
diff --git a/components/favicon/core/favicon_service.h b/components/favicon/core/favicon_service.h
index 9585934169f21c7407aff49adb9fa1718a3a30af..abe03be7c015d3d5307f70885bb54804501ffa3a 100644
--- a/components/favicon/core/favicon_service.h
+++ b/components/favicon/core/favicon_service.h
@@ -8,6 +8,7 @@
 #include <stdint.h>
 
 #include <memory>
+#include <string>
 #include <vector>
 
 #include "base/callback.h"
@@ -15,6 +16,7 @@
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/task/cancelable_task_tracker.h"
+#include "components/data_use_measurement/core/data_use_user_data.h"
 #include "components/favicon_base/favicon_callback.h"
 #include "components/favicon_base/favicon_types.h"
 #include "components/favicon_base/favicon_usage_data.h"
@@ -26,6 +28,10 @@ namespace history {
 class HistoryService;
 }
 
+namespace image_fetcher {
+class ImageFetcher;
+}
+
 namespace favicon {
 
 class FaviconClient;
@@ -37,7 +43,8 @@ class FaviconService : public KeyedService {
  public:
   // The FaviconClient must outlive the constructed FaviconService.
   FaviconService(std::unique_ptr<FaviconClient> favicon_client,
-                 history::HistoryService* history_service);
+                 history::HistoryService* history_service,
+                 std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher);
 
   ~FaviconService() override;
 
@@ -135,6 +142,37 @@ class FaviconService : public KeyedService {
       const favicon_base::FaviconResultsCallback& callback,
       base::CancelableTaskTracker* tracker);
 

[pkotwicz, 2016/09/19 20:07:51]

Thank you for the clear and very informative function comment

[jkrcal, 2016/09/20 15:52:06]

Thanks :)

+  // Downloads the favicon for the page at |page_url| with size
+  // |desired_size_in_pixel| from a Google server. Currently, only sizes 16, 24,
+  // 32, 48, and 64 are supported and only FAVICON IconType is supported. If you
+  // specify any different size < 64, the smallest larger size is fetched. If
+  // you set |desired_size_in_pixel| > 64, the 64px size is fetched. If the
+  // fetched size is not available, the largest available size is upscaled to
+  // the fetched size (by the Google server, at indexing time). If the URL at
+  // |domain_url| has non-empty path, its path is ignored (e.g.
+  // subdomain.foo.com/foo/bar -> subdomain.foo.com/). If no icon is available

[pkotwicz, 2016/09/19 20:07:51]

I think that ignoring the path should be done in the caller of this function not
inside this function. From local testing, the empty path does not seem to be an
API requirement. (I get different icons for www.google.com and
www.google.com/maps)

[jkrcal, 2016/09/20 15:52:06]

You are right, I've changed the |domain_url| param to |page_url|.

+  // for the given domain and size, the callback is called with a result with an
+  // empty image.
+  //
+  // This function does not check the local cache, it always directly calls the
+  // server. However, if the request is successful, it stores the resulting
+  // favicon in the cache for future use. The fetched favicon is immediately
+  // marked as expired so that when the user visits |page_url| for the next
+  // time, the favicon is re-downloaded.
+  //
+  // The Google favicon server requires to specify a string |client_id|
+  // used to identify your use case. Please negotiate a quota for your
+  // |client_id| before making use of this function in production (see
+  // g3doc/social/favicon/); use an arbitrary string in development builds. Use
+  // |data_use_service_name| to count the fetches to your data use (or
+  // NOT_TAGGED if not interested).
+  void DownloadFromGoogleServerFaviconImageForPageURL(
+      const GURL& domain_url,
+      int desired_size_in_pixel,

[noyau (Ping after 24h), 2016/09/19 09:00:50]

I would prefer this API to have a desired size in point and screen scale instead
of a size in pixel. This would express what's desired at the UI level better.
Probably outside the scope of this particular CL, and this would need to be
changed through the fetching API, but this would allow returning images setup
with the proper scale to display on screen. 

[pkotwicz, 2016/09/19 20:07:51]

Passing in the desired size in pixels matches the other methods in
FaviconService.

noyau@: I share your concern about making sure we fetch the right thing for
retina iOS devices.

[jkrcal, 2016/09/20 15:52:06]

I would like to keep it consistent with the rest of the service. 

Anyway, Éric, would a change of the fetching API be necessary? If we know the
favicon server provides only favicons, aren't all of them meant to be 16x16 DIP
(with 1x, 1.5x, 2x, 2.5x and 3x scale factors)?

[noyau (Ping after 24h), 2016/09/21 09:49:20]

I thought the servers was only serving 1x images. Am I mistaken?

[jkrcal, 2016/12/09 13:57:44]

All the images are favicons with 16 dip. They serve different sizes and thus
different scales (1x, 1.5x, 2x, 3x, 4x).

+      std::string client_id,
+      data_use_measurement::DataUseUserData::ServiceName data_use_service_name,
+      const favicon_base::FaviconImageCallback& callback);
+
   // Set the favicon mappings to |page_url| for |icon_types| in the history
   // database.
   // Sample |icon_urls|:
@@ -244,9 +282,18 @@ class FaviconService : public KeyedService {
       const std::vector<favicon_base::FaviconRawBitmapResult>&
           favicon_bitmap_results);
 
+  // Intermediate callback for DownloadFromGoogleServerFaviconImageForPageURL()
+  // so that the client code can deal solely with FaviconImageCallback.
+  void StoreFaviconFromGoogleServiceAndRunFaviconImageCallback(
+      const favicon_base::FaviconImageCallback& callback,
+      const GURL& domain_url,
+      const std::string& icon_url,
+      const gfx::Image& image);
+
   base::hash_set<MissingFaviconURLHash> missing_favicon_urls_;
   std::unique_ptr<FaviconClient> favicon_client_;
   history::HistoryService* history_service_;
+  std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher_;
 
   DISALLOW_COPY_AND_ASSIGN(FaviconService);
 };