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..e6f666882807ec95a043d9f61ccaf6b57075fbb4 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() {
 }
@@ -199,6 +230,30 @@ base::CancelableTaskTracker::TaskId FaviconService::GetFaviconForPageURL(
 }
 
 base::CancelableTaskTracker::TaskId
+FaviconService::GetRawFaviconForPageURLDownloadFromGoogleServerIfNeeded(
+    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) {
+  TRACE_EVENT0("browser",
+               "FaviconService::"
+               "GetRawFaviconForPageURLDownloadFromGoogleServerIfNeeded");
+  std::vector<int> desired_sizes_in_pixel;
+  desired_sizes_in_pixel.push_back(desired_size_in_pixels);
+  return GetFaviconForPageURLImpl(
+      page_url, favicon_base::IconType::FAVICON, desired_sizes_in_pixel,
+      base::Bind(&FaviconService::
+                     RunFaviconImageCallbackOrDownloadFromGoogleFaviconServer,
+                 base::Unretained(this), page_url, minimum_size_in_pixels,
+                 desired_size_in_pixels, 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 +403,83 @@ void FaviconService::RunFaviconRawBitmapCallbackWithBitmapResults(
       ResizeFaviconBitmapResult(favicon_bitmap_results, desired_size_in_pixel));
 }
 
+void FaviconService::RunFaviconImageCallbackOrDownloadFromGoogleFaviconServer(
+    const GURL& page_url,
+    int minimum_size_in_pixels,
+    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");
+  // TODO(jkrcal): Ensure we do not get an icon upscaled from a smaller png than
+  // |minimum_size_in_pixels|. Is extending SelectFaviconFramesFromPNGs() the
+  // simplest path? Or should I do it similarly to GetLargestRawFaviconForID()?

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

Peter, can you help me with this, please?

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

Things get complicated if you want to download a favicon from the google service
if there is a favicon in the database but it is smaller than
|minimum_size_in_pixels|

For instance, you need to worry about whether you want to store the downloaded
favicon into the database. If yes, you cannot clobber the existing data
These things can be solved but over the course of many CLs not one CL

I suggest doing the download only if there is nothing in the database

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

I do not want to download anything in such a case. The expected behaviour is
documented in the .h file.
 
I still would like to find out what is the size of the largest cached icon. 
 - If it does not meet the minimum requested size, the function will return an
empty gfx::Image().
 - If it meets the minimum requested size, it should return this cached favicon
scaled to desired_size.

How can I find out the largest size with duplicating much of work?

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

I recomment adding FaviconRawBitmapResult::width_in_db and
FaviconRawBitmapResult::height_in_db to the FaviconRawBitmapResult struct

[jkrcal, 2016/12/15 18:17:27]

Thanks for the hint! 
When looking in the code, I identified that these new fields need to be set in
HistoryBackend::GetFaviconBitmapResultsForBestMatch() and in
HistoryBackend::GetLargestFaviconForURL(). Does it SG?

More importantly, I do not understand what is the difference between
FaviconRawBitmapResult::width/height_in_db and
FaviconRawBitmapResult::pixel_size. Why cannot I use the latter to find out what
is the size of the largest icon available?

[pkotwicz, 2016/12/15 18:43:57]

For the sake of clarity, I recommend adding width_in_db and height_in_db not
renaming/replacing FaviconRawBitmapResult::width and
FaviconRawBitmapResult::height

[pkotwicz, 2016/12/15 18:43:57]

Sorry for the confusion.

FaviconRawBitmapResult::pixel_size contains the size of the bitmap after it was
resized by FaviconService.

We can add FaviconRawBitmapResult::pixel_size_in_db which would contain the size
of the bitmap before it was resized by HistoryBackend::GetFaviconsForURL().

FaviconRawBitmapResult::pixel_size_in_db might be different than the favicon's
size on the server because we store favicons into the database after resizing.
This should be ok because we either: store the bitmap without resizing (Android)
or store the bitmap after resizing to 32x32 and 16x16 (desktop). If the minimum
size is > 32x32 we are guaranteed that the matching favicon would not have been
resized

I am not sure, but we might be able to replace all uses of
FaviconRawBitmapResult::pixel_size with FaviconRawBitmapResult::pixel_size_in_db

[jkrcal, 2016/12/16 12:53:52]

Thanks for the clarification!

I did not want to mess it up, so I rather introduced the new member. Should I
add a "TODO(pkotwicz, jkrcal): Consider removing |pixel_size| and replacing all
usage by |pixel_size_in_db|"?

+
+  gfx::Image image = favicon_base::SelectFaviconFramesFromPNGs(
+      favicon_bitmap_results,
+      {static_cast<float>(desired_size_in_pixel) / gfx::kFaviconSize},
+      gfx::kFaviconSize);
+
+  if (!image.IsEmpty()) {
+    // Serve the favicon from the cache.
+    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 favicon in the cache. Download it from the server.
+
+  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(
+      image_url.spec(), image_url,
+      base::Bind(&FaviconService::
+                     StoreFaviconFromGoogleServiceAndRunFaviconImageCallback,
+                 base::Unretained(this), callback, page_url));
+}
+
+void FaviconService::StoreFaviconFromGoogleServiceAndRunFaviconImageCallback(
+      const favicon_base::FaviconImageCallback& callback,
+      const GURL& page_url,
+      const std::string& icon_url,
+      const gfx::Image& image) {
+  if (image.IsEmpty())
+    callback.Run(favicon_base::FaviconImageResult());
+
+  // TODO(jkrcal): No rescaling to desired_size_in_px. Should I fetch raw data
+  // from ImageFetcher, construct
+  // std::vector<favicon_base::FaviconRawBitmapResult> and use
+  // SelectFaviconFramesFromPNGs().

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

Peter, can you help me with this, please?

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

My understanding is that you are currently not rescaling to
|desired_size_in_px|. Do you want to rescale to |desired_size_in_px|?

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

Sorry, the comment is a bit unclear :) Yes, I would like to rescale to desired
size for the sake of consistency. 
(Do I get it right that other similar functions in FaviconService do rescale to
the desired size?)

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

You are right. Similar functions in FaviconService do rescale to the desired
size.

Looking at image_decoder_impl it looks like the gfx::Image is created from a
single SkBitmap.

You can get the SkBitmap via gfx::Image::AsBitmap() and then call
ResizeBitmapByDownsamplingIfPossible() in favicon_util.cc The function is
currently in the anonymous namespace but you can move it out

[jkrcal, 2016/12/15 18:17:27]

Done, thanks!

+
+  favicon_base::FaviconImageResult image_result;
+  image_result.icon_url = GURL(icon_url);
+  image_result.image = image;
+  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