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..71cef81a9cdc75735fdc962121ac7a33561b087f 100644
--- a/components/favicon/core/favicon_service.cc
+++ b/components/favicon/core/favicon_service.cc
@@ -5,17 +5,20 @@
 #include "components/favicon/core/favicon_service.h"
 
 #include <stddef.h>
+#include <algorithm>
 #include <cmath>
 #include <utility>
 
 #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 +28,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};
+

[pkotwicz, 2016/12/19 00:03:28]

Perhaps GetClosestGoogleFaviconServerSupportedSize() is clearer

[jkrcal, 2016/12/19 17:07:19]

Done.

+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];
+}
+

[pkotwicz, 2016/12/19 00:03:28]

How about: GetGoogleFaviconServerURLForPageURL()

[jkrcal, 2016/12/19 17:07:19]

Done.

+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 +80,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() {
 }
@@ -199,6 +231,30 @@ base::CancelableTaskTracker::TaskId FaviconService::GetFaviconForPageURL(
 }
 
 base::CancelableTaskTracker::TaskId
+FaviconService::GetRawFaviconForPageURLDownloadFromGoogleServerIfNeeded(

[pkotwicz, 2016/12/19 00:03:28]

How about Get1xFaviconForPageURLDownloadFromGoogleServerIfMissing()

This name is not like the other names in the FaviconService interface but it is
not misleading
- It does not have RawRavicon as a substring. This is good because it does not
use a FaviconRawBitmapResult callback
- It makes it explicit that the return value is single resolution

[pkotwicz, 2016/12/19 00:03:28]

We should add a TODO to switch FaviconHelper#ensureIconIsAvailable() to use this
instead

[jkrcal, 2016/12/19 17:07:19]

Done.

+    const GURL& page_url,
+    int minimum_size_in_pixel,
+    int desired_size_in_pixel,
+    std::string client_id,
+    data_use_measurement::DataUseUserData::ServiceName data_use_service_name,
+    const favicon_base::FaviconImageCallback& callback,
+    base::CancelableTaskTracker* tracker) {
+  TRACE_EVENT0("browser",
+               "FaviconService::"
+               "GetRawFaviconForPageURLDownloadFromGoogleServerIfNeeded");
+  std::vector<int> desired_sizes_in_pixel;
+  desired_sizes_in_pixel.push_back(desired_size_in_pixel);
+  return GetFaviconForPageURLImpl(
+      page_url, favicon_base::IconType::FAVICON, desired_sizes_in_pixel,
+      base::Bind(&FaviconService::
+                     RunFaviconImageCallbackOrDownloadFromGoogleFaviconServer,
+                 base::Unretained(this), page_url, minimum_size_in_pixel,
+                 desired_size_in_pixel, client_id, data_use_service_name,
+                 callback),
+      tracker);
+}
+
+base::CancelableTaskTracker::TaskId
 FaviconService::UpdateFaviconMappingsAndFetch(
     const GURL& page_url,
     const std::vector<GURL>& icon_urls,
@@ -348,4 +404,104 @@ void FaviconService::RunFaviconRawBitmapCallbackWithBitmapResults(
       ResizeFaviconBitmapResult(favicon_bitmap_results, desired_size_in_pixel));
 }
 
