Extend LargeIconService to fetch missing favicons from a Google server

components/favicon/core/large_icon_service.cc

Index: components/favicon/core/large_icon_service.cc
diff --git a/components/favicon/core/large_icon_service.cc b/components/favicon/core/large_icon_service.cc
index fa42fe4d8f54f4189fc24d6595e9cd38b4369110..c32512a6c4ab57b84514cd3ee3418bb2abb21b7c 100644
--- a/components/favicon/core/large_icon_service.cc
+++ b/components/favicon/core/large_icon_service.cc
@@ -10,18 +10,42 @@
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
+#include "base/strings/stringprintf.h"
 #include "base/task_runner.h"
 #include "base/threading/sequenced_worker_pool.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "components/data_use_measurement/core/data_use_user_data.h"
 #include "components/favicon/core/favicon_service.h"
 #include "components/favicon_base/fallback_icon_style.h"
 #include "components/favicon_base/favicon_types.h"
+#include "components/favicon_base/favicon_util.h"
+#include "components/image_fetcher/image_fetcher.h"
 #include "skia/ext/image_operations.h"
 #include "ui/gfx/codec/png_codec.h"
 #include "ui/gfx/geometry/size.h"
 
+namespace favicon {
 namespace {
 
+using image_fetcher::ImageFetcher;

[pkotwicz, 2017/03/03 22:33:25]

Do you need this using statement? Personally, I am only in favor using the
"using" keyword if it is for a class which is used a lot. (My personal
preference probably differs from the Google style guide) There is a single use
of ImageFetcher in this file

[mastiz, 2017/03/06 10:25:33]

Removed. I'm also not a big fan of using declarations unless there are
sufficient references.

+
+// Type APPLE_TOUCH includes TOUCH_ICONs as well as FAVICONs as fallback.

[pkotwicz, 2017/03/03 22:33:25]

Can you please mention in the comment whether TOUCH_ICON includes
apple-touch-icon-precomposed too?

[mastiz, 2017/03/06 10:25:33]

Done. I also added a TODO because preference towards touch icons is questionable
outside iOS.

+const char kGoogleServerV2RequestFormat[] =
+    "https://t0.gstatic.com/"
+    "faviconV2?url=%s&type=APPLE_TOUCH&size=%d&min_size=%d&max_size=%d";

[pkotwicz, 2017/03/03 22:33:25]

I tested this URL in the browser. Is it possible to customize the response in
the case that the server has no data? It looks like a 16x16 icon with a picture
of the earth is returned if the server could not find any data

[mastiz, 2017/03/06 10:25:33]

jkrcal@ will talk to bing to disable this on the server-side. We'll need a final
confirmation from the server owners that the URL we're using is fine.

+const int kGoogleServerV2MaxSizeInPixel = 256;
+const int kGoogleServerV2DesiredSizeInPixel = 192;
+
+GURL GetIconUrlForGoogleServerV2(const GURL& page_url,
+                                 int min_source_size_in_pixel) {

[pkotwicz, 2017/03/03 22:33:25]

Don't you want to hard code the minimum size requested from the server?

If there are two callers to
LargeIconService::GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache()
First callers minimum_size = 64
Second caller minimum_size = 16
The contents of the favicon database for page_url will be affected by the
minimum size specified in

[mastiz, 2017/03/06 10:25:33]

We actually have clients with different requirements, so hardcoding is not
possible (unless we hardcode a very small number, leading to unnecessary
downloads).

What do you think is the actual issue?
a) That the first URL returned a 404 that will affect the second? --> this is
not possible because the 404 cache remembers the exact URL including size
restricts.

b) That the second caller will get a big icon? --> this is going to happen
anyway because we request the largest one available
(kGoogleServerV2DesiredSizeInPixel).

[pkotwicz, 2017/03/08 05:38:16]

I see your point now

+  return GURL(base::StringPrintf(
+      kGoogleServerV2RequestFormat, page_url.spec().c_str(),
+      kGoogleServerV2DesiredSizeInPixel, min_source_size_in_pixel,
+      kGoogleServerV2MaxSizeInPixel));
+}
+
 // Processes the bitmap data returned from the FaviconService as part of a
 // LargeIconService request.
 class LargeIconWorker : public base::RefCountedThreadSafe<LargeIconWorker> {
@@ -160,15 +184,43 @@ void LargeIconWorker::OnIconProcessingComplete() {
   callback_.Run(*result_);
 }
 
-}  // namespace
+void OnFetchIconFromGoogleServerComplete(
+    FaviconService* favicon_service,
+    const GURL& page_url,
+    const base::Callback<void(bool success)>& callback,
+    const std::string& icon_url,
+    const gfx::Image& image) {
+  if (image.IsEmpty()) {
+    DLOG(WARNING) << "large icon server fetch empty " << icon_url;
+    favicon_service->UnableToDownloadFavicon(GURL(icon_url));
+    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                  base::Bind(callback, false));
+    return;
+  }
 
