Extend FaviconService to support fetching favicons from a Google server

components/favicon/core/favicon_service.cc

 // 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.
 
 #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"
 #include "ui/gfx/image/image_skia.h"
 #include "url/gurl.h"
 
 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 GetClosestGoogleFaviconServerSupportedSize(int arbitrary_size) {
+  // Take the smallest size larger than arbitrary_size.

[pkotwicz, 2016/12/20 02:33:01]

Nit: arbitrary_size -> |arbitrary_size|

[jkrcal, 2017/02/06 19:01:22]

Done.

+  for (int size : kGoogleFaviconServiceSupportedSizes) {
+    if (size >= arbitrary_size)
+      return size;
+  }
+  // Or at least the largest available size.
+  return kGoogleFaviconServiceSupportedSizes
+      [arraysize(kGoogleFaviconServiceSupportedSizes) - 1];
+}
+
+GURL GetGoogleFaviconServerURLForPageURL(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(
     const favicon_base::FaviconResultsCallback& callback,
     base::CancelableTaskTracker* tracker) {
   scoped_refptr<base::SingleThreadTaskRunner> thread_runner(
       base::ThreadTaskRunnerHandle::Get());
   return tracker->PostTask(
       thread_runner.get(), FROM_HERE,
       base::Bind(callback,
                  std::vector<favicon_base::FaviconRawBitmapResult>()));
 }
 
 // Returns a vector of pixel edge sizes from |size_in_dip| and
 // favicon_base::GetFaviconScales().
 std::vector<int> GetPixelSizesForFaviconScales(int size_in_dip) {
   std::vector<float> scales = favicon_base::GetFaviconScales();
   std::vector<int> sizes_in_pixel;
   for (size_t i = 0; i < scales.size(); ++i) {
     sizes_in_pixel.push_back(std::ceil(size_in_dip * scales[i]));
   }
   return sizes_in_pixel;
 }
 
 }  // 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() {
 }
 
 // static
 void FaviconService::FaviconResultsCallbackRunner(
     const favicon_base::FaviconResultsCallback& callback,
     const std::vector<favicon_base::FaviconRawBitmapResult>* results) {
   callback.Run(*results);
 }
@@ skipping 124 matching lines @@
   TRACE_EVENT0("browser", "FaviconService::GetFaviconForPageURL");
   return GetFaviconForPageURLImpl(
       page_url,
       icon_types,
       GetPixelSizesForFaviconScales(desired_size_in_dip),
       callback,
       tracker);
 }
 
 base::CancelableTaskTracker::TaskId
