Extend FaviconService to support fetching favicons from a Google server

components/favicon/core/favicon_service.h

 // Copyright (c) 2012 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_FAVICON_SERVICE_H_
 #define COMPONENTS_FAVICON_CORE_FAVICON_SERVICE_H_
 
 #include <stdint.h>
 
 #include <memory>
+#include <string>
 #include <vector>
 
 #include "base/callback.h"
 #include "base/containers/hash_tables.h"
 #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"
 #include "components/keyed_service/core/keyed_service.h"
 
 class GURL;
 
 namespace history {
 class HistoryService;
 }
 
+namespace image_fetcher {
+class ImageFetcher;
+}
+
 namespace favicon {
 
 class FaviconClient;
 
 // The favicon service provides methods to access favicons. It calls the history
 // backend behind the scenes. The callbacks are run asynchronously, even in the
 // case of an error.
 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;
 
   // We usually pass parameters with pointer to avoid copy. This function is a
   // helper to run FaviconResultsCallback with pointer parameters.
   static void FaviconResultsCallbackRunner(
       const favicon_base::FaviconResultsCallback& callback,
       const std::vector<favicon_base::FaviconRawBitmapResult>* results);
 
   //////////////////////////////////////////////////////////////////////////////
@@ skipping 76 matching lines @@
       int minimum_size_in_pixels,
       const favicon_base::FaviconRawBitmapCallback& callback,
       base::CancelableTaskTracker* tracker);
 
   base::CancelableTaskTracker::TaskId GetFaviconForPageURL(
       const GURL& page_url,
       int icon_types,
       int desired_size_in_dip,
       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|:
   //  { ICON_URL1 -> TOUCH_ICON, known to the database,
   //    ICON_URL2 -> TOUCH_ICON, not known to the database,
   //    ICON_URL3 -> TOUCH_PRECOMPOSED_ICON, known to the database }
   // The new mappings are computed from |icon_urls| with these rules:
   // 1) Any urls in |icon_urls| which are not already known to the database are
   //    rejected.
   //    Sample new mappings to |page_url|: { ICON_URL1, ICON_URL3 }
@@ skipping 89 matching lines @@
   // Intermediate callback for GetRawFavicon() and GetRawFaviconForPageURL()
   // so that history service can deal solely with FaviconResultsCallback.
   // Resizes favicon_base::FaviconRawBitmapResult if necessary and runs
   // |callback|.
   void RunFaviconRawBitmapCallbackWithBitmapResults(
       const favicon_base::FaviconRawBitmapCallback& callback,
       int desired_size_in_pixel,
       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);
 };
 
 }  // namespace favicon
 
 #endif  // COMPONENTS_FAVICON_CORE_FAVICON_SERVICE_H_