Remove fallback to FaviconCache from HistoryUiFaviconRequestHandler

Previous to this CL, if a) HistoryUiFaviconRequestHandler could not
find an icon in the local favicon database and b) Chrome was not allowed
to query the Google Favicon Server, then the layer would make a fallback
request to FaviconCache, the in-memory cache for synced icons.

Favicon sync has been stopped (https://crrev.com/c/1917173), which means
a) FaviconCache is no longer populated with remote data.
b) It is still populated with local data, however that data never gets
synced up.

This CL removes the fallback code to FaviconCache, under the assumption
that, except for rare cases, an icon would not be found in the in-memory
cache that was not found in the local database.

Bug: 978775
Change-Id: I9d4f92b19eb6a31ebd06b3579ffe1794b9737b00
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2085375
Commit-Queue: Victor Vianna <victorvianna@google.com>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#749170}

chrome/browser/favicon/history_ui_favicon_request_handler_factory.cc

@@ -11,27 +11,14 @@
 #include "chrome/browser/profiles/incognito_helpers.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
-#include "chrome/browser/sync/session_sync_service_factory.h"
 #include "components/favicon/core/history_ui_favicon_request_handler_impl.h"
-#include "components/favicon_base/favicon_types.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "components/sync/driver/sync_service.h"
 #include "components/sync/driver/sync_service_utils.h"
