Extend LargeIconService to fetch missing favicons from a Google server

components/favicon/core/large_icon_service.cc

 // Copyright 2015 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/large_icon_service.h"
 
 #include <memory>
 
 #include "base/bind.h"
 #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> {
  public:
   LargeIconWorker(int min_source_size_in_pixel,
                   int desired_size_in_pixel,
                   favicon_base::LargeIconCallback callback,
                   scoped_refptr<base::TaskRunner> background_task_runner,
                   base::CancelableTaskTracker* tracker);
 
@@ skipping 118 matching lines @@
       gfx::Size(desired_size_in_pixel_, desired_size_in_pixel_);
   resized_bitmap_result->bitmap_data =
       base::RefCountedBytes::TakeVector(&bitmap_data);
   return true;
 }
 
 void LargeIconWorker::OnIconProcessingComplete() {
   callback_.Run(*result_);
 }
 
+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;
+  }
+
+  // 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
 
-namespace favicon {
-
 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);
 }
 
 LargeIconService::~LargeIconService() {
 }
 
 base::CancelableTaskTracker::TaskId
     LargeIconService::GetLargeIconOrFallbackStyle(
@@ skipping 12 matching lines @@
   // TODO(beaudoin): For now this is just a wrapper around
   //   GetLargestRawFaviconForPageURL. Add the logic required to select the best
   //   possible large icon. Also add logic to fetch-on-demand when the URL of
   //   a large icon is known but its bitmap is not available.
   return favicon_service_->GetLargestRawFaviconForPageURL(
       page_url, large_icon_types_, min_source_size_in_pixel,
       base::Bind(&LargeIconWorker::OnIconLookupComplete, worker),
       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