-namespace favicon {
+  // TODO(jkrcal): Extract the original icon url from the response headers if
+  // available and use it instead of |icon_url|.

[pkotwicz, 2017/03/03 22:33:25]

Please add a TODO for getting the icon type too

[mastiz, 2017/03/06 10:25:33]

What would we use this for? I thought we agreed TOUCH_ICON would always be used
(to avoid mangling with sync), marking them as expired such that they will be
replaced by future visits to the page.

This would presumably require some custom header param so it feels a bit wrong.
I'll anyway get the conversation started with bing.

[pkotwicz, 2017/03/08 05:38:16]

I suggested the TODO because always storing as a TOUCH_ICON is a hack because
the icon may not be a touch icon.

We should at the very least have a comment saying that this is a hack and why
the TOUCH_ICON type was chosen (because unlike the FAVICON type there is no
requirement for the size of the icons of type TOUCH_ICON in the database)

[mastiz, 2017/03/08 16:37:33]

Extended the TODO with this part and clarified the current hackiness.

+
+  // Write fetched icons to FaviconService's cache, but only if no icon was
+  // available (clients are encouraged to do this in advance, but meanwhile
+  // something else could've been written). By marking the icons initially
+  // expired (out-of-date), they will be refetched when we visit the original
+  // page any time in the future.
+  favicon_service->SetExpiredFaviconsIfNoneKnown(
+      page_url, GURL(icon_url), favicon_base::IconType::TOUCH_ICON, image);
+  base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                base::Bind(callback, true));
+}
+
+}  // namespace
 
 LargeIconService::LargeIconService(
     FaviconService* favicon_service,
-    const scoped_refptr<base::TaskRunner>& background_task_runner)
+    const scoped_refptr<base::TaskRunner>& background_task_runner,
+    std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher)
     : favicon_service_(favicon_service),
-      background_task_runner_(background_task_runner) {
+      background_task_runner_(background_task_runner),
+      image_fetcher_(std::move(image_fetcher)) {
   large_icon_types_.push_back(favicon_base::IconType::FAVICON);
   large_icon_types_.push_back(favicon_base::IconType::TOUCH_ICON);
   large_icon_types_.push_back(favicon_base::IconType::TOUCH_PRECOMPOSED_ICON);
@@ -201,4 +253,30 @@ base::CancelableTaskTracker::TaskId
       tracker);
 }
 
+void LargeIconService::
+    GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
+        const GURL& page_url,
+        int min_source_size_in_pixel,
+        const base::Callback<void(bool success)>& callback) {
+  DCHECK_LE(0, min_source_size_in_pixel);
+
+  const GURL icon_url =
+      GetIconUrlForGoogleServerV2(page_url, min_source_size_in_pixel);
+
+  // Do not retry if there is a previous cache miss recorded for |icon_url|.

[pkotwicz, 2017/03/03 22:33:25]

How about: "Do not do download if there is a previous cache miss ..."

[mastiz, 2017/03/06 10:25:33]

Done.

+  if (!image_fetcher_ ||
+      favicon_service_->WasUnableToDownloadFavicon(icon_url)) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                  base::Bind(callback, false));
+    return;
+  }
+
+  image_fetcher_->SetDataUseServiceName(
+      data_use_measurement::DataUseUserData::LARGE_ICON_SERVICE);
+  image_fetcher_->StartOrQueueNetworkRequest(
+      icon_url.spec(), icon_url,
+      base::Bind(&OnFetchIconFromGoogleServerComplete, favicon_service_,
+                 page_url, callback));
+}
+
 }  // namespace favicon