-#include "components/sync_sessions/open_tabs_ui_delegate.h"
-#include "components/sync_sessions/session_sync_service.h"
 #include "content/public/browser/browser_context.h"
 
 namespace {
 
-favicon_base::FaviconRawBitmapResult GetSyncedFaviconForPageUrl(
-    sync_sessions::SessionSyncService* session_sync_service,
-    const GURL& page_url) {
-  sync_sessions::OpenTabsUIDelegate* open_tabs =
-      session_sync_service->GetOpenTabsUIDelegate();
-  return open_tabs ? open_tabs->GetSyncedFaviconForPageURL(page_url)
-                   : favicon_base::FaviconRawBitmapResult();
-}
-
 bool CanSendHistoryData(syncer::SyncService* sync_service) {
   return syncer::GetUploadToGoogleState(sync_service,
                                         syncer::ModelType::SESSIONS) ==
@@ -60,7 +47,6 @@ HistoryUiFaviconRequestHandlerFactory::HistoryUiFaviconRequestHandlerFactory()
           BrowserContextDependencyManager::GetInstance()) {
   DependsOn(FaviconServiceFactory::GetInstance());
   DependsOn(LargeIconServiceFactory::GetInstance());
-  DependsOn(SessionSyncServiceFactory::GetInstance());
   DependsOn(ProfileSyncServiceFactory::GetInstance());
 }
 
@@ -77,8 +63,6 @@ KeyedService* HistoryUiFaviconRequestHandlerFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
   Profile* profile = Profile::FromBrowserContext(context);
   return new favicon::HistoryUiFaviconRequestHandlerImpl(
-      base::BindRepeating(&GetSyncedFaviconForPageUrl,
-                          SessionSyncServiceFactory::GetForProfile(profile)),
       base::BindRepeating(&CanSendHistoryData,
                           ProfileSyncServiceFactory::GetForProfile(profile)),
       FaviconServiceFactory::GetForProfile(profile,

components/favicon/core/history_ui_favicon_request_handler.h

@@ -23,16 +23,16 @@ enum class HistoryUiFaviconRequestOrigin {
 };
 
 // Keyed service for handling favicon requests made by a history UI, forwarding
-// them to local storage, sync or Google server accordingly. This service should
+// them to local storage or Google server accordingly. This service should
 // only be used by the UIs listed in the HistoryUiFaviconRequestOrigin enum.
 // Requests must be made by page url, as opposed to icon url.
 class HistoryUiFaviconRequestHandler : public KeyedService {
  public:
   // Requests favicon bitmap at |page_url| of size |desired_size_in_pixel|.
-  // Tries to fetch the icon from local storage and falls back to sync, or the
-  // Google favicon server if user settings allow to query it using history
-  // data. 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.
+  // Tries to fetch the icon from local storage and falls back to the Google
+  // favicon server if user settings allow to query it using history data.
+  // 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.
   virtual void GetRawFaviconForPageURL(
       const GURL& page_url,
       int desired_size_in_pixel,
@@ -40,12 +40,8 @@ class HistoryUiFaviconRequestHandler : public KeyedService {
       HistoryUiFaviconRequestOrigin request_origin_for_uma,
       const GURL& icon_url_for_uma) = 0;
 
-  // Requests favicon image at |page_url|.
-  // Tries to fetch the icon from local storage and falls back to sync, or the
-  // Google favicon server if user settings allow to query it using history
-  // data.
-  // 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.
+  // Requests favicon image at |page_url|. The same fallback considerations for
+  // GetRawFaviconForPageURL apply.
   // This method is only called by desktop code.
   virtual void GetFaviconImageForPageURL(
       const GURL& page_url,

components/favicon/core/history_ui_favicon_request_handler_impl.cc

@@ -97,13 +97,11 @@ GURL GetGroupIdentifier(const GURL& page_url, const GURL& icon_url) {
 }  // namespace
 
 HistoryUiFaviconRequestHandlerImpl::HistoryUiFaviconRequestHandlerImpl(
-    const SyncedFaviconGetter& synced_favicon_getter,
     const CanSendHistoryDataGetter& can_send_history_data_getter,
     FaviconService* favicon_service,
     LargeIconService* large_icon_service)
     : favicon_service_(favicon_service),
       large_icon_service_(large_icon_service),
-      synced_favicon_getter_(synced_favicon_getter),
       can_send_history_data_getter_(can_send_history_data_getter) {
   DCHECK(favicon_service);
   DCHECK(large_icon_service);
@@ -186,19 +184,7 @@ void HistoryUiFaviconRequestHandlerImpl::OnBitmapLocalDataAvailable(
     return;
   }
 
-  favicon_base::FaviconRawBitmapResult sync_bitmap_result =
-      synced_favicon_getter_.Run(page_url);
-  if (sync_bitmap_result.is_valid()) {
-    // If request to sync succeeds, resize bitmap to desired size and send.
-    RecordFaviconAvailabilityAndLatencyMetric(
-        origin_for_uma, request_start_time_for_uma, FaviconAvailability::kSync);
-    std::move(response_callback)
-        .Run(favicon_base::ResizeFaviconBitmapResult({sync_bitmap_result},
-                                                     desired_size_in_pixel));
-    return;
-  }
-
-  // If sync does not have the favicon, send empty response.
+  // Send empty response.
   RecordFaviconAvailabilityAndLatencyMetric(origin_for_uma,
                                             request_start_time_for_uma,
                                             FaviconAvailability::kNotAvailable);
@@ -246,21 +232,7 @@ void HistoryUiFaviconRequestHandlerImpl::OnImageLocalDataAvailable(
     return;
   }
 
-  favicon_base::FaviconRawBitmapResult sync_bitmap_result =
-      synced_favicon_getter_.Run(page_url);
-  if (sync_bitmap_result.is_valid()) {
-    // If request to sync succeeds, convert the retrieved bitmap to image and
-    // send.
-    RecordFaviconAvailabilityAndLatencyMetric(
-        origin_for_uma, request_start_time_for_uma, FaviconAvailability::kSync);
-    favicon_base::FaviconImageResult sync_image_result;
-    sync_image_result.image =
-        gfx::Image::CreateFrom1xPNGBytes(sync_bitmap_result.bitmap_data.get());
-    std::move(response_callback).Run(sync_image_result);
-    return;
-  }
-
-  // If sync does not have the favicon, send empty response.
+  // Send empty response.
   RecordFaviconAvailabilityAndLatencyMetric(origin_for_uma,
                                             request_start_time_for_uma,
                                             FaviconAvailability::kNotAvailable);

components/favicon/core/history_ui_favicon_request_handler_impl.h

@@ -22,8 +22,9 @@ class LargeIconService;
 enum class FaviconAvailability {
   // Icon recovered from local storage (but may originally come from server).
   kLocal = 0,
+  // DEPRECATED: No icon is retrieved using sync in this layer anymore.
   // Icon recovered using sync.
-  kSync = 1,
+  kDeprecatedSync = 1,
   // Icon not found.
   kNotAvailable = 2,
   kMaxValue = kNotAvailable,
@@ -33,18 +34,12 @@ enum class FaviconAvailability {
 class HistoryUiFaviconRequestHandlerImpl
     : public HistoryUiFaviconRequestHandler {
  public:
-  // Callback that requests the synced bitmap for a page url.
-  using SyncedFaviconGetter =
-      base::RepeatingCallback<favicon_base::FaviconRawBitmapResult(
-          const GURL&)>;
-
   // Callback that checks whether user settings allow to query the favicon
   // server using history data (in particular it must check that history sync is
   // enabled and no custom passphrase is set).
   using CanSendHistoryDataGetter = base::RepeatingCallback<bool()>;
 
   HistoryUiFaviconRequestHandlerImpl(
-      const SyncedFaviconGetter& synced_favicon_getter,
       const CanSendHistoryDataGetter& can_send_history_data_getter,
       FaviconService* favicon_service,
       LargeIconService* large_icon_service);
@@ -65,9 +60,9 @@ class HistoryUiFaviconRequestHandlerImpl
 
  private:
   // 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 on the result
-  // given by |can_send_history_data_getter_|.
+  // storage. If request succeeded, sends the result. Otherwise, if allowed by
+  // user settings, (i.e. if |can_send_history_data_getter_| returns true),
+  // attempts to retrieve from the Google favicon server.
   void OnBitmapLocalDataAvailable(
       const GURL& page_url,
       int desired_size_in_pixel,
@@ -78,9 +73,9 @@ class HistoryUiFaviconRequestHandlerImpl
       const favicon_base::FaviconRawBitmapResult& bitmap_result);
 
   // Called after the first attempt to retrieve the icon image from local
-  // storage. If request succeeded, sends the result. Otherwise attempts to
-  // retrieve from sync or the Google favicon server depending on the result
-  // given by |can_send_history_data_getter_|.
+  // storage. If request succeeded, sends the result. Otherwise, if allowed by
+  // user settings, (i.e. if |can_send_history_data_getter_| returns true),
+  // attempts to retrieve from the Google favicon server.
   void OnImageLocalDataAvailable(
       const GURL& page_url,
       favicon_base::FaviconImageCallback response_callback,
@@ -117,8 +112,6 @@ class HistoryUiFaviconRequestHandlerImpl
 
   LargeIconService* const large_icon_service_;
 
-  SyncedFaviconGetter const synced_favicon_getter_;
-
   CanSendHistoryDataGetter const can_send_history_data_getter_;
 
   // Map from a group identifier to the number of callbacks in that group which

components/favicon/core/history_ui_favicon_request_handler_impl_unittest.cc

@@ -211,26 +211,16 @@ class HistoryUiFaviconRequestHandlerImplTest : public ::testing::Test {
  public:
   HistoryUiFaviconRequestHandlerImplTest()
       : mock_large_icon_service_(&mock_favicon_service_),
-        history_ui_favicon_request_handler_(synced_favicon_getter_.Get(),
-                                            can_send_history_data_getter_.Get(),
+        history_ui_favicon_request_handler_(can_send_history_data_getter_.Get(),
                                             &mock_favicon_service_,
                                             &mock_large_icon_service_) {
     // Allow sending history data by default.
     ON_CALL(can_send_history_data_getter_, Run()).WillByDefault(Return(true));
-
-    // Sync will by default respond it does not contain any icon. Same is done
-    // for the FaviconService and LargeIconService fakes in their constructors.
-    ON_CALL(synced_favicon_getter_, Run(_)).WillByDefault([](auto) {
-      return favicon_base::FaviconRawBitmapResult();
-    });
   }
 
  protected:
   testing::NiceMock<MockFaviconServiceWithFake> mock_favicon_service_;
   testing::NiceMock<MockLargeIconServiceWithFake> mock_large_icon_service_;
-  testing::NiceMock<base::MockCallback<
-      HistoryUiFaviconRequestHandlerImpl::SyncedFaviconGetter>>
-      synced_favicon_getter_;
   testing::NiceMock<base::MockCallback<
       HistoryUiFaviconRequestHandlerImpl::CanSendHistoryDataGetter>>
       can_send_history_data_getter_;
@@ -245,7 +235,6 @@ TEST_F(HistoryUiFaviconRequestHandlerImplTest, ShouldGetEmptyBitmap) {
   EXPECT_CALL(mock_favicon_service_,
               GetRawFaviconForPageURL(GURL(kDummyPageUrl), _,
                                       kDefaultDesiredSizeInPixel, _, _, _));
-  EXPECT_CALL(synced_favicon_getter_, Run(_)).Times(0);
   favicon_base::FaviconRawBitmapResult result;
   history_ui_favicon_request_handler_.GetRawFaviconForPageURL(
       GURL(kDummyPageUrl), kDefaultDesiredSizeInPixel,
@@ -259,26 +248,6 @@ TEST_F(HistoryUiFaviconRequestHandlerImplTest, ShouldGetEmptyBitmap) {
       std::string(kLatencyHistogramName) + kDummyOriginHistogramSuffix, 1);
 }
 
-TEST_F(HistoryUiFaviconRequestHandlerImplTest, ShouldGetSyncBitmap) {
-  ON_CALL(can_send_history_data_getter_, Run()).WillByDefault(Return(false));
-  EXPECT_CALL(mock_favicon_service_,
-              GetRawFaviconForPageURL(GURL(kDummyPageUrl), _,
-                                      kDefaultDesiredSizeInPixel, _, _, _));
-  EXPECT_CALL(synced_favicon_getter_, Run(GURL(kDummyPageUrl)))
-      .WillOnce([](auto) { return CreateTestBitmapResult(); });
-  favicon_base::FaviconRawBitmapResult result;
-  history_ui_favicon_request_handler_.GetRawFaviconForPageURL(
-      GURL(kDummyPageUrl), kDefaultDesiredSizeInPixel,
-      base::BindOnce(&StoreBitmap, &result), kDummyOrigin,
-      /*icon_url_for_uma=*/GURL());
-  EXPECT_TRUE(result.is_valid());
-  histogram_tester_.ExpectUniqueSample(
-      std::string(kAvailabilityHistogramName) + kDummyOriginHistogramSuffix,
-      FaviconAvailability::kSync, 1);
-  histogram_tester_.ExpectTotalCount(
-      std::string(kLatencyHistogramName) + kDummyOriginHistogramSuffix, 1);
-}
-
 TEST_F(HistoryUiFaviconRequestHandlerImplTest, ShouldGetLocalBitmap) {
   mock_favicon_service_.StoreMockLocalFavicon(GURL(kDummyPageUrl));
   EXPECT_CALL(mock_favicon_service_,
@@ -286,7 +255,6 @@ TEST_F(HistoryUiFaviconRequestHandlerImplTest, ShouldGetLocalBitmap) {
                                       kDefaultDesiredSizeInPixel, _, _, _));
   EXPECT_CALL(mock_large_icon_service_,
               TouchIconFromGoogleServer(GURL(kDummyIconUrl)));
-  EXPECT_CALL(synced_favicon_getter_, Run(_)).Times(0);
   favicon_base::FaviconRawBitmapResult result;
   history_ui_favicon_request_handler_.GetRawFaviconForPageURL(
       GURL(kDummyPageUrl), kDefaultDesiredSizeInPixel,
@@ -329,7 +297,6 @@ TEST_F(HistoryUiFaviconRequestHandlerImplTest, ShouldGetGoogleServerBitmap) {
 TEST_F(HistoryUiFaviconRequestHandlerImplTest, ShouldGetEmptyImage) {
   EXPECT_CALL(mock_favicon_service_,
               GetFaviconImageForPageURL(GURL(kDummyPageUrl), _, _));
-  EXPECT_CALL(synced_favicon_getter_, Run(_)).Times(0);
   favicon_base::FaviconImageResult result;
   history_ui_favicon_request_handler_.GetFaviconImageForPageURL(
       GURL(kDummyPageUrl), base::BindOnce(&StoreImage, &result), kDummyOrigin,
@@ -342,31 +309,12 @@ TEST_F(HistoryUiFaviconRequestHandlerImplTest, ShouldGetEmptyImage) {
       std::string(kLatencyHistogramName) + kDummyOriginHistogramSuffix, 1);
 }
 
-TEST_F(HistoryUiFaviconRequestHandlerImplTest, ShouldGetSyncImage) {
-  ON_CALL(can_send_history_data_getter_, Run()).WillByDefault(Return(false));
-  EXPECT_CALL(mock_favicon_service_,
-              GetFaviconImageForPageURL(GURL(kDummyPageUrl), _, _));
-  EXPECT_CALL(synced_favicon_getter_, Run(GURL(kDummyPageUrl)))
-      .WillOnce([](auto) { return CreateTestBitmapResult(); });
-  favicon_base::FaviconImageResult result;
-  history_ui_favicon_request_handler_.GetFaviconImageForPageURL(
-      GURL(kDummyPageUrl), base::BindOnce(&StoreImage, &result), kDummyOrigin,
-      /*icon_url_for_uma=*/GURL());
-  EXPECT_FALSE(result.image.IsEmpty());
-  histogram_tester_.ExpectUniqueSample(
-      std::string(kAvailabilityHistogramName) + kDummyOriginHistogramSuffix,
-      FaviconAvailability::kSync, 1);
-  histogram_tester_.ExpectTotalCount(
-      std::string(kLatencyHistogramName) + kDummyOriginHistogramSuffix, 1);
-}
-
 TEST_F(HistoryUiFaviconRequestHandlerImplTest, ShouldGetLocalImage) {
   mock_favicon_service_.StoreMockLocalFavicon(GURL(kDummyPageUrl));
   EXPECT_CALL(mock_favicon_service_,
               GetFaviconImageForPageURL(GURL(kDummyPageUrl), _, _));
   EXPECT_CALL(mock_large_icon_service_,
               TouchIconFromGoogleServer(GURL(kDummyIconUrl)));
-  EXPECT_CALL(synced_favicon_getter_, Run(_)).Times(0);
   favicon_base::FaviconImageResult result;
   history_ui_favicon_request_handler_.GetFaviconImageForPageURL(
       GURL(kDummyPageUrl), base::BindOnce(&StoreImage, &result), kDummyOrigin,
@@ -389,7 +337,6 @@ TEST_F(HistoryUiFaviconRequestHandlerImplTest, ShouldGetGoogleServerImage) {
               GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
                   GURL(kDummyPageUrl), _,
                   /*should_trim_url_path=*/false, _, _));
-  EXPECT_CALL(synced_favicon_getter_, Run(_)).Times(0);
   favicon_base::FaviconImageResult result;
   history_ui_favicon_request_handler_.GetFaviconImageForPageURL(
       GURL(kDummyPageUrl), base::BindOnce(&StoreImage, &result), kDummyOrigin,
@@ -428,29 +375,5 @@ TEST_F(HistoryUiFaviconRequestHandlerImplTest,
       /*icon_url_for_uma=*/GURL());
 }
 
-TEST_F(HistoryUiFaviconRequestHandlerImplTest, ShouldResizeSyncBitmap) {
-  const int kDesiredSizeInPixel = 32;
-  ON_CALL(can_send_history_data_getter_, Run()).WillByDefault(Return(false));
-  EXPECT_CALL(mock_favicon_service_,
-              GetRawFaviconForPageURL(GURL(kDummyPageUrl), _,
-                                      kDesiredSizeInPixel, _, _, _))
-      .WillOnce([](auto, auto, auto, auto,
-                   favicon_base::FaviconRawBitmapCallback callback, auto) {
-        std::move(callback).Run(favicon_base::FaviconRawBitmapResult());
-        return kDummyTaskId;
-      });
-  // Have sync return bitmap of different size from the one requested.
-  EXPECT_CALL(synced_favicon_getter_, Run(GURL(kDummyPageUrl)))
-      .WillOnce([](auto) { return CreateTestBitmapResult(16); });
-  favicon_base::FaviconRawBitmapResult result;
-  history_ui_favicon_request_handler_.GetRawFaviconForPageURL(
-      GURL(kDummyPageUrl), kDesiredSizeInPixel,
-      base::BindOnce(&StoreBitmap, &result), kDummyOrigin,
-      /*icon_url_for_uma=*/GURL());
-  EXPECT_TRUE(result.is_valid());
-  EXPECT_EQ(gfx::Size(kDesiredSizeInPixel, kDesiredSizeInPixel),
-            result.pixel_size);
-}
-
 }  // namespace
 }  // namespace favicon