Inject SyncedFaviconGetter in constructor of FaviconRequestHandler

Change-Id: Id967aba3dcbb5e08311f4f9ccc913dc91d664aa3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1660501Reviewed-by: David Trainor <dtrainor@chromium.org>
Reviewed-by: Esmael El-Moslimany <aee@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Victor Vianna <victorvianna@google.com>
Cr-Commit-Position: refs/heads/master@{#670158}

chrome/browser/android/favicon_helper.cc

@@ -80,18 +80,6 @@ void OnEnsureIconIsAvailableFinished(
       env, j_availability_callback, newly_available);
 }
 
-scoped_refptr<base::RefCountedMemory> GetSyncedFaviconForPageURL(
-    Profile* profile,
-    const GURL& page_url) {
-  sync_sessions::SessionSyncService* session_sync_service =
-      SessionSyncServiceFactory::GetInstance()->GetForProfile(profile);
-  DCHECK(session_sync_service);
-  sync_sessions::OpenTabsUIDelegate* open_tabs =
-      session_sync_service->GetOpenTabsUIDelegate();
-  return open_tabs ? open_tabs->GetSyncedFaviconForPageURL(page_url.spec())
-                   : nullptr;
-}
-
 // Check if user settings allow querying a Google server using history
 // information.
 bool CanSendHistoryDataToServer(Profile* profile) {
@@ -188,7 +176,6 @@ jboolean FaviconHelper::GetForeignFaviconImageForURL(
       favicon::FaviconRequestPlatform::kMobile,
       /*icon_url_for_uma=*/
       open_tabs ? open_tabs->GetIconUrlForPageUrl(page_url) : GURL(),
-      base::BindOnce(&GetSyncedFaviconForPageURL, profile),
       CanSendHistoryDataToServer(profile), cancelable_task_tracker_.get());
   return true;
 }

chrome/browser/favicon/favicon_request_handler_factory.cc

@@ -10,10 +10,26 @@
 #include "chrome/browser/favicon/large_icon_service_factory.h"
 #include "chrome/browser/profiles/incognito_helpers.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/sync/session_sync_service_factory.h"
 #include "components/favicon/core/favicon_request_handler.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.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 {
+
+scoped_refptr<base::RefCountedMemory> 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.spec())
+                   : nullptr;
+}
+
+}  // namespace
+
 // static
 favicon::FaviconRequestHandler*
 FaviconRequestHandlerFactory::GetForBrowserContext(
@@ -33,6 +49,7 @@ FaviconRequestHandlerFactory::FaviconRequestHandlerFactory()
           BrowserContextDependencyManager::GetInstance()) {
   DependsOn(FaviconServiceFactory::GetInstance());
   DependsOn(LargeIconServiceFactory::GetInstance());
+  DependsOn(SessionSyncServiceFactory::GetInstance());
 }
 
 FaviconRequestHandlerFactory::~FaviconRequestHandlerFactory() {}
@@ -46,6 +63,8 @@ KeyedService* FaviconRequestHandlerFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
   Profile* profile = Profile::FromBrowserContext(context);
   return new favicon::FaviconRequestHandler(
+      base::BindRepeating(&GetSyncedFaviconForPageUrl,
+                          SessionSyncServiceFactory::GetForProfile(profile)),
       FaviconServiceFactory::GetForProfile(profile,
                                            ServiceAccessType::EXPLICIT_ACCESS),
       LargeIconServiceFactory::GetForBrowserContext(context));

chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc

@@ -134,16 +134,6 @@ gfx::Image CreateFavicon(const gfx::VectorIcon& icon) {
                                 ui::NativeTheme::kColorId_DefaultIconColor)));
 }
 
-scoped_refptr<base::RefCountedMemory> RecentTabsGetSyncedFaviconForPageURL(
-    sync_sessions::SessionSyncService* session_sync_service,
-    const GURL& page_url) {
-  DCHECK(session_sync_service);
-  sync_sessions::OpenTabsUIDelegate* open_tabs =
-      session_sync_service->GetOpenTabsUIDelegate();
-  return open_tabs ? open_tabs->GetSyncedFaviconForPageURL(page_url.spec())
-                   : nullptr;
-}
-
 // Check if user settings allow querying a Google server using history
 // information.
 bool CanSendHistoryDataToServer(bool is_local_tab, Browser* browser) {
@@ -608,8 +598,6 @@ void RecentTabsSubMenuModel::AddTabFavicon(int command_id, const GURL& url) {
                      base::Unretained(this), command_id),
       favicon::FaviconRequestOrigin::RECENTLY_CLOSED_TABS,
       open_tabs ? open_tabs->GetIconUrlForPageUrl(url) : GURL(),
-      base::BindOnce(&RecentTabsGetSyncedFaviconForPageURL,
-                     base::Unretained(session_sync_service_)),
       CanSendHistoryDataToServer(is_local_tab, browser_),
       is_local_tab ? &local_tab_cancelable_task_tracker_
                    : &other_devices_tab_cancelable_task_tracker_);

