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..3a1e65b77939c9037647bf551c1de23addbd4bd2 100644
--- a/components/favicon/core/large_icon_service.cc
+++ b/components/favicon/core/large_icon_service.cc
@@ -4,39 +4,78 @@
 
 #include "components/favicon/core/large_icon_service.h"
 
+#include <algorithm>
 #include <memory>
 
 #include "base/bind.h"
+#include "base/feature_list.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/macros.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 "components/data_use_measurement/core/data_use_user_data.h"
 #include "components/favicon/core/favicon_service.h"
+#include "components/favicon/core/features.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"
 
+using image_fetcher::ImageFetcher;
+
+namespace favicon {
+
 namespace {
 
+const double kGoogleServerDefaultDesiredSize = 128;
+const double kGoogleServerMinSizeToDesiredSizeFactor = 2.0;
+const double kGoogleServerDesiredSizeToMaxSizeFactor = 2.0;
+
+const char kGoogleServer1RequestFormat[] =
+    "https://s2.googleusercontent.com/s2/"
+    "favicons?domain=%s&src=chrome_large_icons&sz=%d&alt=404";
+const int kGoogleServer1SupportedSizes[] = {16, 24, 32, 48, 64};
+
+// Type APPLE_TOUCH includes TOUCH_ICONs as well as FAVICONs as fallback.
+const char kGoogleServer2RequestFormat[] =
+    "https://t0.gstatic.com/"
+    "faviconV2?url=%s&type=APPLE_TOUCH&size=%d&min_size=%d&max_size=%d";

[pkotwicz, 2017/02/10 20:44:34]

Doesn't chrome want to prioritize FAVICONS over APPLE_TOUCH and TOUCH_ICONS?

[jkrcal, 2017/02/13 14:39:46]

The code for FaviconService also prioritizes APPLE_TOUCH, see the comment of
GetRawFaviconForPageURL():
  If favicon bitmaps for several IconTypes are available, the
  favicon bitmap is chosen in the priority of TOUCH_PRECOMPOSED_ICON,
  TOUCH_ICON and FAVICON.

Do you think we should change that? 

Anyway, prioritizing FAVICONS over APPLE_TOUCH is currently not supported by the
server. If we agree this is the way to go, can we post-pone it for a later CL?
(when the support on the server is added).

+
+int GetClosestSizeSupportedByGoogleServer1(int arbitrary_size) {
+  // Take the smallest size larger than |arbitrary_size|.
+  for (int size : kGoogleServer1SupportedSizes) {
+    if (size >= arbitrary_size)
+      return size;
+  }
+  // Or at least the largest available size.
+  return kGoogleServer1SupportedSizes[arraysize(kGoogleServer1SupportedSizes) -
+                                      1];
+}
+
 // 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,
+  LargeIconWorker(const GURL& page_url,
+                  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);
+                  base::CancelableTaskTracker* tracker,
+                  FaviconService* favicon_service,
+                  ImageFetcher* image_fetcher);
 
   // Must run on the owner (UI) thread in production.
   // Intermediate callback for GetLargeIconOrFallbackStyle(). Invokes
   // ProcessIconOnBackgroundThread() so we do not perform complex image
   // operations on the UI thread.
-  void OnIconLookupComplete(
+  void OnCachedIconLookupComplete(
       const favicon_base::FaviconRawBitmapResult& bitmap_result);
 
  private:
@@ -49,25 +88,46 @@ class LargeIconWorker : public base::RefCountedThreadSafe<LargeIconWorker> {
   // that does not work, computes the icon fallback style and uses it to
   // invoke |callback_|. This must be run on a background thread because image
   // resizing and dominant color extraction can be expensive.
-  void ProcessIconOnBackgroundThread();
+  void ProcessCachedIconOnBackgroundThread();
 
   // Must run on a background thread in production.
   // If |bitmap_result_| is square and large enough (>= |min_source_in_pixel_|),
   // resizes it to |desired_size_in_pixel_| (but if |desired_size_in_pixel_| is
   // 0 then don't resize). If successful, stores the resulting bitmap data
   // into |resized_bitmap_result| and returns true.
-  bool ResizeLargeIconOnBackgroundThreadIfValid(
+  bool ResizeCachedIconOnBackgroundThreadIfValid(
       favicon_base::FaviconRawBitmapResult* resized_bitmap_result);
 
   // Must run on the owner (UI) thread in production.
   // Invoked when ProcessIconOnBackgroundThread() is done.
-  void OnIconProcessingComplete();
+  void OnCachedIconProcessingComplete();
+
+  // Create the GET URL that requests the desired favicon from a Google favicon
+  // server (version 1 / version 2).
+  GURL GetIconUrlForGoogleServer1() const;
+  GURL GetIconUrlForGoogleServer2() const;
 
+  // Fetches an icon for the given |icon_url| and stores it in the favicon DB.
+  void FetchIconFromGoogleServer(const GURL& icon_url);
+  void OnFetchIconFromGoogleServerComplete(const std::string& icon_url,
+                                           const gfx::Image& image);
+
+  // Must run on a background thread in production.
+  void ProcessAndStoreIconFromGoogleServerOnBackgroundThread(
+      const GURL& icon_url,
+      const gfx::Image& image);
+
+  GURL page_url_;
   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_;
+  FaviconService* favicon_service_;
+  ImageFetcher* image_fetcher_;
+
+  bool icon_found_;
+  bool large_icon_found_;
   favicon_base::FaviconRawBitmapResult bitmap_result_;
   std::unique_ptr<favicon_base::LargeIconResult> result_;
 
@@ -75,35 +135,44 @@ class LargeIconWorker : public base::RefCountedThreadSafe<LargeIconWorker> {
 };
 
 LargeIconWorker::LargeIconWorker(
+    const GURL& page_url,
     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)
-    : min_source_size_in_pixel_(min_source_size_in_pixel),
+    base::CancelableTaskTracker* tracker,
+    FaviconService* favicon_service,
+    ImageFetcher* image_fetcher)
+    : page_url_(page_url),
+      min_source_size_in_pixel_(min_source_size_in_pixel),
       desired_size_in_pixel_(desired_size_in_pixel),
       callback_(callback),
       background_task_runner_(background_task_runner),
-      tracker_(tracker) {
-}
+      tracker_(tracker),
+      favicon_service_(favicon_service),
+      image_fetcher_(image_fetcher),
+      icon_found_(false),
+      large_icon_found_(false) {}
 
 LargeIconWorker::~LargeIconWorker() {
 }
 
-void LargeIconWorker::OnIconLookupComplete(
+void LargeIconWorker::OnCachedIconLookupComplete(
     const favicon_base::FaviconRawBitmapResult& bitmap_result) {
   bitmap_result_ = bitmap_result;
+  icon_found_ = bitmap_result_.is_valid();
   tracker_->PostTaskAndReply(
       background_task_runner_.get(), FROM_HERE,
-      base::Bind(&LargeIconWorker::ProcessIconOnBackgroundThread, this),
-      base::Bind(&LargeIconWorker::OnIconProcessingComplete, this));
+      base::Bind(&LargeIconWorker::ProcessCachedIconOnBackgroundThread, this),
+      base::Bind(&LargeIconWorker::OnCachedIconProcessingComplete, this));
 }
 
-void LargeIconWorker::ProcessIconOnBackgroundThread() {
+void LargeIconWorker::ProcessCachedIconOnBackgroundThread() {
   favicon_base::FaviconRawBitmapResult resized_bitmap_result;
-  if (ResizeLargeIconOnBackgroundThreadIfValid(&resized_bitmap_result)) {
+  if (ResizeCachedIconOnBackgroundThreadIfValid(&resized_bitmap_result)) {
     result_.reset(
         new favicon_base::LargeIconResult(resized_bitmap_result));
+    large_icon_found_ = true;
   } else {
     // Failed to resize |bitmap_result_|, so compute fallback icon style.
     std::unique_ptr<favicon_base::FallbackIconStyle> fallback_icon_style(
@@ -113,11 +182,11 @@ void LargeIconWorker::ProcessIconOnBackgroundThread() {
           bitmap_result_.bitmap_data, fallback_icon_style.get());
     }
     result_.reset(
-        new favicon_base::LargeIconResult(fallback_icon_style.release()));
+        new favicon_base::LargeIconResult(std::move(fallback_icon_style)));
   }
 }
 
-bool LargeIconWorker::ResizeLargeIconOnBackgroundThreadIfValid(
+bool LargeIconWorker::ResizeCachedIconOnBackgroundThreadIfValid(
     favicon_base::FaviconRawBitmapResult* resized_bitmap_result) {
   // Require bitmap to be valid and square.
   if (!bitmap_result_.is_valid() ||
@@ -156,19 +225,113 @@ bool LargeIconWorker::ResizeLargeIconOnBackgroundThreadIfValid(
   return true;
 }
 
-void LargeIconWorker::OnIconProcessingComplete() {
+void LargeIconWorker::OnCachedIconProcessingComplete() {
   callback_.Run(*result_);

[pkotwicz, 2017/02/10 20:44:34]

If we go and fetch results from the Google server shouldn't we call the
callback_ with those results?

[jkrcal, 2017/02/13 14:39:46]

This is conceptually a UX question to me. 

The way it is implemented now it allows the user to:
 - get the results (in this case fallback style) quickly so that the coloured
tile can be rendered quickly,
 - listen to HistoryService's icon updates and crossfade in the UI the true icon
if available any time later

If we wait for the response from the Google server, it may take a very long time
before the callback is called, especially on bad 2g networks. This way, the UI 
  - either leaves the icon area blank and, after the long delay, crossfade the
icon/colored-tile when it obtains the data from the callback;
  - or render a gray tile before calling the function and then, after the long
delay, cross-fade the icon/colored-tile when it obtains the data from the
callback.

Both solutions seem to be slightly worse to me than the current solution. I
haven't consulted with any UX expert, though.

WDYT?

[pkotwicz, 2017/02/13 21:33:31]

Given how you are planning on using this, I'd rather go back to the
implementation in https://codereview.chromium.org/2347173002/ I'd rather change
LargeIconService::GetLargeIconOrFallbackStyle() once many users of
LargeIconService::GetLargeIconOrFallbackStyle() download icons from the Google
server (my preference)

You could also pass two callbacks to
LargeIconService::GetLargeIconOrFallbackStyle(). One for when the favicon fetch
is completed and one for when the Google server fetch has completed

It is weird for LargeIconService::GetLargeIconOrFallbackStyle()
- to do fetching which is unrelated to the value passed to the callback.
- to keep on doing work after the callback has been called

[jkrcal, 2017/02/27 17:31:19]

Okay, the new functionality now does not get triggered by using the previously
existing function.

+
+  // If there is a large icon in the cache, we do not need to download anything.
+  if (large_icon_found_)
+    return;
+  // No fetcher available (e.g. in unittests), we cannot download anything.
+  if (!image_fetcher_)
+    return;
+
+  // No favicon in the cache. Download it from the server.
+
+  if (desired_size_in_pixel_ == 0) {
+    // We need to fix a concrete desired size if none is provided.
+    if (min_source_size_in_pixel_ > 0) {
+      desired_size_in_pixel_ =
+          min_source_size_in_pixel_ * kGoogleServerMinSizeToDesiredSizeFactor;
+    } else {
+      desired_size_in_pixel_ = kGoogleServerDefaultDesiredSize;
+    }
+  }
+
+  if (base::FeatureList::IsEnabled(kFaviconFetchLargeIconFromGoogleServer2)) {
+    FetchIconFromGoogleServer(GetIconUrlForGoogleServer2());
+  } else if (min_source_size_in_pixel_ <= 16 &&
+             base::FeatureList::IsEnabled(
+                 kFaviconFetchLargeIconFromGoogleServer1)) {
+    FetchIconFromGoogleServer(GetIconUrlForGoogleServer1());
+  }
 }
 
-}  // namespace
+GURL LargeIconWorker::GetIconUrlForGoogleServer1() const {
+  int supported_size =
+      GetClosestSizeSupportedByGoogleServer1(desired_size_in_pixel_);
+  return GURL(base::StringPrintf(kGoogleServer1RequestFormat,
+                                 page_url_.spec().c_str(), supported_size));
+}
 
-namespace favicon {
+GURL LargeIconWorker::GetIconUrlForGoogleServer2() const {

[pkotwicz, 2017/02/10 20:44:34]

Different bits of UI want differently sized favicons. For instance, bookmarks on
Android want favicons of size 16x16dp (48x48px on a Nexus 5). The new tab tiles
want favicons of size 48x48dp (144x144px on a Nexus 5).

If a user:
Visits the bookmarks page
then
Visits the new tab page

It is possible that the favicons fetched and put into the database for the
bookmarks page are too small of the new tab page. Thus when the user visits the
new tab page the user will still see crappy icons despite LargeIconService
having fetched icons from the Google server  for the bookmarks page

I suggest hard coding the minimum size and the maximum size for icons fetched
from the Google server. The size fetched from the server will be unrelated to
the size requested by the caller of LargeIconService

Perhaps hard code
min size to 16 dp
and max size to 48 dp

This means that the server needs to be smarter. I don't know if there is a way
to indicate to the server
- to prefer a larger size
- return a smaller size if available

[jkrcal, 2017/02/13 14:39:46]

Good point! I've fixed min, max, and desired sizes in pixels as constants. I do
not think it is appropriate at the moment to compute these sizes from dpi sizes
and from the scale factor of the device. The reason is that icons >= 48px are
still not very wide-spread so we should be very careful about imposing larger
min_size constraints.

The server is already smart about it:
 - it never returns anything out of range of [min, max]
 - if there are images of size >= desired_size, it returns the smallest of them
(i.e. with size closest to desired_size)
 - otherwise, if there are images of size < desired_size, it returns the largest
of them.

[pkotwicz, 2017/02/13 21:33:32]

Thank you for pointing this out. I didn't see the desired size parameter in the
server URL

+  int max_source_size_in_pixel =
+      desired_size_in_pixel_ * kGoogleServerDesiredSizeToMaxSizeFactor;
+  return GURL(
+      base::StringPrintf(kGoogleServer2RequestFormat, page_url_.spec().c_str(),
+                         desired_size_in_pixel_, min_source_size_in_pixel_,
+                         max_source_size_in_pixel));
+}
+
+void LargeIconWorker::FetchIconFromGoogleServer(const GURL& icon_url) {
+  // Do not retry if there is a previous cache miss recorded for |icon_url|.
+  if (favicon_service_->WasUnableToDownloadFavicon(icon_url))
+    return;
+
+  // TODO(jkrcal): Create a large icon service tag.
+  image_fetcher_->SetDataUseServiceName(
+      data_use_measurement::DataUseUserData::NOT_TAGGED);
+  image_fetcher_->StartOrQueueNetworkRequest(
+      icon_url.spec(), icon_url,
+      base::Bind(&LargeIconWorker::OnFetchIconFromGoogleServerComplete,
+                 base::Unretained(this)));

[pkotwicz, 2017/02/10 20:44:34]

Don't use base::Unretained(). LargeIconWorker is ref counted. Using
base::Unretained() makes this code crash

[jkrcal, 2017/02/13 14:39:46]

Done.

+}
+
+void LargeIconWorker::OnFetchIconFromGoogleServerComplete(
+    const std::string& icon_url,
+    const gfx::Image& image) {
+  if (image.IsEmpty()) {
+    favicon_service_->UnableToDownloadFavicon(GURL(icon_url));
+    return;
+  }
+
+  tracker_->PostTask(
+      background_task_runner_.get(), FROM_HERE,
+      base::Bind(&LargeIconWorker::
+                     ProcessAndStoreIconFromGoogleServerOnBackgroundThread,
+                 base::Unretained(this), GURL(icon_url), image));

[pkotwicz, 2017/02/10 20:44:34]

Don't use base::Unretained(). LargeIconWorker is ref counted. Using
base::Unretained() makes this code crash

[jkrcal, 2017/02/13 14:39:46]

Done.

+}
+
+void LargeIconWorker::ProcessAndStoreIconFromGoogleServerOnBackgroundThread(
+    const GURL& icon_url,
+    const gfx::Image& image) {
+  SkBitmap icon = image.AsBitmap();

[pkotwicz, 2017/02/10 20:44:34]

I think that it would be better to store the non-resized icon in the database.
(Perhaps we want to resize if the icon is too big) A non-resized icon will
resize more nicely to the other sizes that you want.

You should however return to the callback a resized version of the icon

[jkrcal, 2017/02/13 14:39:45]

Done.

+  SkBitmap resized_icon = favicon_base::ResizeBitmapByDownsamplingIfPossible(
+      {icon}, desired_size_in_pixel_);
+  gfx::Image resized_image = gfx::Image::CreateFrom1xBitmap(resized_icon);
+  favicon_base::SetFaviconColorSpace(&resized_image);
+
+  // We must make sure we do not overwrite the FAVICON if there is any icon in
+  // the cache (due to sync).
+  favicon_base::IconType store_as_type =
+      icon_found_ ? favicon_base::IconType::TOUCH_ICON
+                  : favicon_base::IconType::FAVICON;
+
+  favicon_service_->SetFavicons(page_url_, icon_url, store_as_type,
+                                resized_image);
+  // Mark the icons as out-of-date so that they are refetched when we visit the
+  // original page any time in the future.
+  favicon_service_->SetFaviconOutOfDateForPage(page_url_);
+}
+
+}  // 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);
@@ -184,20 +347,21 @@ base::CancelableTaskTracker::TaskId
         int desired_size_in_pixel,
         const favicon_base::LargeIconCallback& callback,
         base::CancelableTaskTracker* tracker) {
-  DCHECK_LE(1, min_source_size_in_pixel);
+  DCHECK_LE(0, min_source_size_in_pixel);
   DCHECK_LE(0, desired_size_in_pixel);
 
-  scoped_refptr<LargeIconWorker> worker =
-      new LargeIconWorker(min_source_size_in_pixel, desired_size_in_pixel,
-                          callback, background_task_runner_, tracker);
+  scoped_refptr<LargeIconWorker> worker = new LargeIconWorker(
+      page_url, min_source_size_in_pixel, desired_size_in_pixel, callback,
+      background_task_runner_, tracker, favicon_service_, image_fetcher_.get());
 
-  // 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.
+  // TODO(jkrcal): For now this is just a wrapper around
+  // GetLargestRawFaviconForPageURL. Consider extending
+  // GetRawFaviconForPageURL() by min_original_size_in_pixel and migrating
+  // there. Otherwise, add the logic required to select the best possible large
+  // icon.

[pkotwicz, 2017/02/10 20:44:34]

You should use CancelableTaskTracker::NewTrackedTaskId() to get the task id and
use the callback in the output parameter to determine whether the operation has
been canceled.

[jkrcal, 2017/02/13 14:39:45]

Well, I noticed this function would give me what I need here. I got scared off
by the comment:
  // Note. This function is used to address some special 
  // cancelation requirement in existing code. You SHOULD NOT need 
  // this function in new code.

I do not want to use deprecated functions in new code. WDYT?

[pkotwicz, 2017/02/13 21:33:31]

I didn't see that comment. Looks like the author of the comment (introduced in
https://codereview.chromium.org/11414041) ignored it
https://chromiumcodereview.appspot.com/11441006

   return favicon_service_->GetLargestRawFaviconForPageURL(

[pkotwicz, 2017/02/10 20:44:34]

You can't use GetLargestRawFaviconForPageURL() here because
GetLargestRawFaviconForPageURL() sometimes returns no results at all when there
is data in the database. This function should not under any circumstance
overwrite valid data in the database.

[jkrcal, 2017/02/13 14:39:46]

I am fine with using GetRawFaviconForPageURL() and filtering here.

I am not fine with it prioritizing exact match of type in sorting the results.
If I ask for a 120px icon with type TOUCH_ICON | TOUCH_ICON_PRECOMPOSED |
FAVICON, I would much prefer to get a FAVICON of size 96px over a TOUCH_ICON of
size 60px on the first position. Would you be fine with changing that logic that
orders results from the database (in some later CL)?

[pkotwicz, 2017/02/13 21:33:31]

I have forgotten about the filtering in
ThumbnailDatabase::GetIconMappingsForPageURL(). Yes, we should change the logic
to
1) Filter based on the best size
then
2) Filter so that we have just one icon type

Historic note: the logic in HistoryBackend and ThumbnailDatabase was written to
handle TOUCH_ICON | TOUCH_ICON_PRECOMPOSED not TOUCH_ICON |
TOUCH_ICON_PRECOMPOSED | FAVICON

[jkrcal, 2017/02/27 17:31:19]

Can we leave that for a further CL?

[pkotwicz, 2017/02/28 01:51:56]

Doing this in a follow up CL is OK

       page_url, large_icon_types_, min_source_size_in_pixel,
-      base::Bind(&LargeIconWorker::OnIconLookupComplete, worker),
+      base::Bind(&LargeIconWorker::OnCachedIconLookupComplete, worker),
       tracker);
 }