+FaviconService::Get1xFaviconForPageURLDownloadFromGoogleServerIfMissing(
+    const GURL& page_url,
+    int minimum_size_in_pixel,

[pkotwicz, 2016/12/20 02:33:02]

Aside: Sorry I did not think about this before. If you make this function use a
new type of callback favicon_base::FaviconBitmapCallback

struct FaviconBitmapResult {
  SkBitmap bitmap;
  gfx::Size pixel_size_in_db;
  GURL icon_url;
  IconType icon_type;
};

You can remove the |minimum_size_in_pixel| argument. The caller will check
FaviconBitmapResult::pixel_size_in_db to detect whether the bitmap in the
callback was upscaled from a really small size.

I would hold off on making this change till you get sky@'s comments. He might
not think that my proposal is a good idea

[jkrcal, 2017/02/06 19:01:22]

Do you understand it correctly that you suggest that I introduce a new callback
type and use it (solely for this function) instead of
favicon_base::FaviconImageCallback?

This sounds reasonable to me.

Scott, what do you think?

+    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::"
+               "Get1xFaviconForPageURLDownloadFromGoogleServerIfMissing");
+  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,
     int icon_types,
     int desired_size_in_dip,
     const favicon_base::FaviconResultsCallback& callback,
     base::CancelableTaskTracker* tracker) {
   if (history_service_) {
     return history_service_->UpdateFaviconMappingsAndFetch(
         page_url,
@@ skipping 129 matching lines @@
     const favicon_base::FaviconRawBitmapCallback& callback,
     int desired_size_in_pixel,
     const std::vector<favicon_base::FaviconRawBitmapResult>&
         favicon_bitmap_results) {
   TRACE_EVENT0("browser",
                "FaviconService::RunFaviconRawBitmapCallbackWithBitmapResults");
   callback.Run(
       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) {

[pkotwicz, 2016/12/20 02:33:01]

You need to pass CancelableTaskTracker as an argument too. The callback to
Get1xFaviconForPageURLDownloadFromGoogleServerIfMissing() should not be called
if CancelableTaskTracker::TryCancel() is called

[jkrcal, 2017/02/06 19:01:22]

I see the problem. I've spent quite some time thinking about how to fix it. 

I am not sure that what you propose is the solution to the problem because:
 - I could only post a new task to the tracker which has a different id than the
original id returned to the caller of
Get1xFaviconForPageURLDownloadFromGoogleServerIfMissing() so the caller cannot
cancel it anyway; and
 - it does not feel very safe to pass the tracker to this callback because I do
not own it. From implementation details of the tracker it seems like the tracker
must exist at the moment the reply is called but I am not sure that this must
always stay like this.

The core of the problem is that my task consists of two async subtasks run in a
sequence. CancelableTaskTracker does not seem to be well-suited for such tasks.

I see two suboptimal solutions:

 1) Change the signature of
Get1xFaviconForPageURLDownloadFromGoogleServerIfMissing so that it does not
support any canceling.

 2) Create a wrapper task that wraps the two subtasks and post it from
FaviconService to ThreadTaskRunnerHandle::Get(). This way, you are guaranteed
that the reply is not called after calling TryCancel(). Still, cancelling the
task while it is running will not have any impact on running the subtasks, both
of them will be run.

In both solutions I would need 
 - either another CancelableTaskTracker owned by the favicon service that I pass
to HistoryService (and never use for canceling anything),
 - or create an analogous function in the HistoryService that does not need any
CancelableTaskTracker.

WDYT?

[pkotwicz, 2017/02/07 03:11:44]

Solution #1 sounds ok to me. Especially since we likely aren't planning on
cancelling calls to Get1xFaviconForPageURLDownloadFromGoogleServerIfMissing()
yet

+  TRACE_EVENT0("browser",
+               "FaviconService::"
+               "RunFaviconImageCallbackOrDownloadFromGoogleFaviconServer");
+
+  // Ensure we do not get an icon upscaled from a smaller png than
+  // |minimum_size_in_pixel|.
+  bool large_enough = false;
+  for (const auto& bitmap : favicon_bitmap_results) {
+    if (minimum_size_in_pixel <= std::max(bitmap.pixel_size_in_db.width(),
+                                          bitmap.pixel_size_in_db.height())) {
+      large_enough = true;
+      return;
+    }
+  }
+
+  // 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 (favicon_bitmap_results.size() > 0 && !large_enough) {
+    callback.Run(favicon_base::FaviconImageResult());
+    return;
+  }
+
+  gfx::Image image;
+  image = favicon_base::SelectFaviconFramesFromPNGs(
+      favicon_bitmap_results, {1.0f}, desired_size_in_pixel);
+
+  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()) {
+    // No fetcher available (e.g. in unittests), return an empty result.
+    callback.Run(favicon_base::FaviconImageResult());
+    return;
+  }
+
+  // TODO(jkrcal): When we use the other server backend, require
+  // |minimum_size_in_pixel|.
+  int supported_size =
+      GetClosestGoogleFaviconServerSupportedSize(desired_size_in_pixel);
+  GURL image_url = GetGoogleFaviconServerURLForPageURL(page_url, client_id,
+                                              supported_size);
+
+  image_fetcher_->SetDataUseServiceName(data_use_service_name);
+  image_fetcher_->StartOrQueueNetworkRequest(
+      image_url.spec(), image_url,
+      base::Bind(&FaviconService::
+                     StoreFaviconFromGoogleServerAndRunFaviconImageCallback,
+                 base::Unretained(this), callback, page_url,
+                 desired_size_in_pixel));
+}
+
+void FaviconService::StoreFaviconFromGoogleServerAndRunFaviconImageCallback(
+      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());
+
+  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);
+  SetFavicons(page_url, image_result.icon_url,
+              favicon_base::IconType::FAVICON, image_result.image);
+  // Mark the icons 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