chrome/browser/ui/webui/favicon_source.cc

@@ -44,21 +44,6 @@ favicon::FaviconRequestOrigin ParseFaviconRequestOrigin(const GURL& url) {
   return favicon::FaviconRequestOrigin::UNKNOWN;
 }
 
-sync_sessions::OpenTabsUIDelegate* GetOpenTabsUIDelegate(Profile* profile) {
-  sync_sessions::SessionSyncService* session_sync_service =
-      SessionSyncServiceFactory::GetInstance()->GetForProfile(profile);
-  DCHECK(session_sync_service);
-  return session_sync_service->GetOpenTabsUIDelegate();
-}
-
-scoped_refptr<base::RefCountedMemory> GetSyncedFaviconForPageURL(
-    Profile* profile,
-    const GURL& page_url) {
-  sync_sessions::OpenTabsUIDelegate* open_tabs = GetOpenTabsUIDelegate(profile);
-  return open_tabs ? open_tabs->GetSyncedFaviconForPageURL(page_url.spec())
-                   : nullptr;
-}
-
 // Check if user settings allow querying a Google server using history
 // information.
 bool CanSendHistoryDataToServer(Profile* profile) {
@@ -166,8 +151,10 @@ void FaviconSource::StartDataRequest(
       SendDefaultResponse(callback);
       return;
     }
+    sync_sessions::SessionSyncService* session_sync_service =
+        SessionSyncServiceFactory::GetInstance()->GetForProfile(profile_);
     sync_sessions::OpenTabsUIDelegate* open_tabs =
-        GetOpenTabsUIDelegate(profile_);
+        session_sync_service->GetOpenTabsUIDelegate();
     favicon_request_handler->GetRawFaviconForPageURL(
         url, desired_size_in_pixel,
         base::BindOnce(&FaviconSource::OnFaviconDataAvailable,
@@ -177,7 +164,6 @@ void FaviconSource::StartDataRequest(
         unsafe_request_origin, favicon::FaviconRequestPlatform::kDesktop,
         /*icon_url_for_uma=*/
         open_tabs ? open_tabs->GetIconUrlForPageUrl(url) : GURL(),
-        base::BindOnce(&GetSyncedFaviconForPageURL, base::Unretained(profile_)),
         CanSendHistoryDataToServer(profile_), &cancelable_task_tracker_);
   }
 }

components/favicon/core/favicon_request_handler.cc

@@ -113,10 +113,12 @@ bool CanQueryGoogleServer(FaviconRequestOrigin origin,
 }  // namespace
 
 FaviconRequestHandler::FaviconRequestHandler(
+    const SyncedFaviconGetter& synced_favicon_getter,
     FaviconService* favicon_service,
     LargeIconService* large_icon_service)
     : favicon_service_(favicon_service),
-      large_icon_service_(large_icon_service) {
+      large_icon_service_(large_icon_service),
+      synced_favicon_getter_(synced_favicon_getter) {
   DCHECK(favicon_service);
   DCHECK(large_icon_service);
 }
@@ -130,7 +132,6 @@ void FaviconRequestHandler::GetRawFaviconForPageURL(
     FaviconRequestOrigin request_origin,
     FaviconRequestPlatform request_platform,
     const GURL& icon_url_for_uma,
-    FaviconRequestHandler::SyncedFaviconGetter synced_favicon_getter,
     bool can_send_history_data,
     base::CancelableTaskTracker* tracker) {
   // First attempt to find the icon locally.
@@ -141,7 +142,7 @@ void FaviconRequestHandler::GetRawFaviconForPageURL(
           &FaviconRequestHandler::OnBitmapLocalDataAvailable,
           weak_ptr_factory_.GetWeakPtr(), page_url, desired_size_in_pixel,
           /*response_callback=*/std::move(callback), request_origin,
-          request_platform, icon_url_for_uma, std::move(synced_favicon_getter),
+          request_platform, icon_url_for_uma,
           CanQueryGoogleServer(request_origin, can_send_history_data), tracker),
       tracker);
 }
@@ -151,7 +152,6 @@ void FaviconRequestHandler::GetFaviconImageForPageURL(
     favicon_base::FaviconImageCallback callback,
     FaviconRequestOrigin request_origin,
     const GURL& icon_url_for_uma,
-    FaviconRequestHandler::SyncedFaviconGetter synced_favicon_getter,
     bool can_send_history_data,
     base::CancelableTaskTracker* tracker) {
   // First attempt to find the icon locally.
@@ -161,7 +161,7 @@ void FaviconRequestHandler::GetFaviconImageForPageURL(
           &FaviconRequestHandler::OnImageLocalDataAvailable,
           weak_ptr_factory_.GetWeakPtr(), page_url,
           /*response_callback=*/std::move(callback), request_origin,
-          icon_url_for_uma, std::move(synced_favicon_getter),
+          icon_url_for_uma,
           CanQueryGoogleServer(request_origin, can_send_history_data), tracker),
       tracker);
 }
