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..82ae94d7cc49f54b567f2105d479d1f0b2f72fa9 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,29 @@
 namespace favicon {
 namespace {
 
+const char kGoogleFaviconServiceRequestFormat[] =
+    "https://s2.googleusercontent.com/s2/"
+    "favicons?domain=%s&src=chrome_newtab_mobile&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)

[Marc Treib, 2016/09/19 08:26:39]

I think this should be >=

[noyau (Ping after 24h), 2016/09/19 09:00:50]

Definitely should be >=. I'm surprised that there is no unitttests whatsoever on
this service. It probably need some...

[jkrcal, 2016/09/20 15:52:06]

Done.

+      return size;
+  }
+  // Or at least the largest available size.
+  return kGoogleFaviconServiceSupportedSizes
+      [arraysize(kGoogleFaviconServiceSupportedSizes) - 1];
+}
+
+GURL GetGoogleFaviconServiceURL(GURL domain_url, int supported_size) {
+  return GURL(base::StringPrintf(kGoogleFaviconServiceRequestFormat,
+                                 domain_url.spec().c_str(),
+                                 supported_size));
+}
+
 // Helper to run callback with empty results if we cannot get the history
 // service.
 base::CancelableTaskTracker::TaskId RunWithEmptyResultAsync(
@@ -51,10 +76,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 +226,32 @@ base::CancelableTaskTracker::TaskId FaviconService::GetFaviconForPageURL(
       tracker);
 }
 
+void FaviconService::DownloadFromGoogleServerFaviconImageForPageURL(
+    const GURL& domain_url,
+    int desired_size_in_pixel,
+    std::string client_id,

[pkotwicz, 2016/09/19 20:07:51]

|client_id| seems unused. Do you need to add a TODO()

[jkrcal, 2016/09/20 15:52:06]

Oh, my mistake, thanks! Used now.

+    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 domain_url_sanitized = domain_url.GetWithEmptyPath();
+  GURL image_url =
+      GetGoogleFaviconServiceURL(domain_url_sanitized, 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, domain_url_sanitized));
+}
+
 base::CancelableTaskTracker::TaskId
 FaviconService::UpdateFaviconMappingsAndFetch(
     const GURL& page_url,
@@ -348,4 +402,29 @@ void FaviconService::RunFaviconRawBitmapCallbackWithBitmapResults(
       ResizeFaviconBitmapResult(favicon_bitmap_results, desired_size_in_pixel));
 }
 
+void FaviconService::StoreFaviconFromGoogleServiceAndRunFaviconImageCallback(
+      const favicon_base::FaviconImageCallback& callback,
+      const GURL& domain_url,
+      const std::string& icon_url,
+      const gfx::Image& image) {
+  favicon_base::FaviconImageResult image_result;
+  image_result.image = image;
+  if (image_result.image.IsEmpty()) {
+    image_result.icon_url = GURL();

[pkotwicz, 2016/09/19 20:07:51]

The "icon URL" is used as a key in the database. Use |domain_url| as the "icon
URL" to get something unique. (Otherwise,
StoreFaviconFromGoogleServiceAndRunFaviconImageCallback() will clobber whatever
it has last stored in the database)

[jkrcal, 2016/09/20 15:52:06]

I do not understand what you mean. Maybe I puzzled you by the missing "return"
statement? This image_result.icon_url only returns back to the caller, does not
go into the database. Still, it maybe makes sense to fill in the used icon_url.

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

Sorry my bad. I did not realize that |icon_url| is passed into this function

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

No problem :)

+    callback.Run(image_result);
+  }
+
+  image_result.icon_url = GURL(icon_url);
+  favicon_base::SetFaviconColorSpace(&image_result.image);
+
+  // Store the favicon in the cache.
+  SetFavicons(domain_url, image_result.icon_url,
+              favicon_base::IconType::FAVICON, image_result.image);

[noyau (Ping after 24h), 2016/09/19 09:00:50]

The documentation for SetFavicons is very explicit: "It is important that
|image| contains image reps for all of ui::GetSupportedScaleFactors()". This is
not the case here, at least on iOS. The image is 1x, the supported scale factor
is 2x or 3x. On Android, there could be more than one supported scale factor if
the device is hi-dpi. Desktop always have more than one scale factor (1x and
2x).

