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..b015366e3b02e8c6e3d261e87641145602b1b5fe 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,48 @@ class FaviconService : public KeyedService {
       const favicon_base::FaviconResultsCallback& callback,
       base::CancelableTaskTracker* tracker);
 
+  // If there are any FAVICONs for the page at |page_url| in the local cache, it
+  // uses only the local cache and never requests the icon from any Google
+  // server. If there is no icon in the cache larger than
+  // |minimum_size_in_pixels|, the result is an empty image. Otherwise, the
+  // result is a favicon rescaled to |desired_size_in_pixel|. If there are
+  // multiple icons to choose from, the one with size closest to the desired one
+  // is used (always preferring larger than |desired_size_in_pixel| over smaller
+  // ones).
+  //
+  // If there are no FAVICONs for the page at |page_url|, the function downloads
+  // the favicon for the page at |page_url| with size |desired_size_in_pixel|
+  // from a Google server. Currently, only icons of FAVICON IconType and of
+  // sizes 16, 24, 32, 48, and 64 are 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 no icon is available for
+  // the given url and size, the callback is called with a result with an empty
+  // image.
+  //
+  // If the request is successful, it stores the resulting favicon in the cache
+  // for future use (possibly overwriting previous image in the cache for the
+  // given url). 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).
+  base::CancelableTaskTracker::TaskId
+  GetRawFaviconForPageURLDownloadFromGoogleServerIfNeeded(

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

Peter, in an offline discussion, you've suggested the name
GetRawFaviconForPageURLDownloadFromGoogleServerIfNeeded. Why "Raw" when it
returns an gfx::Image?  (and not RawData)

[pkotwicz, 2016/12/09 19:46:30]

The goal of this function is to get a single resolution bitmap as a opposed to a
multi resolution gfx::Image. We currently return single resolution bitmaps as
base::FaviconRawBitmapResult

I need to think some more about what the best function name would be. I
understand why you are currently using FaviconImageResult

[jkrcal, 2016/12/14 15:18:50]

Okay, thanks. Will keep it as it is, rename it in later revisions.

+      const GURL& page_url,
+      int minimum_size_in_pixels,
+      int desired_size_in_pixels,
+      std::string client_id,
+      data_use_measurement::DataUseUserData::ServiceName data_use_service_name,
+      const favicon_base::FaviconImageCallback& callback,
+      base::CancelableTaskTracker* tracker);
+
   // Set the favicon mappings to |page_url| for |icon_types| in the history
   // database.
   // Sample |icon_urls|:
@@ -244,9 +293,32 @@ class FaviconService : public KeyedService {
       const std::vector<favicon_base::FaviconRawBitmapResult>&
           favicon_bitmap_results);
 
+  // Intermediate callback for
+  // GetRawFaviconForPageURLDownloadFromGoogleServerIfNeeded() so that the
+  // client code can deal solely with FaviconImageCallback.
+  void RunFaviconImageCallbackOrDownloadFromGoogleFaviconServer(
+      const GURL& page_url,
+      int minimum_size_in_pixels,
+      int desired_size_in_pixels,
+      std::string client_id,
+      data_use_measurement::DataUseUserData::ServiceName data_use_service_name,
+      const favicon_base::FaviconImageCallback& callback,
+      const std::vector<favicon_base::FaviconRawBitmapResult>&
+          favicon_bitmap_results);
+
+  // Intermediate callback for
+  // RunFaviconImageCallbackOrDownloadFromGoogleFaviconServer() 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_;

[pkotwicz, 2016/12/09 19:46:30]

Is the security team OK with decoding images from the web in the browser
process? The security team has been uncomfortable with this in the past. (Hence
the existance of WebContents::DownloadImage())

If they are ok with this, that's great but I would love to know the reasoning
for this change in policy

[jkrcal, 2016/12/14 15:18:50]

componentes/image_fetcher uses an isolated process for decoding the images
(applying https://cs.chromium.org/chromium/src/chrome/browser/image_decoder.h).

[pkotwicz, 2016/12/15 01:22:27]

You are right. I see now :)

 
   DISALLOW_COPY_AND_ASSIGN(FaviconService);
 };