Use single size to request from favicon server in LargeIconService

Currently LargeIconService allows callers to decide the size of the
favicon they wish to download from the favicon server. Depending on
which UI surface the user opens first, a different size is downloaded,
which can cause displaying upscaled icons and decrease in quality. We
solve this by downloading a single sufficiently big size (chosen based
on existing values used by callers), aiming to only have downscaling.

Bug: 982810
Change-Id: Ibad636d4c4d63791e9eb62d99221d9f35c9cb375
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1695268
Commit-Queue: Victor Vianna <victorvianna@google.com>
Reviewed-by: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Reviewed-by: Kristi Park <kristipark@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: edchin <edchin@chromium.org>
Reviewed-by: Nicolas Zea <zea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#681751}

chrome/browser/android/bookmarks/partner_bookmarks_reader.cc

@@ -320,8 +320,7 @@ void PartnerBookmarksReader::OnGetFaviconFromCacheFinished(
         })");
   GetLargeIconService()
       ->GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
-          favicon::FaviconServerFetcherParams::CreateForMobile(
-              page_url, desired_favicon_size_px),
+          favicon::FaviconServerFetcherParams::CreateForMobile(page_url),
           false /* may_page_url_be_private */,
           false /* should_trim_page_url_path */, traffic_annotation,
           base::Bind(&PartnerBookmarksReader::OnGetFaviconFromServerFinished,

components/favicon/core/favicon_server_fetcher_params.cc

@@ -6,6 +6,7 @@
 
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
+#include "build/build_config.h"
 #include "components/favicon_base/favicon_util.h"
 #include "ui/gfx/favicon_size.h"
 
@@ -14,6 +15,13 @@ namespace {
 
 const char kClientParamDesktop[] = "client=chrome_desktop";
 const char kClientParamMobile[] = "client=chrome";
+// TODO(https://crbug.com/982810): Refactor to remove ios specific code from
+// here.
+#if defined(OS_IOS)
+const int kMobileSizeInDip = 32;
+#else
+const int kMobileSizeInDip = 24;
+#endif
 
 float GetMaxDeviceScaleFactor() {
   std::vector<float> favicon_scales = favicon_base::GetFaviconScales();
@@ -32,10 +40,10 @@ FaviconServerFetcherParams::CreateForDesktop(const GURL& page_url) {
 }
 
 std::unique_ptr<FaviconServerFetcherParams>
-FaviconServerFetcherParams::CreateForMobile(const GURL& page_url,
-                                            int desired_size_in_pixel) {
+FaviconServerFetcherParams::CreateForMobile(const GURL& page_url) {
   return base::WrapUnique(new FaviconServerFetcherParams(
-      page_url, favicon_base::IconType::kTouchIcon, desired_size_in_pixel,
+      page_url, favicon_base::IconType::kTouchIcon,
+      std::ceil(kMobileSizeInDip * GetMaxDeviceScaleFactor()),
       kClientParamMobile));
 }
 

components/favicon/core/favicon_server_fetcher_params.h

@@ -18,8 +18,7 @@ class FaviconServerFetcherParams {
   static std::unique_ptr<FaviconServerFetcherParams> CreateForDesktop(
       const GURL& page_url);
   static std::unique_ptr<FaviconServerFetcherParams> CreateForMobile(
-      const GURL& page_url,
-      int desired_size_in_pixel);
+      const GURL& page_url);
 
   ~FaviconServerFetcherParams();
 

components/favicon/core/history_ui_favicon_request_handler_impl.cc

@@ -173,8 +173,7 @@ void HistoryUiFaviconRequestHandlerImpl::OnBitmapLocalDataAvailable(
             base::AdaptCallbackForRepeating(std::move(response_callback));
     std::unique_ptr<FaviconServerFetcherParams> server_parameters =
         platform == FaviconRequestPlatform::kMobile
-            ? FaviconServerFetcherParams::CreateForMobile(page_url,
-                                                          desired_size_in_pixel)
+            ? FaviconServerFetcherParams::CreateForMobile(page_url)
             : FaviconServerFetcherParams::CreateForDesktop(page_url);
     RequestFromGoogleServer(
         page_url, std::move(server_parameters),

components/favicon/core/large_icon_service.h

@@ -74,8 +74,10 @@ class LargeIconService : public KeyedService {
   // favicon database contains an icon for |page_url|, so clients are
   // encouraged to use GetLargeIconOrFallbackStyle() first.
   //
-  // |desired_size_in_pixel| serves only as a hint to the service, no guarantees
-  // on the fetched size are provided.
+  // A parameter in the server request representing the desired favicon size is
+  // set according solely to the device and scale factor. However, it serves
+  // only as a hint to the service, no guarantees on the fetched size are
+  // provided.
   //
   // Unless you are sure |page_url| is a public URL (known to Google Search),
   // set |may_page_url_be_private| to true. This slighty increases the chance of

components/favicon/core/large_icon_service_impl_unittest.cc

@@ -11,12 +11,14 @@
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted_memory.h"
+#include "base/strings/stringprintf.h"
 #include "base/task/cancelable_task_tracker.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/mock_callback.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/scoped_task_environment.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "build/build_config.h"
 #include "components/favicon/core/favicon_client.h"
 #include "components/favicon/core/favicon_server_fetcher_params.h"
 #include "components/favicon/core/test/mock_favicon_service.h"
@@ -56,6 +58,11 @@ using testing::SaveArg;
 const char kDummyUrl[] = "http://www.example.com";
 const char kDummyIconUrl[] = "http://www.example.com/touch_icon.png";
 const SkColor kTestColor = SK_ColorRED;
+#if defined(OS_IOS)
+const int kMobileSizeInDip = 32;
+#else
+const int kMobileSizeInDip = 24;
+#endif
 
 ACTION_P(PostFetchReply, p0) {
   base::ThreadTaskRunnerHandle::Get()->PostTask(
@@ -138,10 +145,11 @@ class LargeIconServiceTest : public testing::Test {
 };
 
 TEST_F(LargeIconServiceTest, ShouldGetFromGoogleServer) {
-  const GURL kExpectedServerUrl(
+  const GURL kExpectedServerUrl(base::StringPrintf(
       "https://t0.gstatic.com/faviconV2?client=chrome&nfrp=2"
-      "&check_seen=true&size=61&min_size=16&max_size=256"
-      "&fallback_opts=TYPE,SIZE,URL&url=http://www.example.com/");
+      "&check_seen=true&size=%d&min_size=16&max_size=256"
+      "&fallback_opts=TYPE,SIZE,URL&url=http://www.example.com/",
+      kMobileSizeInDip * 2));
 
   EXPECT_CALL(mock_favicon_service_, UnableToDownloadFavicon(_)).Times(0);
   EXPECT_CALL(mock_favicon_service_,
@@ -168,9 +176,7 @@ TEST_F(LargeIconServiceTest, ShouldGetFromGoogleServer) {
 
   large_icon_service_
       .GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
-          favicon::FaviconServerFetcherParams::CreateForMobile(
-              GURL(kDummyUrl),
-              /*desired_size_in_pixel=*/61),
+          favicon::FaviconServerFetcherParams::CreateForMobile(GURL(kDummyUrl)),
           /*may_page_url_be_private=*/true, /*should_trim_page_url_path=*/false,
           TRAFFIC_ANNOTATION_FOR_TESTS, callback.Get());
 
@@ -225,10 +231,11 @@ TEST_F(LargeIconServiceTest, ShouldGetFromGoogleServerForDesktop) {
 }
 
 TEST_F(LargeIconServiceTest, ShouldGetFromGoogleServerWithOriginalUrl) {
-  const GURL kExpectedServerUrl(
+  const GURL kExpectedServerUrl(base::StringPrintf(
       "https://t0.gstatic.com/faviconV2?client=chrome&nfrp=2"
-      "&check_seen=true&size=61&min_size=16&max_size=256"
-      "&fallback_opts=TYPE,SIZE,URL&url=http://www.example.com/");
+      "&check_seen=true&size=%d&min_size=16&max_size=256"
+      "&fallback_opts=TYPE,SIZE,URL&url=http://www.example.com/",
+      kMobileSizeInDip * 2));
   const GURL kExpectedOriginalUrl("http://www.example.com/favicon.png");
 
   EXPECT_CALL(mock_favicon_service_,
@@ -259,9 +266,7 @@ TEST_F(LargeIconServiceTest, ShouldGetFromGoogleServerWithOriginalUrl) {
   base::MockCallback<favicon_base::GoogleFaviconServerCallback> callback;
   large_icon_service_
       .GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
-          favicon::FaviconServerFetcherParams::CreateForMobile(
-              GURL(kDummyUrl),
-              /*desired_size_in_pixel=*/61),
+          favicon::FaviconServerFetcherParams::CreateForMobile(GURL(kDummyUrl)),
           /*may_page_url_be_private=*/true, /*should_trim_page_url_path=*/false,
           TRAFFIC_ANNOTATION_FOR_TESTS, callback.Get());
 
@@ -272,10 +277,11 @@ TEST_F(LargeIconServiceTest, ShouldGetFromGoogleServerWithOriginalUrl) {
 
 TEST_F(LargeIconServiceTest, ShouldTrimQueryParametersForGoogleServer) {
   const GURL kDummyUrlWithQuery("http://www.example.com?foo=1");
-  const GURL kExpectedServerUrl(
+  const GURL kExpectedServerUrl(base::StringPrintf(
       "https://t0.gstatic.com/faviconV2?client=chrome&nfrp=2"
-      "&check_seen=true&size=61&min_size=16&max_size=256"
-      "&fallback_opts=TYPE,SIZE,URL&url=http://www.example.com/");
+      "&check_seen=true&size=%d&min_size=16&max_size=256"
+      "&fallback_opts=TYPE,SIZE,URL&url=http://www.example.com/",
+      kMobileSizeInDip * 2));
 
   EXPECT_CALL(mock_favicon_service_,
               CanSetOnDemandFavicons(GURL(kDummyUrlWithQuery),
@@ -296,8 +302,7 @@ TEST_F(LargeIconServiceTest, ShouldTrimQueryParametersForGoogleServer) {
   large_icon_service_
       .GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
           favicon::FaviconServerFetcherParams::CreateForMobile(
-              GURL(kDummyUrlWithQuery),
-              /*desired_size_in_pixel=*/61),
+              GURL(kDummyUrlWithQuery)),
           /*may_page_url_be_private=*/true, /*should_trim_page_url_path=*/false,
           TRAFFIC_ANNOTATION_FOR_TESTS,
           favicon_base::GoogleFaviconServerCallback());
@@ -325,9 +330,7 @@ TEST_F(LargeIconServiceTest, ShouldNotCheckOnPublicUrls) {
 
   large_icon_service_
       .GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
-          favicon::FaviconServerFetcherParams::CreateForMobile(
-              GURL(kDummyUrl),
-              /*desired_size_in_pixel=*/61),
+          favicon::FaviconServerFetcherParams::CreateForMobile(GURL(kDummyUrl)),
           /*may_page_url_be_private=*/false,
           /*should_trim_page_url_path=*/false, TRAFFIC_ANNOTATION_FOR_TESTS,
           callback.Get());
@@ -347,8 +350,7 @@ TEST_F(LargeIconServiceTest, ShouldNotQueryGoogleServerIfInvalidScheme) {
   large_icon_service_
       .GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
           favicon::FaviconServerFetcherParams::CreateForMobile(
-              GURL(kDummyFtpUrl),
-              /*desired_size_in_pixel=*/61),
+              GURL(kDummyFtpUrl)),
           /*may_page_url_be_private=*/true, /*should_trim_page_url_path=*/false,
           TRAFFIC_ANNOTATION_FOR_TESTS, callback.Get());
 
@@ -370,8 +372,7 @@ TEST_F(LargeIconServiceTest, ShouldNotQueryGoogleServerIfInvalidURL) {
   large_icon_service_
       .GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
           favicon::FaviconServerFetcherParams::CreateForMobile(
-              GURL(kDummyInvalidUrl),
-              /*desired_size_in_pixel=*/61),
+              GURL(kDummyInvalidUrl)),
           /*may_page_url_be_private=*/true, /*should_trim_page_url_path=*/false,
           TRAFFIC_ANNOTATION_FOR_TESTS, callback.Get());
 
@@ -384,10 +385,11 @@ TEST_F(LargeIconServiceTest, ShouldNotQueryGoogleServerIfInvalidURL) {
 }
 
 TEST_F(LargeIconServiceTest, ShouldReportUnavailableIfFetchFromServerFails) {
-  const GURL kExpectedServerUrl(
+  const GURL kExpectedServerUrl(base::StringPrintf(
       "https://t0.gstatic.com/faviconV2?client=chrome&nfrp=2"
-      "&check_seen=true&size=61&min_size=16&max_size=256"
-      "&fallback_opts=TYPE,SIZE,URL&url=http://www.example.com/");
+      "&check_seen=true&size=%d&min_size=16&max_size=256"
+      "&fallback_opts=TYPE,SIZE,URL&url=http://www.example.com/",
+      kMobileSizeInDip * 2));
 
   EXPECT_CALL(mock_favicon_service_,
               CanSetOnDemandFavicons(GURL(kDummyUrl),
@@ -408,9 +410,7 @@ TEST_F(LargeIconServiceTest, ShouldReportUnavailableIfFetchFromServerFails) {
 
   large_icon_service_
       .GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
-          favicon::FaviconServerFetcherParams::CreateForMobile(
-              GURL(kDummyUrl),
-              /*desired_size_in_pixel=*/61),
+          favicon::FaviconServerFetcherParams::CreateForMobile(GURL(kDummyUrl)),
           /*may_page_url_be_private=*/true, /*should_trim_page_url_path=*/false,
           TRAFFIC_ANNOTATION_FOR_TESTS, callback.Get());
 
@@ -424,10 +424,11 @@ TEST_F(LargeIconServiceTest, ShouldReportUnavailableIfFetchFromServerFails) {
 
 TEST_F(LargeIconServiceTest, ShouldNotGetFromGoogleServerIfUnavailable) {
   ON_CALL(mock_favicon_service_,
-          WasUnableToDownloadFavicon(
-              GURL("https://t0.gstatic.com/faviconV2?client=chrome&nfrp=2"
-                   "&check_seen=true&size=61&min_size=16&max_size=256"
-                   "&fallback_opts=TYPE,SIZE,URL&url=http://www.example.com/")))
+          WasUnableToDownloadFavicon(GURL(base::StringPrintf(
+              "https://t0.gstatic.com/faviconV2?client=chrome&nfrp=2"
+              "&check_seen=true&size=%d&min_size=16&max_size=256"
+              "&fallback_opts=TYPE,SIZE,URL&url=http://www.example.com/",
+              kMobileSizeInDip * 2))))
       .WillByDefault(Return(true));
 
   EXPECT_CALL(mock_favicon_service_, UnableToDownloadFavicon(_)).Times(0);
@@ -438,9 +439,7 @@ TEST_F(LargeIconServiceTest, ShouldNotGetFromGoogleServerIfUnavailable) {
   base::MockCallback<favicon_base::GoogleFaviconServerCallback> callback;
   large_icon_service_
       .GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
-          favicon::FaviconServerFetcherParams::CreateForMobile(
-              GURL(kDummyUrl),
-              /*desired_size_in_pixel=*/61),
+          favicon::FaviconServerFetcherParams::CreateForMobile(GURL(kDummyUrl)),
           /*may_page_url_be_private=*/true, /*should_trim_page_url_path=*/false,
           TRAFFIC_ANNOTATION_FOR_TESTS, callback.Get());
 
@@ -469,9 +468,7 @@ TEST_F(LargeIconServiceTest, ShouldNotGetFromGoogleServerIfCannotSet) {
   base::MockCallback<favicon_base::GoogleFaviconServerCallback> callback;
   large_icon_service_
       .GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
-          favicon::FaviconServerFetcherParams::CreateForMobile(
-              GURL(kDummyUrl),
-              /*desired_size_in_pixel=*/61),
+          favicon::FaviconServerFetcherParams::CreateForMobile(GURL(kDummyUrl)),
           /*may_page_url_be_private=*/true, /*should_trim_page_url_path=*/false,
           TRAFFIC_ANNOTATION_FOR_TESTS, callback.Get());
 

components/ntp_snippets/content_suggestions_service.cc

@@ -292,8 +292,7 @@ void ContentSuggestionsService::OnGetFaviconFromCacheFinished(
         })");
   large_icon_service_
       ->GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
-          favicon::FaviconServerFetcherParams::CreateForMobile(
-              publisher_url, desired_size_in_pixel),
+          favicon::FaviconServerFetcherParams::CreateForMobile(publisher_url),
           /*may_page_url_be_private=*/false,
           /*should_trim_page_url_path=*/false, traffic_annotation,
           base::Bind(

components/ntp_tiles/icon_cacher_impl.cc

@@ -35,12 +35,10 @@ constexpr int kDesiredFrameSize = 128;
 // arguments from the UI so that we desire for the right size on a given device.
 // See crbug.com/696563.
 constexpr int kDefaultTileIconMinSizePx = 1;
-constexpr int kDefaultTileIconDesiredSizePx = 96;
 
 const char kImageFetcherUmaClient[] = "IconCacher";
 
 constexpr char kTileIconMinSizePxFieldParam[] = "min_size";
-constexpr char kTileIconDesiredSizePxFieldParam[] = "desired_size";
 
 favicon_base::IconType IconType(const PopularSites::Site& site) {
   return site.large_icon_url.is_valid() ? favicon_base::IconType::kTouchIcon
@@ -66,12 +64,6 @@ int GetMinimumFetchingSizeForChromeSuggestionsFaviconsFromServer() {
       kDefaultTileIconMinSizePx);
 }
 
-int GetDesiredFetchingSizeForChromeSuggestionsFaviconsFromServer() {
-  return base::GetFieldTrialParamByFeatureAsInt(
-      kNtpMostLikelyFaviconsFromServerFeature, kTileIconDesiredSizePxFieldParam,
-      kDefaultTileIconDesiredSizePx);
-}
-
 }  // namespace
 
 IconCacherImpl::IconCacherImpl(
@@ -262,9 +254,7 @@ void IconCacherImpl::OnGetLargeIconOrFallbackStyleFinished(
         })");
   large_icon_service_
       ->GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
-          favicon::FaviconServerFetcherParams::CreateForMobile(
-              page_url,
-              GetDesiredFetchingSizeForChromeSuggestionsFaviconsFromServer()),
+          favicon::FaviconServerFetcherParams::CreateForMobile(page_url),
           /*may_page_url_be_private=*/true, /*should_trim_page_url_path=*/false,
           traffic_annotation,
           base::Bind(&IconCacherImpl::OnMostLikelyFaviconDownloaded,

ios/chrome/browser/favicon/favicon_loader.h

@@ -36,7 +36,8 @@ class FaviconLoader : public KeyedService {
   //   1. Use |large_icon_service_| to fetch from local DB managed by
   //      HistoryService;
   //   2. Use |large_icon_service_| to fetch from Google Favicon server if
-  //      |fallback_to_google_server|=YES;
+  //      |fallback_to_google_server|=YES (|size_in_points| is ignored when
+  //      fetching from the Google server);
   //   3. Create a favicon base on the fallback style from |large_icon_service|.
   void FaviconForPageUrl(const GURL& page_url,
                          float size_in_points,

ios/chrome/browser/favicon/favicon_loader.mm

@@ -74,7 +74,6 @@ void FaviconLoader::FaviconForPageUrl(
   }
 
   const CGFloat scale = UIScreen.mainScreen.scale;
-  const CGFloat favicon_size_in_pixels = scale * size_in_points;
   GURL block_page_url(page_url);
   auto favicon_block = ^(const favicon_base::LargeIconResult& result) {
     // GetLargeIconOrFallbackStyle() either returns a valid favicon (which can
@@ -113,7 +112,7 @@ void FaviconLoader::FaviconForPageUrl(
       large_icon_service_
           ->GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
               favicon::FaviconServerFetcherParams::CreateForMobile(
-                  block_page_url, favicon_size_in_pixels),
+                  block_page_url),
               /*may_page_url_be_private=*/true,
               /*should_trim_page_url_path=*/false, kTrafficAnnotation,
               base::BindRepeating(favicon_loaded_from_server_block));
@@ -141,7 +140,7 @@ void FaviconLoader::FaviconForPageUrl(
   // Now fetch the image synchronously.
   DCHECK(large_icon_service_);
   large_icon_service_->GetLargeIconRawBitmapOrFallbackStyleForPageUrl(
-      page_url, scale * min_size_in_points, favicon_size_in_pixels,
+      page_url, scale * min_size_in_points, scale * size_in_points,
       base::BindRepeating(favicon_block), &cancelable_task_tracker_);
 }