Extend FaviconService to support fetching favicons from a Google server

components/favicon/core/favicon_service.cc

Index: components/favicon/core/favicon_service.cc
diff --git a/components/favicon/core/favicon_service.cc b/components/favicon/core/favicon_service.cc
index 57a29deaf5fe69c7250a85ef799d064bab7ffe7a..6c307f49b447158b9e62ea3adb0217244a0103d8 100644
--- a/components/favicon/core/favicon_service.cc
+++ b/components/favicon/core/favicon_service.cc
@@ -10,12 +10,14 @@
 
 #include "base/hash.h"
 #include "base/single_thread_task_runner.h"
+#include "base/strings/stringprintf.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/trace_event/trace_event.h"
 #include "components/favicon/core/favicon_client.h"
 #include "components/favicon_base/favicon_util.h"
 #include "components/favicon_base/select_favicon_frames.h"
 #include "components/history/core/browser/history_service.h"
+#include "components/image_fetcher/image_fetcher.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/gfx/codec/png_codec.h"
 #include "ui/gfx/favicon_size.h"
@@ -25,6 +27,32 @@
 namespace favicon {
 namespace {
 
+const char kGoogleFaviconServiceRequestFormat[] =
+    "https://s2.googleusercontent.com/s2/"
+    "favicons?domain=%s&src=%s&sz=%d&alt=404";
+
+const int kGoogleFaviconServiceSupportedSizes[] = {16, 24, 32, 48, 64};
+
+int GetGoogleFaviconServiceSupportedSize(int arbitrary_size) {
+  // Take the smallest size larger than arbitrary_size.
+  for (int size : kGoogleFaviconServiceSupportedSizes) {
+    if (size >= arbitrary_size)
+      return size;
+  }
+  // Or at least the largest available size.
+  return kGoogleFaviconServiceSupportedSizes
+      [arraysize(kGoogleFaviconServiceSupportedSizes) - 1];
+}
+
+GURL GetGoogleFaviconServiceURL(GURL page_url,
+                                const std::string& client_id,
+                                int supported_size) {
+  return GURL(base::StringPrintf(kGoogleFaviconServiceRequestFormat,
+                                 page_url.spec().c_str(),
+                                 client_id.c_str(),
+                                 supported_size));
+}
+
 // Helper to run callback with empty results if we cannot get the history
 // service.
 base::CancelableTaskTracker::TaskId RunWithEmptyResultAsync(
@@ -51,10 +79,13 @@ std::vector<int> GetPixelSizesForFaviconScales(int size_in_dip) {
 
 }  // namespace
 
-FaviconService::FaviconService(std::unique_ptr<FaviconClient> favicon_client,
-                               history::HistoryService* history_service)
+FaviconService::FaviconService(
+    std::unique_ptr<FaviconClient> favicon_client,
+    history::HistoryService* history_service,
+    std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher)
     : favicon_client_(std::move(favicon_client)),
-      history_service_(history_service) {}
+      history_service_(history_service),
+      image_fetcher_(std::move(image_fetcher)) {}
 
 FaviconService::~FaviconService() {
 }
@@ -198,6 +229,31 @@ base::CancelableTaskTracker::TaskId FaviconService::GetFaviconForPageURL(
       tracker);
 }
 
+void FaviconService::DownloadFromGoogleServerFaviconImageForPageURL(
+    const GURL& page_url,
+    int desired_size_in_pixel,
+    std::string client_id,
+    data_use_measurement::DataUseUserData::ServiceName data_use_service_name,
+    const favicon_base::FaviconImageCallback& callback) {
+  if (!image_fetcher_.get()) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(callback, favicon_base::FaviconImageResult()));
+    return;
+  }
+
+  int supported_size =
+      GetGoogleFaviconServiceSupportedSize(desired_size_in_pixel);
+  GURL image_url = GetGoogleFaviconServiceURL(page_url, client_id,
+                                              supported_size);
+
+  image_fetcher_->SetDataUseServiceName(data_use_service_name);
+  image_fetcher_->StartOrQueueNetworkRequest(

[pkotwicz, 2016/09/21 20:55:53]

I think that ImageFetcherImpl::StartOrQueueNetworkRequest() does an HTTP GET
request.

I assume that if someone malicious:
- used one of Chrome's client IDs (The client IDs will not be in the internal
portion of Chrome?)
- does millions of HTTP GET requests
That this would not affect Chrome's legitimate requests?

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

This would probably affect Chrome's legitimate requests. None of the both
favicon services teams I am in contact with seem to be too concerned about
having such a public API. The client ID is just for basic understanding of the
traffic.

Do you see any other reasonably simple mechanism that 
 - would work also for non-signed-in users and
 - would not be prone to such simple attacks?

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

I don't know of a mechanism which would not be prone to simple attacks. (I am no
security expert though)

I don't understand why we need a client ID at all (other than for one Google app
to affect another Google app's usage of the service).

In the feature that I work on (WebAPKs) we use an API key as well. I don't
understand why we are using the API key in the case of WebAPKs either

The API key is probably OK because it is "the Google Way"

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

Acknowledged.

+      image_url.spec(), image_url,
+      base::Bind(&FaviconService::
+                     StoreFaviconFromGoogleServiceAndRunFaviconImageCallback,
+                 base::Unretained(this), callback, page_url));
+}
+
 base::CancelableTaskTracker::TaskId
 FaviconService::UpdateFaviconMappingsAndFetch(
     const GURL& page_url,
@@ -348,4 +404,28 @@ void FaviconService::RunFaviconRawBitmapCallbackWithBitmapResults(
       ResizeFaviconBitmapResult(favicon_bitmap_results, desired_size_in_pixel));
 }
 
+void FaviconService::StoreFaviconFromGoogleServiceAndRunFaviconImageCallback(
+      const favicon_base::FaviconImageCallback& callback,
+      const GURL& page_url,
+      const std::string& icon_url,
+      const gfx::Image& image) {
+  favicon_base::FaviconImageResult image_result;
+  image_result.icon_url = GURL(icon_url);
+  image_result.image = image;
+  if (image_result.image.IsEmpty()) {

[pkotwicz, 2016/12/07 04:31:22]

Can you move this if to line 412

I am suggesting:
if (image.IsEmpty()) {
  callback.Run(base::FaviconImageResult());
}
favicon base::FaviconImageResult image_result;
...

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

Sure. It's much better this way!
I was not sure what is the contract (whether the icon_url can be empty).

+    callback.Run(image_result);
+    return;
+  }
+
+  favicon_base::SetFaviconColorSpace(&image_result.image);
+  // Store the favicon in the cache.
+  SetFavicons(page_url, image_result.icon_url,
+              favicon_base::IconType::FAVICON, image_result.image);
+  // Mark them as out-of-date so that they are refetched when we visit the
+  // original page any time in the future.
+  SetFaviconOutOfDateForPage(page_url);
+
+  callback.Run(image_result);
+}
+
 }  // namespace favicon