+void FaviconService::RunFaviconImageCallbackOrDownloadFromGoogleFaviconServer(
+    const GURL& page_url,
+    int minimum_size_in_pixel,
+    int desired_size_in_pixel,
+    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) {
+  TRACE_EVENT0(
+      "browser",
+      "FaviconService::DownloadFromGoogleServerFaviconImageForPageURL");

[pkotwicz, 2016/12/19 00:03:28]

Shouldn't the second argument be
"FaviconService::RunFaviconImageCallbackOrDownloadFromGoogleFaviconServer"

[jkrcal, 2016/12/19 17:07:19]

Sure, thanks!

+
+  // Ensure we do not get an icon upscaled from a smaller png than
+  // |minimum_size_in_pixel|.
+  int max_size_in_pixel = 0;
+  for (const auto& bitmap : favicon_bitmap_results) {

[pkotwicz, 2016/12/19 00:03:28]

Can you make |max_size_in_pixel| a boolean and early return once you got a
match?

[jkrcal, 2016/12/19 17:07:19]

Done.

+    max_size_in_pixel =
+        std::max(max_size_in_pixel, std::max(bitmap.pixel_size_in_db.width(),
+                                             bitmap.pixel_size_in_db.height()));
+  }
+
+  // If there are some cached bitmaps but none is large enough, we cannot return
+  // any of them as the result and we also do not want to overwrite them by an
+  // icon from the Google server. Thus, return an empty result.
+  if (max_size_in_pixel > 0 && max_size_in_pixel < minimum_size_in_pixel) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(

[pkotwicz, 2016/12/19 00:03:28]

What's the reason for posting a task? It looks like only
RunWithEmptyResultAsync() posts a task in this class.

RunFaviconImageCallbackOrDownloadFromGoogleFaviconServer() is not called
synchronously from GetRawFaviconForPageURLDownloadFromGoogleServerIfNeeded() so
I don't think you need to post a task

[jkrcal, 2016/12/19 17:07:19]

Ah, sure, thanks!

+        FROM_HERE, base::Bind(callback, favicon_base::FaviconImageResult()));
+    return;
+  }
+
+  gfx::Image image;
+  image = favicon_base::SelectFaviconFramesFromPNGs(
+      favicon_bitmap_results,

[pkotwicz, 2016/12/19 00:03:28]

You should set the desired scale to be 1.0f and not do anything fancy with
|desired_size_in_pixel|

[jkrcal, 2016/12/19 17:07:19]

Done.

+      {static_cast<float>(desired_size_in_pixel) / gfx::kFaviconSize},
+      gfx::kFaviconSize);
+
+  if (!image.IsEmpty()) {
+    // If a large enough cached bitmap is successfully rescaled, serve it!
+    favicon_base::FaviconImageResult image_result;
+    image_result.image = image;
+    favicon_base::SetFaviconColorSpace(&image_result.image);
+    image_result.icon_url = favicon_bitmap_results[0].icon_url;
+    callback.Run(image_result);
+    return;
+  }
+
+  // No good enough favicon in the cache. Download it from the server.
+
+  if (!image_fetcher_.get()) {

[pkotwicz, 2016/12/19 00:03:28]

You don't need to post a task

[jkrcal, 2016/12/19 17:07:19]

Done.

+    // No fetcher available (e.g. in unittests), return an empty result.
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(callback, favicon_base::FaviconImageResult()));
+    return;
+  }
+
+  // TODO(jkrcal): When we use the other server backend, require
+  // |minimum_size_in_pixel|.
+  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(
+      image_url.spec(), image_url,
+      base::Bind(&FaviconService::
+                     StoreFaviconFromGoogleServiceAndRunFaviconImageCallback,
+                 base::Unretained(this), callback, page_url,
+                 desired_size_in_pixel));
+}
+
+void FaviconService::StoreFaviconFromGoogleServiceAndRunFaviconImageCallback(

[pkotwicz, 2016/12/19 00:03:28]

How about: StoreFaviconFromGoogleServerAndRunFaviconImageCallback

[jkrcal, 2016/12/19 17:07:19]

Done.

+      const favicon_base::FaviconImageCallback& callback,
+      const GURL& page_url,
+      int desired_size_in_pixel,
+      const std::string& icon_url,
+      const gfx::Image& image) {
+  if (image.IsEmpty())
+    callback.Run(favicon_base::FaviconImageResult());
+
+  // Resize the icon to the desired size.

[pkotwicz, 2016/12/19 00:03:28]

Nuke this comment. What ResizeBitmapByDownsamplingIfPossible() does is obvious
from its name

[jkrcal, 2016/12/19 17:07:19]

Done.

+  SkBitmap icon = image.AsBitmap();
+  SkBitmap icon_resized = favicon_base::ResizeBitmapByDownsamplingIfPossible(
+      {icon}, desired_size_in_pixel);
+
+  favicon_base::FaviconImageResult image_result;
+  image_result.icon_url = GURL(icon_url);
+  image_result.image = gfx::Image::CreateFrom1xBitmap(icon_resized);
+  favicon_base::SetFaviconColorSpace(&image_result.image);
+  // Store the favicon in the cache.

[pkotwicz, 2016/12/19 00:03:28]

Nuke this comment. What the function does should be obvious from its name. If it
isn't we should change the name.

I understand that it currently isn't obvious that SetFavicons() store bitmaps
into the cache.

I am not suggesting that you rename SetFavicons() in this CL

[jkrcal, 2016/12/19 17:07:19]

Done.

+  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

[pkotwicz, 2016/12/19 00:03:28]

Nit: "them" -> "the icons"

+  // original page any time in the future.
+  SetFaviconOutOfDateForPage(page_url);
+
+  callback.Run(image_result);
+}
+
 }  // namespace favicon