Also I'm sad that after all these efforts to fetch images directly as UIImage,
those are saved as ImageSkia bitmap representation, incurring a transform on
save and another on fetch.

[jkrcal, 2016/09/20 15:52:06]

I am not sure how to address your comment. 
 - Do you oppose the idea that this function fetches only one image? (which is
important as regards data usage for users from emerging markets) 

 - Do oppose the way I call SetFavicons with this one image? If so, would you
prefer me:
 * getting the raw png data, 
 * fill it in a favicon_base::FaviconRawBitmapResult raw_image, and
 * call favicon_base::SelectFaviconFramesFromPNGs(
      std::vector<favicon_base::FaviconRawBitmapResult>{raw_image},
      favicon_base::GetFaviconScales(),
      16)?
I do not get this comment. Does it target the SetFavicons API (and not my CL)?

[noyau (Ping after 24h), 2016/09/21 09:49:20]

I'm not sure how to solve the issue, pkotwicz@ is more qualified than me to
provide an answer to this, or to tell you that your proposal is fine. I was
simply pointing out that your use of SetFavicons seems to go against the spirit
of it as explained in its comment. 
No, I'm just sad that this callback is going to receive a gfx::Image with an
UIImage as its underlying represnetation and that the code saving the image will
generate a bitmap from it to save to disk. The only way to fix this would be to
have the favicon save its data in a platform specific way, and this is
completely ouside the scope of this CL. But still, this makes me sad.

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

noyau: Not having image reps for all of ui::GetSupportedScaleFactors() should be
OK since the icon is stored as expired

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

Acknowledged.

+  // Mark them as out-of-date so that they are refetched when we visit the
+  // original page any time in the future.
+  SetFaviconOutOfDateForPage(domain_url);

[pkotwicz, 2016/09/19 20:07:51]

- favicon_base::FAVICON is only meant to store 16x16 DIP (device independant
pixel) favicons. The 16x16px favicons are used by sync. Perhaps these should be
stored as favicon_base::TOUCH_ICON instead? I think that
favicon_base::TOUCH_ICON is mobile only. Are we planning on using
DownloadFromGoogleServerFaviconImageForPageURL() on non-mobile?
- I am concerned that people will use
DownloadFromGoogleServerFaviconImageForPageURL() and that will overwrite an
existing favicon stored for the page. Maybe add a warning in the comment for
DownloadFromGoogleServerFaviconImageForPageURL() ?

For a given page URL the database only supports storing one icon per
favicon_base::IconType

The other FaviconService methods such as
FaviconService#GetFaviconImageForPageURL() call the callback with the image
resized to the pixel size requested in the caller. Is there a reason that this
method does not resize the image to the pixel size passed to
DownloadFromGoogleServerFaviconImageForPageURL() ?

[jkrcal, 2016/09/20 15:52:06]

My observations:
 - If I correctly understand what does IconType::FAVICON mean, the Google server
provides only these icons (possibly in 1x, 1.5x, 2x, 2.5x, and 3x scale
factors).
 - This function is meant to be used only if the local cache does not contain
any FAVICON images. I've added the warning about overwriting existing entries.
 - In the near future (say next 6 months), I see only mobile use of this
function from our side. Still I do not think that icons that are conceptually of
the FAVICON type should be stored as TOUCH_ICONs.
Good point, I plan to change it (once I get more feedback above).

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

We only sync favicons for bookmarks. It is possible (but unlikely) that:
- One of the most-visited sites on the NTP is bookmarked
- We do not have a favicon for the bookmarked page
- There is an icon for the site in gstatic

In StoreFaviconFromGoogleServiceAndRunFaviconImageCallback() I would recommend:
- constructing a multi-resolution gfx::ImageSkia with:
  - the fetched bitmap
  - the fetched bitmap resized to 16x16
Ideally you would download both the
|desired_size_in_pixel|x|desired_size_in_pixel| and the 16x16 images from the
Google server. However, not doing so should be OK because you are setting the
stored favicon as expired (For the sake of clarity I still think that you should
construct a multi-resolution gfx::ImageSkia)

Storing icons with the FAVICON type should be ok.

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

As per offline discussion with pkotwicz@, only one size is ok.

+
+  callback.Run(image_result);
+}
+
 }  // namespace favicon