Created params class for LargeIconService and added a desktop client

The params class sets default parameters for specific platforms, particularly desktop (which should not need to specify min size and desired size, and should store icons as IconType::kFavicon). Also added a separate Google Favicon Service client to be used on desktop. These will be used in the upcoming NTP redesign.

Bug: 837798
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: Ib1a78c49a2518acffdc8f1f7c746a751377cfb45
Reviewed-on: https://chromium-review.googlesource.com/1031562Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Commit-Queue: Kristi Park <kristipark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556922}

chrome/browser/android/bookmarks/partner_bookmarks_reader.cc

@@ -15,6 +15,7 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "components/bookmarks/browser/bookmark_model.h"
+#include "components/favicon/core/favicon_server_fetcher_params.h"
 #include "components/favicon/core/favicon_service.h"
 #include "components/favicon/core/large_icon_service.h"
 #include "components/favicon_base/favicon_types.h"
@@ -313,9 +314,10 @@ void PartnerBookmarksReader::OnGetFaviconFromCacheFinished(
         })");
   GetLargeIconService()
       ->GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
-          page_url, kPartnerBookmarksMinimumFaviconSizePx,
-          desired_favicon_size_px, false /* may_page_url_be_private */,
-          traffic_annotation,
+          favicon::FaviconServerFetcherParams::CreateForMobile(
+              page_url, kPartnerBookmarksMinimumFaviconSizePx,
+              desired_favicon_size_px),
+          false /* may_page_url_be_private */, traffic_annotation,
           base::Bind(&PartnerBookmarksReader::OnGetFaviconFromServerFinished,
                      base::Unretained(this), page_url, desired_favicon_size_px,
                      base::Passed(std::move(callback))));

components/favicon/core/BUILD.gn

