Move image annotation client into keyed service

The service (in //services/image_annotation) has been
hanging off of ProfileImpl since it was moved off of Service Manager
APIs. That breaks image annotation for OTR profiles.

This CL migrates the client code to the AccessibilityLabelsService that
hangs off of all Profiles.

This also opportunistically moves Data Decoder consumption off of
Service Manager APIs in favor of the new DataDecoder client API.

Fixed: 1020657
Change-Id: I62f77f2f1bcfc84225efb8b62a75d74f2d0c655c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1895963Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Nektarios Paisios <nektar@chromium.org>
Reviewed-by: Andrew Moylan <amoylan@chromium.org>
Reviewed-by: Stefan Kuhne <skuhne@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#712657}

chrome/browser/accessibility/DEPS

+include_rules = [
+  "+services/image_annotation",
+]

chrome/browser/accessibility/accessibility_labels_service.cc

@@ -5,16 +5,22 @@
 #include "chrome/browser/accessibility/accessibility_labels_service.h"
 
 #include "base/metrics/histogram_functions.h"
+#include "base/no_destructor.h"
 #include "build/build_config.h"
 #include "chrome/browser/accessibility/accessibility_state_utils.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/tab_contents/tab_contents_iterator.h"
+#include "chrome/common/channel_info.h"
 #include "chrome/common/pref_names.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
 #include "components/sync_preferences/pref_service_syncable.h"
+#include "components/version_info/channel.h"
 #include "content/public/browser/browser_accessibility_state.h"
 #include "content/public/common/content_features.h"
+#include "google_apis/google_api_keys.h"
+#include "services/data_decoder/public/cpp/data_decoder.h"
+#include "services/image_annotation/image_annotation_service.h"
 #include "ui/accessibility/ax_action_data.h"
 #include "ui/accessibility/ax_enums.mojom.h"
 
@@ -24,6 +30,41 @@
 #include "chrome/browser/ui/browser_list.h"
 #endif
 
+namespace {
+
+// Returns the Chrome Google API key for the channel of this build.
+std::string APIKeyForChannel() {
+  if (chrome::GetChannel() == version_info::Channel::STABLE)
+    return google_apis::GetAPIKey();
+  return google_apis::GetNonStableAPIKey();
+}
+
+AccessibilityLabelsService::ImageAnnotatorBinder&
+GetImageAnnotatorBinderOverride() {
+  static base::NoDestructor<AccessibilityLabelsService::ImageAnnotatorBinder>
+      binder;
+  return *binder;
+}
+
+class ImageAnnotatorClient : public image_annotation::Annotator::Client {
+ public:
+  ImageAnnotatorClient() = default;
+  ~ImageAnnotatorClient() override = default;
+
+  // image_annotation::Annotator::Client implementation:
+  void BindJsonParser(mojo::PendingReceiver<data_decoder::mojom::JsonParser>
+                          receiver) override {
+    data_decoder_.GetService()->BindJsonParser(std::move(receiver));
+  }
+
+ private:
+  data_decoder::DataDecoder data_decoder_;
+
+  DISALLOW_COPY_AND_ASSIGN(ImageAnnotatorClient);
+};
+
+}  // namespace
+
 AccessibilityLabelsService::~AccessibilityLabelsService() {}
 
 // static
@@ -109,6 +150,29 @@ void AccessibilityLabelsService::EnableLabelsServiceOnce() {
 #endif
 }
 
+void AccessibilityLabelsService::BindImageAnnotator(
+    mojo::PendingReceiver<image_annotation::mojom::Annotator> receiver) {
+  if (!remote_service_) {
+    auto service_receiver = remote_service_.BindNewPipeAndPassReceiver();
+    auto& binder = GetImageAnnotatorBinderOverride();
+    if (binder) {
+      binder.Run(std::move(service_receiver));
+    } else {
+      service_ = std::make_unique<image_annotation::ImageAnnotationService>(
+          std::move(service_receiver), APIKeyForChannel(),
+          profile_->GetURLLoaderFactory(),
+          std::make_unique<ImageAnnotatorClient>());
+    }
+  }
+
+  remote_service_->BindAnnotator(std::move(receiver));
+}
+
+void AccessibilityLabelsService::OverrideImageAnnotatorBinderForTesting(
+    ImageAnnotatorBinder binder) {
+  GetImageAnnotatorBinderOverride() = std::move(binder);
+}
+
 void AccessibilityLabelsService::OnImageLabelsEnabledChanged() {
   // TODO(dmazzoni) Implement for Android, which doesn't support
   // AllTabContentses(). crbug.com/905419

chrome/browser/accessibility/accessibility_labels_service.h

@@ -8,10 +8,17 @@
 #include "base/memory/weak_ptr.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/prefs/pref_change_registrar.h"
+#include "mojo/public/cpp/bindings/pending_receiver.h"
+#include "mojo/public/cpp/bindings/remote.h"
+#include "services/image_annotation/public/mojom/image_annotation.mojom.h"
 #include "ui/accessibility/ax_mode.h"
 
 class Profile;
 
+namespace image_annotation {
+class ImageAnnotationService;
+}
+
 namespace user_prefs {
 class PrefRegistrySyncable;
 }
@@ -35,6 +42,17 @@ class AccessibilityLabelsService : public KeyedService {
 
   void EnableLabelsServiceOnce();
 
+  // Routes an Annotator interface receiver to the Image Annotation service for
+  // binding.
+  void BindImageAnnotator(
+      mojo::PendingReceiver<image_annotation::mojom::Annotator> receiver);
+
+  // Allows tests to override how this object binds a connection to a remote
+  // ImageAnnotationService.
+  using ImageAnnotatorBinder = base::RepeatingCallback<void(
+      mojo::PendingReceiver<image_annotation::mojom::ImageAnnotationService>)>;
+  void OverrideImageAnnotatorBinderForTesting(ImageAnnotatorBinder binder);
+
  private:
   friend class AccessibilityLabelsServiceFactory;
 
@@ -51,6 +69,10 @@ class AccessibilityLabelsService : public KeyedService {
 
   PrefChangeRegistrar pref_change_registrar_;
 
+  // Implementation of and remote connection to the Image Annotation service.
+  std::unique_ptr<image_annotation::ImageAnnotationService> service_;
+  mojo::Remote<image_annotation::mojom::ImageAnnotationService> remote_service_;
+
   base::WeakPtrFactory<AccessibilityLabelsService> weak_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(AccessibilityLabelsService);

chrome/browser/accessibility/image_annotation_browsertest.cc

@@ -8,8 +8,9 @@
 #include "base/strings/string_util.h"
 #include "base/test/scoped_feature_list.h"
 #include "build/build_config.h"
+#include "chrome/browser/accessibility/accessibility_labels_service.h"
+#include "chrome/browser/accessibility/accessibility_labels_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/profiles/profile_impl.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/test/base/in_process_browser_test.h"
@@ -207,8 +208,9 @@ class ImageAnnotationBrowserTest : public InProcessBrowserTest {
     content::WebContents* web_contents =
         browser()->tab_strip_model()->GetActiveWebContents();
 
-    ProfileImpl::OverrideImageAnnotationServiceBinderForTesting(
-        base::BindRepeating(&BindImageAnnotatorService));
+    AccessibilityLabelsServiceFactory::GetForProfile(browser()->profile())
+        ->OverrideImageAnnotatorBinderForTesting(
+            base::BindRepeating(&BindImageAnnotatorService));
 
     ui::AXMode mode = ui::kAXModeComplete;
     mode.set_mode(ui::AXMode::kLabelImages, true);
@@ -218,8 +220,8 @@ class ImageAnnotationBrowserTest : public InProcessBrowserTest {
   }
 
   void TearDownOnMainThread() override {
-    ProfileImpl::OverrideImageAnnotationServiceBinderForTesting(
-        base::NullCallback());
+    AccessibilityLabelsServiceFactory::GetForProfile(browser()->profile())
+        ->OverrideImageAnnotatorBinderForTesting(base::NullCallback());
     InProcessBrowserTest::TearDownOnMainThread();
   }
 

chrome/browser/chrome_browser_interface_binders.cc

@@ -6,6 +6,8 @@
 
 #include "base/feature_list.h"
 #include "build/build_config.h"
+#include "chrome/browser/accessibility/accessibility_labels_service.h"
+#include "chrome/browser/accessibility/accessibility_labels_service_factory.h"
 #include "chrome/browser/content_settings/content_settings_manager_impl.h"
 #include "chrome/browser/navigation_predictor/navigation_predictor.h"
 #include "chrome/browser/profiles/profile.h"
@@ -36,13 +38,14 @@
 namespace chrome {
 namespace internal {
 
-// Forward image Annotator requests to the profile's ImageAnnotationService.
+// Forward image Annotator requests to the profile's AccessibilityLabelsService.
 void BindImageAnnotator(
     content::RenderFrameHost* const frame_host,
     mojo::PendingReceiver<image_annotation::mojom::Annotator> receiver) {
-  Profile::FromBrowserContext(frame_host->GetProcess()->GetBrowserContext())
-      ->GetImageAnnotationService()
-      ->BindAnnotator(std::move(receiver));
+  AccessibilityLabelsServiceFactory::GetForProfile(
+      Profile::FromBrowserContext(
+          frame_host->GetProcess()->GetBrowserContext()))
+      ->BindImageAnnotator(std::move(receiver));
 }
 
 #if defined(OS_ANDROID)

chrome/browser/profiles/DEPS

@@ -3,8 +3,5 @@ specific_include_rules = {
     # Profile embeds the Identity Service, but the dependence
     # should be used *only* for Identity Service creation.
     "+services/identity/identity_service.h",
-
-    # Profile embeds the image_annotation service.
-    "+services/image_annotation/image_annotation_service.h",
   ],
 }

chrome/browser/profiles/profile.cc

@@ -293,11 +293,6 @@ identity::mojom::IdentityService* Profile::GetIdentityService() {
   return nullptr;
 }
 
-image_annotation::mojom::ImageAnnotationService*
-Profile::GetImageAnnotationService() {
-  return nullptr;
-}
-
 bool Profile::IsNewProfile() {
 #if !defined(OS_ANDROID)
   // The profile is new if the preference files has just been created, except on

chrome/browser/profiles/profile.h

@@ -45,12 +45,6 @@ class IdentityService;
 }  // namespace mojom
 }  // namespace identity
 
-namespace image_annotation {
-namespace mojom {
-class ImageAnnotationService;
-}
-}  // namespace image_annotation
-
 namespace policy {
 class SchemaRegistryService;
 class ProfilePolicyConnector;
@@ -394,11 +388,6 @@ class Profile : public content::BrowserContext {
   // null if the profile does not have a corresponding service instance.
   virtual identity::mojom::IdentityService* GetIdentityService();
 
-  // Exposes access to the profile's Image Annotation service instance. This may
-  // return null if the profile does not have a corresponding service instance.
-  virtual image_annotation::mojom::ImageAnnotationService*
-  GetImageAnnotationService();
-
   // Stop sending accessibility events until ResumeAccessibilityEvents().
   // Calls to Pause nest; no events will be sent until the number of
   // Resume calls matches the number of Pause calls received.

chrome/browser/profiles/profile_impl.cc

@@ -155,14 +155,12 @@
 #include "content/public/browser/url_data_source.h"
 #include "content/public/common/content_constants.h"
 #include "extensions/buildflags/buildflags.h"
-#include "google_apis/google_api_keys.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/self_owned_receiver.h"
 #include "ppapi/buildflags/buildflags.h"
 #include "printing/buildflags/buildflags.h"
 #include "services/data_decoder/public/mojom/constants.mojom.h"
 #include "services/identity/identity_service.h"
-#include "services/image_annotation/image_annotation_service.h"
 #include "services/network/public/cpp/features.h"
 #include "services/preferences/public/cpp/in_process_service_factory.h"
 #include "services/preferences/public/mojom/preferences.mojom.h"
@@ -257,13 +255,6 @@ const int kCreateSessionServiceDelayMS = 500;
 const char kPrefExitTypeCrashed[] = "Crashed";
 const char kPrefExitTypeSessionEnded[] = "SessionEnded";
 
-// Returns the Chrome Google API key for the channel of this build.
-std::string APIKeyForChannel() {
-  if (chrome::GetChannel() == version_info::Channel::STABLE)
-    return google_apis::GetAPIKey();
-  return google_apis::GetNonStableAPIKey();
-}
-
 // Gets the creation time for |path|, returning base::Time::Now() on failure.
 base::Time GetCreationTimeForPath(const base::FilePath& path) {
   base::File::Info info;
@@ -339,28 +330,6 @@ bool LocaleNotChanged(const std::string& pref_locale,
 }
 #endif  // defined(OS_CHROMEOS)
 
-class ImageAnnotatorClient : public image_annotation::Annotator::Client {
- public:
-  ImageAnnotatorClient() = default;
-  ~ImageAnnotatorClient() override = default;
-
-  // image_annotation::Annotator::Client implementation:
-  void BindJsonParser(mojo::PendingReceiver<data_decoder::mojom::JsonParser>
-                          receiver) override {
-    content::GetSystemConnector()->Connect(data_decoder::mojom::kServiceName,
-                                           std::move(receiver));
-  }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(ImageAnnotatorClient);
-};
-
-ProfileImpl::ImageAnnotationServiceBinder&
-GetImageAnnotationServiceBinderOverride() {
-  static base::NoDestructor<ProfileImpl::ImageAnnotationServiceBinder> binder;
-  return *binder;
-}
-
 }  // namespace
 
 // static
@@ -1099,24 +1068,6 @@ identity::mojom::IdentityService* ProfileImpl::GetIdentityService() {
   return remote_identity_service_.get();
 }
 
-image_annotation::mojom::ImageAnnotationService*
-ProfileImpl::GetImageAnnotationService() {
-  if (!remote_image_annotation_service_) {
-    auto receiver =
-        remote_image_annotation_service_.BindNewPipeAndPassReceiver();
-    const auto& binder = GetImageAnnotationServiceBinderOverride();
-    if (binder) {
-      binder.Run(std::move(receiver));
-    } else {
-      image_annotation_service_impl_ =
-          std::make_unique<image_annotation::ImageAnnotationService>(
-              std::move(receiver), APIKeyForChannel(), GetURLLoaderFactory(),
-              std::make_unique<ImageAnnotatorClient>());
-    }
-  }
-  return remote_image_annotation_service_.get();
-}
-
 PrefService* ProfileImpl::GetPrefs() {
   return const_cast<PrefService*>(
       static_cast<const ProfileImpl*>(this)->GetPrefs());
@@ -1511,12 +1462,6 @@ void ProfileImpl::SetCreationTimeForTesting(base::Time creation_time) {
   creation_time_ = creation_time;
 }
 
-// static
-void ProfileImpl::OverrideImageAnnotationServiceBinderForTesting(
-    ImageAnnotationServiceBinder binder) {
-  GetImageAnnotationServiceBinderOverride() = std::move(binder);
-}
-
 GURL ProfileImpl::GetHomePage() {
   // --homepage overrides any preferences.
   const base::CommandLine& command_line =

chrome/browser/profiles/profile_impl.h

@@ -26,7 +26,6 @@
 #include "extensions/buildflags/buildflags.h"
 #include "mojo/public/cpp/bindings/remote.h"
 #include "services/identity/public/mojom/identity_service.mojom.h"
-#include "services/image_annotation/public/mojom/image_annotation.mojom.h"
 
 #if !defined(OS_ANDROID)
 #include "chrome/browser/ui/zoom/chrome_zoom_level_prefs.h"
@@ -53,10 +52,6 @@ namespace identity {
 class IdentityService;
 }
 
-namespace image_annotation {
-class ImageAnnotationService;
-}
-
 namespace policy {
 class ConfigurationPolicyProvider;
 class ProfilePolicyConnector;
@@ -171,8 +166,6 @@ class ProfileImpl : public Profile {
   bool ShouldRestoreOldSessionCookies() override;
   bool ShouldPersistSessionCookies() override;
   identity::mojom::IdentityService* GetIdentityService() override;
-  image_annotation::mojom::ImageAnnotationService* GetImageAnnotationService()
-      override;
 
 #if defined(OS_CHROMEOS)
   void ChangeAppLocale(const std::string& locale, AppLocaleChangedVia) override;
@@ -182,13 +175,6 @@ class ProfileImpl : public Profile {
 
   void SetCreationTimeForTesting(base::Time creation_time) override;
 
-  // Overrides how the ImageAnnotationService service instance is bound in a
-  // testing environment. If |binder| is null, any existing override is removed.
-  using ImageAnnotationServiceBinder = base::RepeatingCallback<void(
-      mojo::PendingReceiver<image_annotation::mojom::ImageAnnotationService>)>;
-  static void OverrideImageAnnotationServiceBinderForTesting(
-      ImageAnnotationServiceBinder binder);
-
  private:
 #if defined(OS_CHROMEOS)
   friend class chromeos::KioskTest;
@@ -261,12 +247,6 @@ class ProfileImpl : public Profile {
   // |GetIdentityService()|.
   mojo::Remote<identity::mojom::IdentityService> remote_identity_service_;
 
-  // The Image Annotation service instance for this profile.
-  std::unique_ptr<image_annotation::ImageAnnotationService>
-      image_annotation_service_impl_;
-  mojo::Remote<image_annotation::mojom::ImageAnnotationService>
-      remote_image_annotation_service_;
-
   // !!! BIG HONKING WARNING !!!
   //  The order of the members below is important. Do not change it unless
   //  you know what you're doing. Also, if adding a new member here make sure