[journeys] Make EntityImageService chrome-free for componentization

This CL prepares `EntityImageService` for componentization by making it
no longer directly depend on /chrome code.

Followup CL will rename it to `ImageService` and move the
implementation into /components.

Larger plan is to hook it up into `QueryClustersState` and make it
suitable for carrying URL-keyed image requests too.

Bug: b/244507194, 1367485, b/248367751
Change-Id: Iee1db7b4fb865a7952e59ae6b0f5a262b76b7646
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4045243
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Reviewed-by: Sophie Chang <sophiechang@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1076447}

chrome/browser/BUILD.gn

@@ -609,6 +609,8 @@ static_library("browser") {
     "image_fetcher/image_decoder_impl.h",
     "image_fetcher/image_fetcher_service_factory.cc",
     "image_fetcher/image_fetcher_service_factory.h",
+    "image_service/image_service_factory.cc",
+    "image_service/image_service_factory.h",
     "infobars/confirm_infobar_creator.cc",
     "infobars/confirm_infobar_creator.h",
     "infobars/infobar_responder.cc",

chrome/browser/history_clusters/entity_image_service.cc

@@ -8,10 +8,7 @@
 #include "base/functional/bind.h"
 #include "base/functional/callback.h"
 #include "base/i18n/case_conversion.h"
-#include "base/memory/singleton.h"
 #include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/profiles/profile_keyed_service_factory.h"
-#include "chrome/browser/sync/sync_service_factory.h"
 #include "components/history_clusters/core/config.h"
 #include "components/omnibox/browser/remote_suggestions_service.h"
 #include "components/omnibox/browser/search_suggestion_parser.h"
@@ -24,40 +21,6 @@ namespace history_clusters {
 
 namespace {
 
-// Anonymous namespace factory based on LookalikeUrlServiceFactory.
-class EntityImageServiceFactory : public ProfileKeyedServiceFactory {
- public:
-  static EntityImageService* GetForProfile(Profile* profile) {
-    return static_cast<EntityImageService*>(
-        GetInstance()->GetServiceForBrowserContext(profile,
-                                                   /*create=*/true));
-  }
-  static EntityImageServiceFactory* GetInstance() {
-    return base::Singleton<EntityImageServiceFactory>::get();
-  }
-
-  EntityImageServiceFactory(const EntityImageServiceFactory&) = delete;
-  EntityImageServiceFactory& operator=(const EntityImageServiceFactory&) =
-      delete;
-
- private:
-  friend struct base::DefaultSingletonTraits<EntityImageServiceFactory>;
-
-  // EntityImageServiceFactory:
-  EntityImageServiceFactory()
-      : ProfileKeyedServiceFactory("EntityImageServiceFactory") {
-    DependsOn(SyncServiceFactory::GetInstance());
-  }
-
-  ~EntityImageServiceFactory() override = default;
-
-  // BrowserContextKeyedServiceFactory:
-  KeyedService* BuildServiceInstanceFor(
-      content::BrowserContext* profile) const override {
-    return new EntityImageService(static_cast<Profile*>(profile));
-  }
-};
-
 // A one-time use object that encapsulates tagging a vector of clusters with
 // entity images. Used to manage all the fetch jobs dispatched, and runs the
 // main callback after it's done.
@@ -145,15 +108,12 @@ void DeleteManagerAndRunCallback(
 class EntityImageService::SuggestEntityImageURLFetcher {
  public:
   SuggestEntityImageURLFetcher(
-      Profile* profile,
-      ChromeAutocompleteProviderClient* autocomplete_provider_client,
+      AutocompleteProviderClient* autocomplete_provider_client,
       const std::u16string& search_query,
       const std::string& entity_id)
-      : profile_(profile),
-        autocomplete_provider_client_(autocomplete_provider_client),
+      : autocomplete_provider_client_(autocomplete_provider_client),
         search_query_(base::i18n::ToLower(search_query)),
         entity_id_(entity_id) {
-    DCHECK(profile);
     DCHECK(autocomplete_provider_client);
   }
   SuggestEntityImageURLFetcher(const SuggestEntityImageURLFetcher&) = delete;
@@ -228,7 +188,6 @@ class EntityImageService::SuggestEntityImageURLFetcher {
       std::move(callback_).Run(GURL());
   }
 
-  const raw_ptr<Profile> profile_;
   const raw_ptr<AutocompleteProviderClient> autocomplete_provider_client_;
 
   // The search query and entity ID we are searching for.
@@ -244,21 +203,16 @@ class EntityImageService::SuggestEntityImageURLFetcher {
   base::WeakPtrFactory<SuggestEntityImageURLFetcher> weak_factory_{this};
 };
 
-EntityImageService::EntityImageService(Profile* profile)
-    : profile_(profile),
-      autocomplete_provider_client_(profile),
-      url_consent_helper_(unified_consent::UrlKeyedDataCollectionConsentHelper::
-                              NewPersonalizedDataCollectionConsentHelper(
-                                  SyncServiceFactory::GetForProfile(profile))) {
-}
+EntityImageService::EntityImageService(
+    std::unique_ptr<AutocompleteProviderClient> autocomplete_provider_client,
+    syncer::SyncService* sync_service)
+    : autocomplete_provider_client_(std::move(autocomplete_provider_client)),
+      url_consent_helper_(
+          unified_consent::UrlKeyedDataCollectionConsentHelper::
+              NewPersonalizedDataCollectionConsentHelper(sync_service)) {}
 
 EntityImageService::~EntityImageService() = default;
 
-// static
-EntityImageService* EntityImageService::Get(Profile* profile) {
-  return EntityImageServiceFactory::GetForProfile(profile);
-}
-
 void EntityImageService::PopulateEntityImagesFor(
     std::vector<history::Cluster> clusters,
     base::OnceCallback<void(std::vector<history::Cluster>)> callback) {
@@ -283,7 +237,7 @@ bool EntityImageService::FetchImageFor(const std::u16string& search_query,
   DCHECK(url_consent_helper_ && url_consent_helper_->IsEnabled());
 
   auto fetcher = std::make_unique<SuggestEntityImageURLFetcher>(
-      profile_, &autocomplete_provider_client_, search_query, entity_id);
+      autocomplete_provider_client_.get(), search_query, entity_id);
 
   // Use a raw pointer temporary so we can give ownership of the unique_ptr to
   // the callback and have a well defined SuggestEntityImageURLFetcher lifetime.

chrome/browser/history_clusters/entity_image_service.h

@@ -5,36 +5,35 @@
 #ifndef CHROME_BROWSER_HISTORY_CLUSTERS_ENTITY_IMAGE_SERVICE_H_
 #define CHROME_BROWSER_HISTORY_CLUSTERS_ENTITY_IMAGE_SERVICE_H_
 
+#include <memory>
 #include <string>
 
 #include "base/functional/callback_forward.h"
 #include "base/memory/raw_ptr.h"
 #include "base/memory/weak_ptr.h"
-#include "chrome/browser/autocomplete/chrome_autocomplete_provider_client.h"
-#include "chrome/browser/profiles/profile.h"
 #include "components/history/core/browser/history_types.h"
 #include "components/keyed_service/core/keyed_service.h"
+#include "components/omnibox/browser/autocomplete_provider_client.h"
+#include "components/sync/driver/sync_service.h"
 #include "components/unified_consent/url_keyed_data_collection_consent_helper.h"
 
 namespace history_clusters {
 
 // Used to get the image URL associated with a cluster. It doesn't actually
 // fetch the image, that's up to the UI to do.
+// TODO(tommycli): Move to /components and rename to `ImageService`.
 class EntityImageService : public KeyedService {
  public:
   using ResultCallback = base::OnceCallback<void(const GURL& image_url)>;
 
-  // This should only be called by the internal factory. Most callers should use
-  // the `Get()` method instead.
-  explicit EntityImageService(Profile* profile);
+  EntityImageService(
+      std::unique_ptr<AutocompleteProviderClient> autocomplete_provider_client,
+      syncer::SyncService* sync_service);
   EntityImageService(const EntityImageService&) = delete;
   EntityImageService& operator=(const EntityImageService&) = delete;
 
   ~EntityImageService() override;
 
-  // Gets the fetcher associated with `profile`. Always succeeds.
-  static EntityImageService* Get(Profile* profile);
-
   // Populates entity images into the `image_url` of any eligible visits within
   // every cluster in `clusters`. `clusters` should be moved into the parameter.
   // `callback` is called when we're done, and it can be called synchronously
@@ -58,9 +57,7 @@ class EntityImageService : public KeyedService {
                       ResultCallback callback,
                       const GURL& image_url);
 
-  const raw_ptr<Profile> profile_;
-  ChromeAutocompleteProviderClient autocomplete_provider_client_;
-
+  std::unique_ptr<AutocompleteProviderClient> autocomplete_provider_client_;
   std::unique_ptr<unified_consent::UrlKeyedDataCollectionConsentHelper>
       url_consent_helper_;
 

chrome/browser/image_service/OWNERS

@@ -0,0 +1,2 @@
+sophiechang@chromium.org
+tommycli@chromium.org

chrome/browser/image_service/README.md

@@ -0,0 +1,4 @@
+# //chrome/browser/image_service
+
+Service implementation for Chrome Images feature, to provide relevant images
+for Chrome UI usage.

chrome/browser/image_service/image_service_factory.cc

@@ -0,0 +1,44 @@
+// Copyright 2022 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/image_service/image_service_factory.h"
+
+#include "base/no_destructor.h"
+#include "chrome/browser/autocomplete/chrome_autocomplete_provider_client.h"
+#include "chrome/browser/history_clusters/entity_image_service.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/sync/sync_service_factory.h"
+
+namespace image_service {
+
+// static
+history_clusters::EntityImageService* ImageServiceFactory::GetForBrowserContext(
+    content::BrowserContext* browser_context) {
+  return static_cast<history_clusters::EntityImageService*>(
+      GetInstance().GetServiceForBrowserContext(browser_context, true));
+}
+
+// static
+ImageServiceFactory& ImageServiceFactory::GetInstance() {
+  static base::NoDestructor<ImageServiceFactory> instance;
+  return *instance;
+}
+
+ImageServiceFactory::ImageServiceFactory()
+    : ProfileKeyedServiceFactory("ImageService") {
+  DependsOn(SyncServiceFactory::GetInstance());
+}
+
+ImageServiceFactory::~ImageServiceFactory() = default;
+
+KeyedService* ImageServiceFactory::BuildServiceInstanceFor(
+    content::BrowserContext* context) const {
+  auto* profile = Profile::FromBrowserContext(context);
+  // TODO(tommycli): Move EntityImageService to the image_service component.
+  return new history_clusters::EntityImageService(
+      std::make_unique<ChromeAutocompleteProviderClient>(profile),
+      SyncServiceFactory::GetForProfile(profile));
+}
+
+}  // namespace image_service

chrome/browser/image_service/image_service_factory.h

@@ -0,0 +1,43 @@
+// Copyright 2022 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_IMAGE_SERVICE_IMAGE_SERVICE_FACTORY_H_
+#define CHROME_BROWSER_IMAGE_SERVICE_IMAGE_SERVICE_FACTORY_H_
+
+#include "base/no_destructor.h"
+#include "chrome/browser/profiles/profile_keyed_service_factory.h"
+
+namespace content {
+class BrowserContext;
+}
+
+namespace history_clusters {
+class EntityImageService;
+}
+
+namespace image_service {
+
+// Factory for BrowserContext keyed ImageService, which provides images for
+// Journeys related features.
+class ImageServiceFactory : public ProfileKeyedServiceFactory {
+ public:
+  // This can return nullptr in tests.
+  static history_clusters::EntityImageService* GetForBrowserContext(
+      content::BrowserContext* browser_context);
+
+ private:
+  friend base::NoDestructor<ImageServiceFactory>;
+  static ImageServiceFactory& GetInstance();
+
+  ImageServiceFactory();
+  ~ImageServiceFactory() override;
+
+  // BrowserContextKeyedServiceFactory:
+  KeyedService* BuildServiceInstanceFor(
+      content::BrowserContext* context) const override;
+};
+
+}  // namespace image_service
+
+#endif  // CHROME_BROWSER_IMAGE_SERVICE_IMAGE_SERVICE_FACTORY_H_

chrome/browser/ui/webui/history_clusters/history_clusters_handler.cc

@@ -22,6 +22,7 @@
 #include "chrome/browser/history_clusters/entity_image_service.h"
 #include "chrome/browser/history_clusters/history_clusters_metrics_logger.h"
 #include "chrome/browser/history_clusters/history_clusters_service_factory.h"
+#include "chrome/browser/image_service/image_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/sync/sync_service_factory.h"
@@ -513,15 +514,21 @@ void HistoryClustersHandler::OnGotClustersBatch(
     bool can_load_more,
     bool is_continuation) {
   // TODO(tommycli): It's weird that there's one more post-processing step here
-  // that's not encapsulated within `QueryClustersState`. That's because
-  // `EntityImageService` can't live in the component yet, because it depends
-  // on code in the /chrome directory. Fix this using dependency injection.
-  auto* entity_image_service = EntityImageService::Get(GetProfile());
-  entity_image_service->PopulateEntityImagesFor(
-      std::move(clusters_batch),
-      base::BindOnce(&HistoryClustersHandler::SendClustersToPage,
-                     weak_ptr_factory_.GetWeakPtr(), query, can_load_more,
-                     is_continuation));
+  // that's not encapsulated within `QueryClustersState`. After componentizing
+  // ImageService, have HistoryClustersService pass a pointer to it in the
+  // constructor, so `QueryClustersState` can do this for itself.
+  if (auto* image_service =
+          image_service::ImageServiceFactory::GetForBrowserContext(
+              GetProfile())) {
+    image_service->PopulateEntityImagesFor(
+        std::move(clusters_batch),
+        base::BindOnce(&HistoryClustersHandler::SendClustersToPage,
+                       weak_ptr_factory_.GetWeakPtr(), query, can_load_more,
+                       is_continuation));
+  } else {
+    SendClustersToPage(query, can_load_more, is_continuation,
+                       std::move(clusters_batch));
+  }
 }
 
 void HistoryClustersHandler::SendClustersToPage(