@@ -173,7 +173,6 @@ void FaviconRequestHandler::OnBitmapLocalDataAvailable(
     FaviconRequestOrigin origin,
     FaviconRequestPlatform platform,
     const GURL& icon_url_for_uma,
-    FaviconRequestHandler::SyncedFaviconGetter synced_favicon_getter,
     bool can_query_google_server,
     base::CancelableTaskTracker* tracker,
     const favicon_base::FaviconRawBitmapResult& bitmap_result) {
@@ -214,7 +213,7 @@ void FaviconRequestHandler::OnBitmapLocalDataAvailable(
   }
 
   scoped_refptr<base::RefCountedMemory> sync_bitmap =
-      std::move(synced_favicon_getter).Run(page_url);
+      synced_favicon_getter_.Run(page_url);
   if (sync_bitmap) {
     // If request to sync succeeds, send the retrieved bitmap.
     RecordFaviconAvailabilityMetric(origin, FaviconAvailability::kSync);
@@ -234,7 +233,6 @@ void FaviconRequestHandler::OnImageLocalDataAvailable(
     favicon_base::FaviconImageCallback response_callback,
     FaviconRequestOrigin origin,
     const GURL& icon_url_for_uma,
-    FaviconRequestHandler::SyncedFaviconGetter synced_favicon_getter,
     bool can_query_google_server,
     base::CancelableTaskTracker* tracker,
     const favicon_base::FaviconImageResult& image_result) {
@@ -269,7 +267,7 @@ void FaviconRequestHandler::OnImageLocalDataAvailable(
   }
 
   scoped_refptr<base::RefCountedMemory> sync_bitmap =
-      std::move(synced_favicon_getter).Run(page_url);
+      synced_favicon_getter_.Run(page_url);
   if (sync_bitmap) {
     // If request to sync succeeds, convert the retrieved bitmap to image and
     // send.

components/favicon/core/favicon_request_handler.h

@@ -61,9 +61,11 @@ class FaviconRequestHandler : public KeyedService {
  public:
   // Callback that requests the synced bitmap for a page url.
   using SyncedFaviconGetter =
-      base::OnceCallback<scoped_refptr<base::RefCountedMemory>(const GURL&)>;
+      base::RepeatingCallback<scoped_refptr<base::RefCountedMemory>(
+          const GURL&)>;
 
-  FaviconRequestHandler(FaviconService* favicon_service,
+  FaviconRequestHandler(const SyncedFaviconGetter& synced_favicon_getter,
+                        FaviconService* favicon_service,
                         LargeIconService* large_icon_service);
 
   ~FaviconRequestHandler() override;
@@ -83,7 +85,6 @@ class FaviconRequestHandler : public KeyedService {
                                FaviconRequestOrigin request_origin,
                                FaviconRequestPlatform request_platform,
                                const GURL& icon_url_for_uma,
-                               SyncedFaviconGetter synced_favicon_getter,
                                bool can_send_history_data,
                                base::CancelableTaskTracker* tracker);
 
@@ -100,7 +101,6 @@ class FaviconRequestHandler : public KeyedService {
                                  favicon_base::FaviconImageCallback callback,
                                  FaviconRequestOrigin request_origin,
                                  const GURL& icon_url_for_uma,
-                                 SyncedFaviconGetter synced_favicon_getter,
                                  bool can_send_history_data,
                                  base::CancelableTaskTracker* tracker);
 
@@ -116,7 +116,6 @@ class FaviconRequestHandler : public KeyedService {
       FaviconRequestOrigin origin,
       FaviconRequestPlatform platform,
       const GURL& icon_url_for_uma,
-      SyncedFaviconGetter synced_favicon_getter,
       bool can_query_google_server,
       base::CancelableTaskTracker* tracker,
       const favicon_base::FaviconRawBitmapResult& bitmap_result);
@@ -130,7 +129,6 @@ class FaviconRequestHandler : public KeyedService {
       favicon_base::FaviconImageCallback response_callback,
       FaviconRequestOrigin origin,
       const GURL& icon_url_for_uma,
-      SyncedFaviconGetter synced_favicon_getter,
       bool can_query_google_server,
       base::CancelableTaskTracker* tracker,
       const favicon_base::FaviconImageResult& image_result);
@@ -162,6 +160,8 @@ class FaviconRequestHandler : public KeyedService {
 
   LargeIconService* const large_icon_service_;
 
+  SyncedFaviconGetter const synced_favicon_getter_;
+
   // Map from a group identifier to the number of callbacks in that group which
   // would be waiting for execution. Used for recording metrics for the possible
   // benefit of grouping.

components/favicon/core/favicon_request_handler_unittest.cc

@@ -114,7 +114,8 @@ class MockLargeIconService : public LargeIconService {
 class FaviconRequestHandlerTest : public ::testing::Test {
  public:
   FaviconRequestHandlerTest()
-      : favicon_request_handler_(&mock_favicon_service_,
+      : favicon_request_handler_(synced_favicon_getter_.Get(),
+                                 &mock_favicon_service_,
                                  &mock_large_icon_service_) {}
 
  protected:
@@ -146,7 +147,7 @@ TEST_F(FaviconRequestHandlerTest, ShouldGetEmptyBitmap) {
       GURL(kDummyPageUrl), kDesiredSizeInPixel,
       base::BindOnce(&StoreBitmap, &result), FaviconRequestOrigin::UNKNOWN,
       kDummyPlatform,
-      /*icon_url_for_uma=*/GURL(), synced_favicon_getter_.Get(),
+      /*icon_url_for_uma=*/GURL(),
       /*can_send_history_data=*/true, &tracker_);
   EXPECT_FALSE(result.is_valid());
   histogram_tester_.ExpectUniqueSample("Sync.FaviconAvailability.UNKNOWN",
@@ -171,7 +172,7 @@ TEST_F(FaviconRequestHandlerTest, ShouldGetSyncBitmap) {
       GURL(kDummyPageUrl), kDesiredSizeInPixel,
       base::BindOnce(&StoreBitmap, &result), FaviconRequestOrigin::UNKNOWN,
       kDummyPlatform,
-      /*icon_url_for_uma=*/GURL(), synced_favicon_getter_.Get(),
+      /*icon_url_for_uma=*/GURL(),
       /*can_send_history_data=*/true, &tracker_);
   EXPECT_TRUE(result.is_valid());
   histogram_tester_.ExpectUniqueSample("Sync.FaviconAvailability.UNKNOWN",
@@ -197,7 +198,7 @@ TEST_F(FaviconRequestHandlerTest, ShouldGetLocalBitmap) {
       GURL(kDummyPageUrl), kDesiredSizeInPixel,
       base::BindOnce(&StoreBitmap, &result), FaviconRequestOrigin::UNKNOWN,
       kDummyPlatform,
-      /*icon_url_for_uma=*/GURL(), synced_favicon_getter_.Get(),
+      /*icon_url_for_uma=*/GURL(),
       /*can_send_history_data=*/true, &tracker_);
   EXPECT_TRUE(result.is_valid());
   histogram_tester_.ExpectUniqueSample("Sync.FaviconAvailability.UNKNOWN",
@@ -234,7 +235,7 @@ TEST_F(FaviconRequestHandlerTest, ShouldGetGoogleServerBitmapForFullUrl) {
       GURL(kDummyPageUrl), kDesiredSizeInPixel,
       base::BindOnce(&StoreBitmap, &result), FaviconRequestOrigin::HISTORY,
       kDummyPlatform,
-      /*icon_url_for_uma=*/GURL(), synced_favicon_getter_.Get(),
+      /*icon_url_for_uma=*/GURL(),
       /*can_send_history_data=*/true, &tracker_);
   EXPECT_TRUE(result.is_valid());
 }
@@ -269,7 +270,7 @@ TEST_F(FaviconRequestHandlerTest, ShouldGetGoogleServerBitmapForTrimmedUrl) {
       GURL(kDummyPageUrl), kDesiredSizeInPixel,
       base::BindOnce(&StoreBitmap, &result), FaviconRequestOrigin::HISTORY,
       kDummyPlatform,
-      /*icon_url_for_uma=*/GURL(), synced_favicon_getter_.Get(),
+      /*icon_url_for_uma=*/GURL(),
       /*can_send_history_data=*/true, &tracker_);
   EXPECT_TRUE(result.is_valid());
   histogram_tester_.ExpectUniqueSample("Sync.FaviconAvailability.HISTORY",
@@ -293,7 +294,7 @@ TEST_F(FaviconRequestHandlerTest, ShouldGetEmptyImage) {
   favicon_request_handler_.GetFaviconImageForPageURL(
       GURL(kDummyPageUrl), base::BindOnce(&StoreImage, &result),
       FaviconRequestOrigin::UNKNOWN,
-      /*icon_url_for_uma=*/GURL(), synced_favicon_getter_.Get(),
+      /*icon_url_for_uma=*/GURL(),
       /*can_send_history_data=*/true, &tracker_);
   EXPECT_TRUE(result.image.IsEmpty());
   histogram_tester_.ExpectUniqueSample("Sync.FaviconAvailability.UNKNOWN",
@@ -315,7 +316,7 @@ TEST_F(FaviconRequestHandlerTest, ShouldGetSyncImage) {
   favicon_request_handler_.GetFaviconImageForPageURL(
       GURL(kDummyPageUrl), base::BindOnce(&StoreImage, &result),
       FaviconRequestOrigin::UNKNOWN,
-      /*icon_url_for_uma=*/GURL(), synced_favicon_getter_.Get(),
+      /*icon_url_for_uma=*/GURL(),
       /*can_send_history_data=*/true, &tracker_);
   EXPECT_FALSE(result.image.IsEmpty());
   histogram_tester_.ExpectUniqueSample("Sync.FaviconAvailability.UNKNOWN",
@@ -338,7 +339,7 @@ TEST_F(FaviconRequestHandlerTest, ShouldGetLocalImage) {
   favicon_request_handler_.GetFaviconImageForPageURL(
       GURL(kDummyPageUrl), base::BindOnce(&StoreImage, &result),
       FaviconRequestOrigin::UNKNOWN,
-      /*icon_url_for_uma=*/GURL(), synced_favicon_getter_.Get(),
+      /*icon_url_for_uma=*/GURL(),
       /*can_send_history_data=*/true, &tracker_);
   EXPECT_FALSE(result.image.IsEmpty());
   histogram_tester_.ExpectUniqueSample("Sync.FaviconAvailability.UNKNOWN",
@@ -371,7 +372,7 @@ TEST_F(FaviconRequestHandlerTest, ShouldGetGoogleServerImageForFullUrl) {
   favicon_request_handler_.GetFaviconImageForPageURL(
       GURL(kDummyPageUrl), base::BindOnce(&StoreImage, &result),
       FaviconRequestOrigin::RECENTLY_CLOSED_TABS, /*icon_url_for_uma=*/GURL(),
-      synced_favicon_getter_.Get(), /*can_send_history_data=*/true, &tracker_);
+      /*can_send_history_data=*/true, &tracker_);
   EXPECT_FALSE(result.image.IsEmpty());
   histogram_tester_.ExpectUniqueSample(
       "Sync.FaviconAvailability.RECENTLY_CLOSED_TABS",
@@ -406,7 +407,7 @@ TEST_F(FaviconRequestHandlerTest, ShouldGetGoogleServerImageForTrimmedUrl) {
   favicon_request_handler_.GetFaviconImageForPageURL(
       GURL(kDummyPageUrl), base::BindOnce(&StoreImage, &result),
       FaviconRequestOrigin::RECENTLY_CLOSED_TABS, /*icon_url_for_uma=*/GURL(),
-      synced_favicon_getter_.Get(),
+
       /*can_send_history_data=*/true, &tracker_);
   EXPECT_FALSE(result.image.IsEmpty());
 }
@@ -431,7 +432,7 @@ TEST_F(FaviconRequestHandlerTest, ShouldNotQueryGoogleServerIfCannotSendData) {
       GURL(kDummyPageUrl), kDesiredSizeInPixel,
       base::BindOnce(&StoreBitmap, &result), FaviconRequestOrigin::HISTORY,
       kDummyPlatform,
-      /*icon_url_for_uma=*/GURL(), synced_favicon_getter_.Get(),
+      /*icon_url_for_uma=*/GURL(),
       /*can_send_history_data=*/false, &tracker_);
 }