Extend FaviconRequestHandler functionality for mobile

We extend the functionality for mobile, including different favicon
sizes in the favicon server request. We add no mobile UI callers in
this CL.

Change-Id: I115ba3d494fbb6ebbfbced328fe75d9a21f331f5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1635470
Auto-Submit: Victor Vianna <victorvianna@google.com>
Reviewed-by: Esmael El-Moslimany <aee@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Esmael El-Moslimany <aee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#665632}

chrome/browser/ui/webui/favicon_source.cc

@@ -171,7 +171,8 @@ void FaviconSource::StartDataRequest(
             &FaviconSource::OnFaviconDataAvailable, base::Unretained(this),
             IconRequest(callback, url, parsed.size_in_dip,
                         parsed.device_scale_factor, unsafe_request_origin)),
-        unsafe_request_origin, favicon_service,
+        unsafe_request_origin, favicon::FaviconRequestPlatform::kDesktop,
+        favicon_service,
         LargeIconServiceFactory::GetForBrowserContext(profile_),
         /*icon_url_for_uma=*/
         open_tabs ? open_tabs->GetIconUrlForPageUrl(url) : GURL(),

components/favicon/core/favicon_request_handler.cc

@@ -79,8 +79,22 @@ void RecordFaviconServerGroupingMetric(FaviconRequestOrigin origin,
 const bool kFallbackToHost = true;
 
 // Parameter used for local bitmap queries by page url.
-favicon_base::IconTypeSet GetIconTypesForLocalQuery() {
-  return favicon_base::IconTypeSet{favicon_base::IconType::kFavicon};
+favicon_base::IconTypeSet GetIconTypesForLocalQuery(
+    FaviconRequestPlatform platform) {
+  // The value must agree with the one written in the local data for retrieved
+  // server icons so that we can find them on the second lookup. This depends on
+  // whether the caller is a mobile UI or not.
+  switch (platform) {
+    case FaviconRequestPlatform::kMobile:
+      return favicon_base::IconTypeSet{
+          favicon_base::IconType::kFavicon, favicon_base::IconType::kTouchIcon,
+          favicon_base::IconType::kTouchPrecomposedIcon,
+          favicon_base::IconType::kWebManifestIcon};
+    case FaviconRequestPlatform::kDesktop:
+      return favicon_base::IconTypeSet{favicon_base::IconType::kFavicon};
+  }
+  NOTREACHED();
+  return favicon_base::IconTypeSet{};
 }
 
 bool CanOriginQueryGoogleServer(FaviconRequestOrigin origin) {
@@ -92,6 +106,7 @@ bool CanOriginQueryGoogleServer(FaviconRequestOrigin origin) {
     case FaviconRequestOrigin::UNKNOWN:
       return false;
   }
+  NOTREACHED();
   return false;
 }
 
@@ -101,18 +116,16 @@ GURL GetGroupIdentifier(const GURL& page_url, const GURL& icon_url) {
   return !icon_url.is_empty() ? icon_url : page_url;
 }
 
-}  // namespace
-
-// static
-bool FaviconRequestHandler::CanQueryGoogleServer(
-    LargeIconService* large_icon_service,
-    FaviconRequestOrigin origin,
-    bool can_send_history_data) {
+bool CanQueryGoogleServer(LargeIconService* large_icon_service,
+                          FaviconRequestOrigin origin,
+                          bool can_send_history_data) {
   return large_icon_service && CanOriginQueryGoogleServer(origin) &&
          can_send_history_data &&
          base::FeatureList::IsEnabled(kEnableHistoryFaviconsGoogleServerQuery);
 }
 
+}  // namespace
+
 FaviconRequestHandler::FaviconRequestHandler() {}
 
 FaviconRequestHandler::~FaviconRequestHandler() {}
@@ -122,6 +135,7 @@ void FaviconRequestHandler::GetRawFaviconForPageURL(
     int desired_size_in_pixel,
     favicon_base::FaviconRawBitmapCallback callback,
     FaviconRequestOrigin request_origin,
+    FaviconRequestPlatform request_platform,
     FaviconService* favicon_service,
     LargeIconService* large_icon_service,
     const GURL& icon_url_for_uma,
@@ -137,14 +151,14 @@ void FaviconRequestHandler::GetRawFaviconForPageURL(
 
   // First attempt to find the icon locally.
   favicon_service->GetRawFaviconForPageURL(
-      page_url, GetIconTypesForLocalQuery(), desired_size_in_pixel,
-      kFallbackToHost,
+      page_url, GetIconTypesForLocalQuery(request_platform),
+      desired_size_in_pixel, kFallbackToHost,
       base::BindOnce(&FaviconRequestHandler::OnBitmapLocalDataAvailable,
                      weak_ptr_factory_.GetWeakPtr(), page_url,
                      desired_size_in_pixel,
                      /*response_callback=*/std::move(callback), request_origin,
-                     favicon_service, large_icon_service, icon_url_for_uma,
-                     std::move(synced_favicon_getter),
+                     request_platform, favicon_service, large_icon_service,
+                     icon_url_for_uma, std::move(synced_favicon_getter),
                      CanQueryGoogleServer(large_icon_service, request_origin,
                                           can_send_history_data),
                      tracker),
@@ -187,6 +201,7 @@ void FaviconRequestHandler::OnBitmapLocalDataAvailable(
     int desired_size_in_pixel,
     favicon_base::FaviconRawBitmapCallback response_callback,
     FaviconRequestOrigin origin,
+    FaviconRequestPlatform platform,
     FaviconService* favicon_service,
     LargeIconService* large_icon_service,
     const GURL& icon_url_for_uma,
@@ -207,8 +222,15 @@ void FaviconRequestHandler::OnBitmapLocalDataAvailable(
     base::RepeatingCallback<void(const favicon_base::FaviconRawBitmapResult&)>
         repeating_response_callback =
             base::AdaptCallbackForRepeating(std::move(response_callback));
+    // TODO(victorvianna): Set |min_source_size_in_pixel| correctly.
+    std::unique_ptr<FaviconServerFetcherParams> server_parameters =
+        platform == FaviconRequestPlatform::kMobile
+            ? FaviconServerFetcherParams::CreateForMobile(
+                  page_url, /*min_source_size_in_pixel=*/1,
+                  desired_size_in_pixel)
+            : FaviconServerFetcherParams::CreateForDesktop(page_url);
     RequestFromGoogleServer(
-        page_url,
+        page_url, std::move(server_parameters),
         /*empty_response_callback=*/
         base::BindOnce(repeating_response_callback,
                        favicon_base::FaviconRawBitmapResult()),
@@ -216,8 +238,8 @@ void FaviconRequestHandler::OnBitmapLocalDataAvailable(
         base::BindOnce(
             base::IgnoreResult(&FaviconService::GetRawFaviconForPageURL),
             base::Unretained(favicon_service), page_url,
-            GetIconTypesForLocalQuery(), desired_size_in_pixel, kFallbackToHost,
-            repeating_response_callback, tracker),
+            GetIconTypesForLocalQuery(platform), desired_size_in_pixel,
+            kFallbackToHost, repeating_response_callback, tracker),
         large_icon_service, icon_url_for_uma, origin);
     return;
   }
@@ -262,8 +284,10 @@ void FaviconRequestHandler::OnImageLocalDataAvailable(
     base::RepeatingCallback<void(const favicon_base::FaviconImageResult&)>
         repeating_response_callback =
             base::AdaptCallbackForRepeating(std::move(response_callback));
+    // We use CreateForDesktop because GetFaviconImageForPageURL is only called
+    // by desktop.
     RequestFromGoogleServer(
-        page_url,
+        page_url, FaviconServerFetcherParams::CreateForDesktop(page_url),
         /*empty_response_callback=*/
         base::BindOnce(repeating_response_callback,
                        favicon_base::FaviconImageResult()),
@@ -296,6 +320,7 @@ void FaviconRequestHandler::OnImageLocalDataAvailable(
 
 void FaviconRequestHandler::RequestFromGoogleServer(
     const GURL& page_url,
+    std::unique_ptr<FaviconServerFetcherParams> server_parameters,
     base::OnceClosure empty_response_callback,
     base::OnceClosure local_lookup_callback,
     LargeIconService* large_icon_service,
@@ -345,7 +370,7 @@ void FaviconRequestHandler::RequestFromGoogleServer(
       /* default_value= */ false);
   large_icon_service
       ->GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
-          FaviconServerFetcherParams::CreateForDesktop(page_url),
+          std::move(server_parameters),
           /*may_page_url_be_private=*/true, should_trim_url_path,
           traffic_annotation,
           base::BindOnce(&FaviconRequestHandler::OnGoogleServerDataAvailable,

components/favicon/core/favicon_request_handler.h

@@ -16,16 +16,18 @@
 
 namespace favicon {
 
+class FaviconServerFetcherParams;
 class FaviconService;
 class LargeIconService;
 
 // The UI origin of an icon request.
+// TODO(victorvianna): Rename to agree with the naming style of the other enums.
 enum class FaviconRequestOrigin {
   // Unknown origin.
   UNKNOWN,
-  // chrome://history.
+  // History page.
   HISTORY,
-  // chrome://history/syncedTabs.
+  // History synced tabs page (desktop only).
   HISTORY_SYNCED_TABS,
   // Recently closed tabs menu.
   RECENTLY_CLOSED_TABS,
@@ -42,6 +44,12 @@ enum class FaviconAvailability {
   kMaxValue = kNotAvailable,
 };
 
+// Platform making the request.
+enum class FaviconRequestPlatform {
+  kMobile,
+  kDesktop,
+};
+
 // Class for handling favicon requests by page url, forwarding them to local
 // storage, sync or Google server accordingly.
 // TODO(victorvianna): Refactor LargeIconService to avoid having to pass both
@@ -69,10 +77,12 @@ class FaviconRequestHandler {
   // that history sync is enabled and no custom passphrase is set).
   // If a non-empty |icon_url_for_uma| (optional) is passed, it will be used to
   // record UMA about the grouping of requests to the favicon server.
+  // |request_platform| specifies whether the caller is mobile or desktop code.
   void GetRawFaviconForPageURL(const GURL& page_url,
                                int desired_size_in_pixel,
                                favicon_base::FaviconRawBitmapCallback callback,
                                FaviconRequestOrigin request_origin,
+                               FaviconRequestPlatform request_platform,
                                FaviconService* favicon_service,
                                LargeIconService* large_icon_service,
                                const GURL& icon_url_for_uma,
@@ -88,6 +98,7 @@ class FaviconRequestHandler {
   // that history sync is enabled and no custom passphrase is set).
   // If a non-empty |icon_url_for_uma| (optional) is passed, it will be used to
   // record UMA about the grouping of requests to the favicon server.
+  // This method is only called by desktop code.
   void GetFaviconImageForPageURL(const GURL& page_url,
                                  favicon_base::FaviconImageCallback callback,
                                  FaviconRequestOrigin request_origin,
@@ -99,10 +110,6 @@ class FaviconRequestHandler {
                                  base::CancelableTaskTracker* tracker);
 
  private:
-  static bool CanQueryGoogleServer(LargeIconService* large_icon_service,
-                                   FaviconRequestOrigin origin,
-                                   bool can_send_history_data);
-
   // Called after the first attempt to retrieve the icon bitmap from local
   // storage. If request succeeded, sends the result. Otherwise attempts to
   // retrieve from sync or the Google favicon server depending whether
@@ -112,6 +119,7 @@ class FaviconRequestHandler {
       int desired_size_in_pixel,
       favicon_base::FaviconRawBitmapCallback response_callback,
       FaviconRequestOrigin origin,
+      FaviconRequestPlatform platform,
       FaviconService* favicon_service,
       LargeIconService* large_icon_service,
       const GURL& icon_url_for_uma,
@@ -139,12 +147,14 @@ class FaviconRequestHandler {
   // Requests an icon from Google favicon server. Since requests work by
   // populating local storage, a |local_lookup_callback| will be needed in case
   // of success and an |empty_response_callback| in case of failure.
-  void RequestFromGoogleServer(const GURL& page_url,
-                               base::OnceClosure empty_response_callback,
-                               base::OnceClosure local_lookup_callback,
-                               LargeIconService* large_icon_service,
-                               const GURL& icon_url_for_uma,
-                               FaviconRequestOrigin origin);
+  void RequestFromGoogleServer(
+      const GURL& page_url,
+      std::unique_ptr<FaviconServerFetcherParams> server_parameters,
+      base::OnceClosure empty_response_callback,
+      base::OnceClosure local_lookup_callback,
+      LargeIconService* large_icon_service,
+      const GURL& icon_url_for_uma,
+      FaviconRequestOrigin origin);
 
   // Called once the request to the favicon server has finished. If the request
   // succeeded, |local_lookup_callback| is called to effectively retrieve the

components/favicon/core/favicon_request_handler_unittest.cc

@@ -29,6 +29,8 @@ using testing::Invoke;
 
 const char kDummyPageUrl[] = "https://www.example.com";
 const int kDesiredSizeInPixel = 16;
+// TODO(victorvianna): Add unit tests specific for mobile.
+const FaviconRequestPlatform kDummyPlatform = FaviconRequestPlatform::kDesktop;
 const SkColor kTestColor = SK_ColorRED;
 base::CancelableTaskTracker::TaskId kDummyTaskId = 1;
 
@@ -136,7 +138,7 @@ TEST_F(FaviconRequestHandlerTest, ShouldGetEmptyBitmap) {
   favicon_request_handler_.GetRawFaviconForPageURL(
       GURL(kDummyPageUrl), kDesiredSizeInPixel,
       base::BindOnce(&StoreBitmap, &result), FaviconRequestOrigin::UNKNOWN,
-      &mock_favicon_service_, &mock_large_icon_service_,
+      kDummyPlatform, &mock_favicon_service_, &mock_large_icon_service_,
       /*icon_url_for_uma=*/GURL(), synced_favicon_getter_.Get(),
       /*can_send_history_data=*/true, &tracker_);
   EXPECT_FALSE(result.is_valid());
@@ -161,7 +163,7 @@ TEST_F(FaviconRequestHandlerTest, ShouldGetSyncBitmap) {
   favicon_request_handler_.GetRawFaviconForPageURL(
       GURL(kDummyPageUrl), kDesiredSizeInPixel,
       base::BindOnce(&StoreBitmap, &result), FaviconRequestOrigin::UNKNOWN,
-      &mock_favicon_service_, &mock_large_icon_service_,
+      kDummyPlatform, &mock_favicon_service_, &mock_large_icon_service_,
       /*icon_url_for_uma=*/GURL(), synced_favicon_getter_.Get(),
       /*can_send_history_data=*/true, &tracker_);
   EXPECT_TRUE(result.is_valid());
@@ -185,7 +187,7 @@ TEST_F(FaviconRequestHandlerTest, ShouldGetLocalBitmap) {
   favicon_request_handler_.GetRawFaviconForPageURL(
       GURL(kDummyPageUrl), kDesiredSizeInPixel,
       base::BindOnce(&StoreBitmap, &result), FaviconRequestOrigin::UNKNOWN,
-      &mock_favicon_service_, &mock_large_icon_service_,
+      kDummyPlatform, &mock_favicon_service_, &mock_large_icon_service_,
       /*icon_url_for_uma=*/GURL(), synced_favicon_getter_.Get(),
       /*can_send_history_data=*/true, &tracker_);
   EXPECT_TRUE(result.is_valid());
@@ -223,7 +225,7 @@ TEST_F(FaviconRequestHandlerTest, ShouldGetGoogleServerBitmapForFullUrl) {
   favicon_request_handler_.GetRawFaviconForPageURL(
       GURL(kDummyPageUrl), kDesiredSizeInPixel,
       base::BindOnce(&StoreBitmap, &result), FaviconRequestOrigin::HISTORY,
-      &mock_favicon_service_, &mock_large_icon_service_,
+      kDummyPlatform, &mock_favicon_service_, &mock_large_icon_service_,
       /*icon_url_for_uma=*/GURL(), synced_favicon_getter_.Get(),
       /*can_send_history_data=*/true, &tracker_);
   EXPECT_TRUE(result.is_valid());
@@ -259,7 +261,7 @@ TEST_F(FaviconRequestHandlerTest, ShouldGetGoogleServerBitmapForTrimmedUrl) {
   favicon_request_handler_.GetRawFaviconForPageURL(
       GURL(kDummyPageUrl), kDesiredSizeInPixel,
       base::BindOnce(&StoreBitmap, &result), FaviconRequestOrigin::HISTORY,
-      &mock_favicon_service_, &mock_large_icon_service_,
+      kDummyPlatform, &mock_favicon_service_, &mock_large_icon_service_,
       /*icon_url_for_uma=*/GURL(), synced_favicon_getter_.Get(),
       /*can_send_history_data=*/true, &tracker_);
   EXPECT_TRUE(result.is_valid());
@@ -284,8 +286,9 @@ TEST_F(FaviconRequestHandlerTest, ShouldGetEmptyImage) {
   favicon_request_handler_.GetFaviconImageForPageURL(
       GURL(kDummyPageUrl), base::BindOnce(&StoreImage, &result),
       FaviconRequestOrigin::UNKNOWN, &mock_favicon_service_,
-      &mock_large_icon_service_, /*icon_url_for_uma=*/GURL(),
-      synced_favicon_getter_.Get(), /*can_send_history_data=*/true, &tracker_);
+      &mock_large_icon_service_,
+      /*icon_url_for_uma=*/GURL(), synced_favicon_getter_.Get(),
+      /*can_send_history_data=*/true, &tracker_);
   EXPECT_TRUE(result.image.IsEmpty());
   histogram_tester_.ExpectUniqueSample("Sync.FaviconAvailability.UNKNOWN",
                                        FaviconAvailability::kNotAvailable, 1);
@@ -306,8 +309,9 @@ TEST_F(FaviconRequestHandlerTest, ShouldGetSyncImage) {
   favicon_request_handler_.GetFaviconImageForPageURL(
       GURL(kDummyPageUrl), base::BindOnce(&StoreImage, &result),
       FaviconRequestOrigin::UNKNOWN, &mock_favicon_service_,
-      &mock_large_icon_service_, /*icon_url_for_uma=*/GURL(),
-      synced_favicon_getter_.Get(), /*can_send_history_data=*/true, &tracker_);
+      &mock_large_icon_service_,
+      /*icon_url_for_uma=*/GURL(), synced_favicon_getter_.Get(),
+      /*can_send_history_data=*/true, &tracker_);
   EXPECT_FALSE(result.image.IsEmpty());
   histogram_tester_.ExpectUniqueSample("Sync.FaviconAvailability.UNKNOWN",
                                        FaviconAvailability::kSync, 1);
@@ -327,8 +331,9 @@ TEST_F(FaviconRequestHandlerTest, ShouldGetLocalImage) {
   favicon_request_handler_.GetFaviconImageForPageURL(
       GURL(kDummyPageUrl), base::BindOnce(&StoreImage, &result),
       FaviconRequestOrigin::UNKNOWN, &mock_favicon_service_,
-      &mock_large_icon_service_, /*icon_url_for_uma=*/GURL(),
-      synced_favicon_getter_.Get(), /*can_send_history_data=*/true, &tracker_);
+      &mock_large_icon_service_,
+      /*icon_url_for_uma=*/GURL(), synced_favicon_getter_.Get(),
+      /*can_send_history_data=*/true, &tracker_);
   EXPECT_FALSE(result.image.IsEmpty());
   histogram_tester_.ExpectUniqueSample("Sync.FaviconAvailability.UNKNOWN",
                                        FaviconAvailability::kLocal, 1);
@@ -424,7 +429,7 @@ TEST_F(FaviconRequestHandlerTest, ShouldNotQueryGoogleServerIfCannotSendData) {
   favicon_request_handler_.GetRawFaviconForPageURL(
       GURL(kDummyPageUrl), kDesiredSizeInPixel,
       base::BindOnce(&StoreBitmap, &result), FaviconRequestOrigin::HISTORY,
-      &mock_favicon_service_, &mock_large_icon_service_,
+      kDummyPlatform, &mock_favicon_service_, &mock_large_icon_service_,
       /*icon_url_for_uma=*/GURL(), synced_favicon_getter_.Get(),
       /*can_send_history_data=*/false, &tracker_);
 }