@@ -14,6 +14,8 @@ static_library("core") {
     "favicon_driver_observer.h",
     "favicon_handler.cc",
     "favicon_handler.h",
+    "favicon_server_fetcher_params.cc",
+    "favicon_server_fetcher_params.h",
     "favicon_service.cc",
     "favicon_service.h",
     "favicon_service_impl.cc",

components/favicon/core/favicon_server_fetcher_params.cc

+// Copyright (c) 2018 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/favicon_server_fetcher_params.h"
+
+#include "base/logging.h"
+#include "base/memory/ptr_util.h"
+#include "components/favicon_base/favicon_util.h"
+#include "ui/gfx/favicon_size.h"
+
+namespace favicon {
+namespace {
+
+const char kClientParamDesktop[] = "client=chrome_desktop";
+const char kClientParamMobile[] = "client=chrome";
+
+float GetMaxDeviceScaleFactor() {
+  std::vector<float> favicon_scales = favicon_base::GetFaviconScales();
+  DCHECK(!favicon_scales.empty());
+  return favicon_scales.back();
+}
+
+}  // namespace
+
+std::unique_ptr<FaviconServerFetcherParams>
+FaviconServerFetcherParams::CreateForDesktop(const GURL& page_url) {
+  return base::WrapUnique(new FaviconServerFetcherParams(
+      page_url, favicon_base::IconType::kFavicon,
+      std::ceil(gfx::kFaviconSize * GetMaxDeviceScaleFactor()), 0,
+      kClientParamDesktop));
+}
+
+std::unique_ptr<FaviconServerFetcherParams>
+FaviconServerFetcherParams::CreateForMobile(const GURL& page_url,
+                                            int min_source_size_in_pixel,
+                                            int desired_size_in_pixel) {
+  return base::WrapUnique(new FaviconServerFetcherParams(
+      page_url, favicon_base::IconType::kTouchIcon, min_source_size_in_pixel,
+      desired_size_in_pixel, kClientParamMobile));
+}
+
+FaviconServerFetcherParams::FaviconServerFetcherParams(
+    const GURL& page_url,
+    favicon_base::IconType icon_type,
+    int min_source_size_in_pixel,
+    int desired_size_in_pixel,
+    const std::string& google_server_client_param)
+    : page_url_(page_url),
+      icon_type_(icon_type),
+      min_source_size_in_pixel_(min_source_size_in_pixel),
+      desired_size_in_pixel_(desired_size_in_pixel),
+      google_server_client_param_(google_server_client_param) {}
+
+FaviconServerFetcherParams::~FaviconServerFetcherParams() = default;
+
+}  // namespace favicon

components/favicon/core/favicon_server_fetcher_params.h

+// Copyright (c) 2018 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.
+
+#ifndef COMPONENTS_FAVICON_CORE_FAVICON_SERVER_FETCHER_PARAMS_H_
+#define COMPONENTS_FAVICON_CORE_FAVICON_SERVER_FETCHER_PARAMS_H_
+
+#include "components/favicon_base/favicon_util.h"
+
+class GURL;
+
+namespace favicon {
+
+class FaviconServerFetcherParams {
+ public:
+  // Platform specific constructors that set default fetch parameters. Any
+  // platform not Android nor iOS is considered desktop.
+  static std::unique_ptr<FaviconServerFetcherParams> CreateForDesktop(
+      const GURL& page_url);
+  static std::unique_ptr<FaviconServerFetcherParams> CreateForMobile(
+      const GURL& page_url,
+      int min_source_size_in_pixel,
+      int desired_size_in_pixel);
+
+  ~FaviconServerFetcherParams();
+
+  const GURL& page_url() const { return page_url_; };
+  favicon_base::IconType icon_type() const { return icon_type_; };
+  int min_source_size_in_pixel() const { return min_source_size_in_pixel_; };
+  int desired_size_in_pixel() const { return desired_size_in_pixel_; };
+  const std::string& google_server_client_param() const {
+    return google_server_client_param_;
+  };
+
+ private:
+  FaviconServerFetcherParams(const GURL& page_url,
+                             favicon_base::IconType icon_type,
+                             int min_source_size_in_pixel,
+                             int desired_size_in_pixel,
+                             const std::string& google_server_client_param);
+
+  const GURL& page_url_;
+  favicon_base::IconType icon_type_;
+  int min_source_size_in_pixel_;
+  int desired_size_in_pixel_;
+  std::string google_server_client_param_;
+
+  DISALLOW_COPY_AND_ASSIGN(FaviconServerFetcherParams);
+};
+
+}  // namespace favicon
+
+#endif  // COMPONENTS_FAVICON_CORE_FAVICON_SERVER_FETCHER_PARAMS_H_

components/favicon/core/large_icon_service.cc

@@ -24,6 +24,7 @@
 #include "base/task_scheduler/post_task.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_server_fetcher_params.h"
 #include "components/favicon/core/favicon_service.h"
 #include "components/favicon_base/fallback_icon_style.h"
 #include "components/favicon_base/favicon_types.h"
@@ -52,6 +53,8 @@ const char kGoogleServerV2RequestFormat[] =
     "size=%d&min_size=%d&max_size=%d&fallback_opts=TYPE,SIZE,URL&url=%s";
 const char kGoogleServerV2RequestFormatParam[] = "request_format";
 
+const char kClientParam[] = "client=chrome";
+
 const char kCheckSeenParam[] = "check_seen=true&";
 
 const int kGoogleServerV2EnforcedMinSizeInPixel = 16;
@@ -79,10 +82,12 @@ GURL TrimPageUrlForGoogleServer(const GURL& page_url) {
   return page_url.ReplaceComponents(replacements);
 }
 
-GURL GetRequestUrlForGoogleServerV2(const GURL& page_url,
-                                    int min_source_size_in_pixel,
-                                    int desired_size_in_pixel,
-                                    bool may_page_url_be_private) {
+GURL GetRequestUrlForGoogleServerV2(
+    const GURL& page_url,
+    const std::string& google_server_client_param,
+    int min_source_size_in_pixel,
+    int desired_size_in_pixel,
+    bool may_page_url_be_private) {
   std::string url_format = base::GetFieldTrialParamValueByFeature(
       kLargeIconServiceFetchingFeature, kGoogleServerV2RequestFormatParam);
   double desired_to_max_size_factor = base::GetFieldTrialParamByFeatureAsDouble(
@@ -105,10 +110,13 @@ GURL GetRequestUrlForGoogleServerV2(const GURL& page_url,
       static_cast<int>(desired_size_in_pixel * desired_to_max_size_factor);
   max_size_in_pixel = std::max(max_size_in_pixel, minimum_max_size_in_pixel);
 
-  return GURL(base::StringPrintf(
+  std::string request_url = base::StringPrintf(
       url_format.empty() ? kGoogleServerV2RequestFormat : url_format.c_str(),
       may_page_url_be_private ? kCheckSeenParam : "", desired_size_in_pixel,
-      min_source_size_in_pixel, max_size_in_pixel, page_url.spec().c_str()));
+      min_source_size_in_pixel, max_size_in_pixel, page_url.spec().c_str());
+  base::ReplaceFirstSubstringAfterOffset(
+      &request_url, 0, std::string(kClientParam), google_server_client_param);
+  return GURL(request_url);
 }
 
 bool IsDbResultAdequate(const favicon_base::FaviconRawBitmapResult& db_result,
@@ -193,8 +201,8 @@ void ProcessIconOnBackgroundThread(
 void FinishServerRequestAsynchronously(
     const favicon_base::GoogleFaviconServerCallback& callback,
     favicon_base::GoogleFaviconServerRequestStatus status) {
-  base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
-                                                base::Bind(callback, status));
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::BindOnce(callback, status));
 }
 
 // Singleton map keyed by organization-identifying domain (excludes registrar
@@ -407,6 +415,7 @@ void OnSetOnDemandFaviconComplete(
 void OnFetchIconFromGoogleServerComplete(
     FaviconService* favicon_service,
     const GURL& page_url,
+    favicon_base::IconType icon_type,
     const favicon_base::GoogleFaviconServerCallback& callback,
     const std::string& server_request_url,
     const gfx::Image& image,
@@ -437,8 +446,8 @@ void OnFetchIconFromGoogleServerComplete(
   // expired (out-of-date), they will be refetched when we visit the original
   // page any time in the future.
   favicon_service->SetOnDemandFavicons(
-      page_url, GURL(original_icon_url), favicon_base::IconType::kTouchIcon,
-      image, base::Bind(&OnSetOnDemandFaviconComplete, callback));
+      page_url, GURL(original_icon_url), icon_type, image,
+      base::BindOnce(&OnSetOnDemandFaviconComplete, callback));
 }
 
 }  // namespace
@@ -485,13 +494,11 @@ LargeIconService::GetLargeIconImageOrFallbackStyle(
 
 void LargeIconService::
     GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
-        const GURL& page_url,
-        int min_source_size_in_pixel,
-        int desired_size_in_pixel,
+        std::unique_ptr<FaviconServerFetcherParams> params,
         bool may_page_url_be_private,
         const net::NetworkTrafficAnnotationTag& traffic_annotation,
         const favicon_base::GoogleFaviconServerCallback& callback) {
-  DCHECK_LE(0, min_source_size_in_pixel);
+  DCHECK_LE(0, params->min_source_size_in_pixel());
 
   if (net::NetworkChangeNotifier::IsOffline()) {
     // By exiting early when offline, we avoid caching the failure and thus
@@ -501,13 +508,13 @@ void LargeIconService::
     return;
   }
 
-  if (!page_url.is_valid()) {
+  if (!params->page_url().is_valid()) {
     FinishServerRequestAsynchronously(
         callback, GoogleFaviconServerRequestStatus::FAILURE_TARGET_URL_INVALID);
     return;
   }
 
-  const GURL trimmed_page_url = TrimPageUrlForGoogleServer(page_url);
+  const GURL trimmed_page_url = TrimPageUrlForGoogleServer(params->page_url());
   if (!trimmed_page_url.is_valid()) {
     FinishServerRequestAsynchronously(
         callback, GoogleFaviconServerRequestStatus::FAILURE_TARGET_URL_SKIPPED);
@@ -515,7 +522,8 @@ void LargeIconService::
   }
 
   const GURL server_request_url = GetRequestUrlForGoogleServerV2(
-      trimmed_page_url, min_source_size_in_pixel, desired_size_in_pixel,
+      trimmed_page_url, params->google_server_client_param(),
+      params->min_source_size_in_pixel(), params->desired_size_in_pixel(),
       may_page_url_be_private);
   if (!server_request_url.is_valid()) {
     FinishServerRequestAsynchronously(
@@ -532,10 +540,11 @@ void LargeIconService::
   }
 
   favicon_service_->CanSetOnDemandFavicons(
-      page_url, favicon_base::IconType::kTouchIcon,
+      params->page_url(), params->icon_type(),
       base::BindOnce(&LargeIconService::OnCanSetOnDemandFaviconComplete,
                      weak_ptr_factory_.GetWeakPtr(), server_request_url,
-                     page_url, traffic_annotation, callback));
+                     params->page_url(), params->icon_type(),
+                     traffic_annotation, callback));
 }
 
 void LargeIconService::TouchIconFromGoogleServer(const GURL& icon_url) {
@@ -591,6 +600,7 @@ LargeIconService::GetLargeIconOrFallbackStyleImpl(
 void LargeIconService::OnCanSetOnDemandFaviconComplete(
     const GURL& server_request_url,
     const GURL& page_url,
+    favicon_base::IconType icon_type,
     const net::NetworkTrafficAnnotationTag& traffic_annotation,
     const favicon_base::GoogleFaviconServerCallback& callback,
     bool can_set_on_demand_favicon) {
@@ -603,8 +613,8 @@ void LargeIconService::OnCanSetOnDemandFaviconComplete(
       data_use_measurement::DataUseUserData::LARGE_ICON_SERVICE);
   image_fetcher_->FetchImage(
       server_request_url.spec(), server_request_url,
-      base::BindRepeating(&OnFetchIconFromGoogleServerComplete,
-                          favicon_service_, page_url, callback),
+      base::BindOnce(&OnFetchIconFromGoogleServerComplete, favicon_service_,
+                     page_url, icon_type, callback),
       traffic_annotation);
 }
 

components/favicon/core/large_icon_service.h

@@ -26,6 +26,7 @@ class ImageFetcher;
 namespace favicon {
 
 class FaviconService;
+class FaviconServerFetcherParams;
 
 // The large icon service provides methods to access large icons. It relies on
 // the favicon service.
@@ -93,9 +94,7 @@ class LargeIconService : public KeyedService {
   // TODO(jkrcal): It is not clear from the name of this function, that it
   // actually adds the icon to the local cache. Maybe "StoreLargeIcon..."?
   void GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
-      const GURL& page_url,
-      int min_source_size_in_pixel,
-      int desired_size_in_pixel,
+      std::unique_ptr<FaviconServerFetcherParams> params,
       bool may_page_url_be_private,
       const net::NetworkTrafficAnnotationTag& traffic_annotation,
       const favicon_base::GoogleFaviconServerCallback& callback);
@@ -125,6 +124,7 @@ class LargeIconService : public KeyedService {
   void OnCanSetOnDemandFaviconComplete(
       const GURL& server_request_url,
       const GURL& page_url,
+      favicon_base::IconType icon_type,
       const net::NetworkTrafficAnnotationTag& traffic_annotation,
       const favicon_base::GoogleFaviconServerCallback& callback,
       bool can_set_on_demand_favicon);

components/ntp_snippets/content_suggestions_service.cc

@@ -18,6 +18,7 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/default_clock.h"
 #include "base/values.h"
+#include "components/favicon/core/favicon_server_fetcher_params.h"
 #include "components/favicon/core/large_icon_service.h"
 #include "components/favicon_base/fallback_icon_style.h"
 #include "components/favicon_base/favicon_types.h"
@@ -295,7 +296,8 @@ void ContentSuggestionsService::OnGetFaviconFromCacheFinished(
         })");
   large_icon_service_
       ->GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
-          publisher_url, minimum_size_in_pixel, desired_size_in_pixel,
+          favicon::FaviconServerFetcherParams::CreateForMobile(
+              publisher_url, minimum_size_in_pixel, desired_size_in_pixel),
           /*may_page_url_be_private=*/false, traffic_annotation,
           base::Bind(
               &ContentSuggestionsService::OnGetFaviconFromGoogleServerFinished,

components/ntp_tiles/icon_cacher_impl.cc

@@ -8,6 +8,7 @@
 
 #include "base/metrics/field_trial_params.h"
 #include "base/metrics/histogram_macros.h"
+#include "components/favicon/core/favicon_server_fetcher_params.h"
 #include "components/favicon/core/favicon_service.h"
 #include "components/favicon/core/favicon_util.h"
 #include "components/favicon/core/large_icon_service.h"
@@ -264,9 +265,10 @@ void IconCacherImpl::OnGetLargeIconOrFallbackStyleFinished(
         })");
   large_icon_service_
       ->GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
-          page_url,
-          GetMinimumFetchingSizeForChromeSuggestionsFaviconsFromServer(),
-          GetDesiredFetchingSizeForChromeSuggestionsFaviconsFromServer(),
+          favicon::FaviconServerFetcherParams::CreateForMobile(
+              page_url,
+              GetMinimumFetchingSizeForChromeSuggestionsFaviconsFromServer(),
+              GetDesiredFetchingSizeForChromeSuggestionsFaviconsFromServer()),
           /*may_page_url_be_private=*/true, traffic_annotation,
           base::Bind(&IconCacherImpl::OnMostLikelyFaviconDownloaded,
                      weak_ptr_factory_.GetWeakPtr(), page_url));

ios/chrome/browser/ui/bookmarks/bookmark_table_view.mm

@@ -9,6 +9,7 @@
 #include "base/strings/sys_string_conversions.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/favicon/core/fallback_url_util.h"
+#include "components/favicon/core/favicon_server_fetcher_params.h"
 #include "components/favicon/core/large_icon_service.h"
 #include "components/favicon_base/fallback_icon_style.h"
 #include "components/favicon_base/favicon_types.h"
@@ -757,7 +758,9 @@ using IntegerPair = std::pair<NSInteger, NSInteger>;
       strongSelf.sharedState.faviconDownloadCount++;
       IOSChromeLargeIconServiceFactory::GetForBrowserState(self.browserState)
           ->GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
-              node->url(), minFaviconSizeInPixel, desiredFaviconSizeInPixel,
+              favicon::FaviconServerFetcherParams::CreateForMobile(
+                  node->url(), minFaviconSizeInPixel,
+                  desiredFaviconSizeInPixel),
               /*may_page_url_be_private=*/true, kTrafficAnnotation,
               base::BindBlockArc(faviconLoadedFromServerBlock));
     }