[Sync] Wire ConsentSyncBridge behind a feature (not on iOS).

Connect ConsentSyncBridge to ConsentAuditor behind a feature
on all platforms except iOS. ConsentAuditor does not use the
bridge at all yet.

This CL is a part of a series to split user consents from user events
into a separate datatype.

Bug: 840357
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Id23914d4afcf105f21dab856208d98979aa82209
Reviewed-on: https://chromium-review.googlesource.com/1084475
Commit-Queue: vitaliii <vitaliii@chromium.org>
Reviewed-by: Benoit Zanotti <bzanotti@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Markus Heintz <markusheintz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565353}

chrome/browser/consent_auditor/consent_auditor_factory.cc

@@ -4,13 +4,22 @@
 
 #include "chrome/browser/consent_auditor/consent_auditor_factory.h"
 
+#include "base/bind_helpers.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/browser/sync/user_event_service_factory.h"
+#include "chrome/common/channel_info.h"
+#include "components/browser_sync/profile_sync_service.h"
 #include "components/consent_auditor/consent_auditor.h"
+#include "components/consent_auditor/consent_sync_bridge.h"
+#include "components/consent_auditor/consent_sync_bridge_impl.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
+#include "components/sync/base/report_unrecoverable_error.h"
+#include "components/sync/driver/sync_driver_switches.h"
+#include "components/sync/model_impl/client_tag_based_model_type_processor.h"
 #include "components/version_info/version_info.h"
 
 // static
@@ -35,6 +44,10 @@ ConsentAuditorFactory::ConsentAuditorFactory()
           "ConsentAuditor",
           BrowserContextDependencyManager::GetInstance()) {
   DependsOn(browser_sync::UserEventServiceFactory::GetInstance());
+  // TODO(crbug.com/850428): This is missing
+  // DependsOn(ProfileSyncServiceFactory::GetInstance()), which we can't
+  // simply add because ProfileSyncServiceFactory itself depends on this
+  // factory.
 }
 
 ConsentAuditorFactory::~ConsentAuditorFactory() {}
@@ -42,8 +55,31 @@ ConsentAuditorFactory::~ConsentAuditorFactory() {}
 KeyedService* ConsentAuditorFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
   Profile* profile = static_cast<Profile*>(context);
+
+  std::unique_ptr<syncer::ConsentSyncBridge> bridge;
+  if (base::FeatureList::IsEnabled(switches::kSyncUserConsentSeparateType)) {
+    syncer::OnceModelTypeStoreFactory store_factory =
+        browser_sync::ProfileSyncService::GetModelTypeStoreFactory(
+            profile->GetPath());
+    auto change_processor =
+        std::make_unique<syncer::ClientTagBasedModelTypeProcessor>(
+            syncer::USER_CONSENTS,
+            base::BindRepeating(&syncer::ReportUnrecoverableError,
+                                chrome::GetChannel()));
+    syncer::SyncService* sync_service =
+        ProfileSyncServiceFactory::GetForProfile(profile);
+    bridge = std::make_unique<syncer::ConsentSyncBridgeImpl>(
+        std::move(store_factory), std::move(change_processor),
+        /*authenticated_account_id_callback=*/
+        base::BindRepeating(
+            [](syncer::SyncService* sync_service) {
+              return sync_service->GetAuthenticatedAccountInfo().account_id;
+            },
+            base::Unretained(sync_service)));
+  }
+  // TODO(vitaliii): Don't create UserEventService when it won't be used.
   return new consent_auditor::ConsentAuditor(
-      profile->GetPrefs(),
+      profile->GetPrefs(), std::move(bridge),
       browser_sync::UserEventServiceFactory::GetForProfile(profile),
       // The browser version and locale do not change runtime, so we can pass
       // them directly.

chrome/browser/sync/chrome_sync_client.cc

@@ -15,6 +15,7 @@
 #include "chrome/browser/autofill/personal_data_manager_factory.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/consent_auditor/consent_auditor_factory.h"
 #include "chrome/browser/dom_distiller/dom_distiller_service_factory.h"
 #include "chrome/browser/favicon/favicon_service_factory.h"
 #include "chrome/browser/history/history_service_factory.h"
@@ -49,6 +50,7 @@
 #include "components/browser_sync/browser_sync_switches.h"
 #include "components/browser_sync/profile_sync_components_factory_impl.h"
 #include "components/browser_sync/profile_sync_service.h"
+#include "components/consent_auditor/consent_auditor.h"
 #include "components/dom_distiller/core/dom_distiller_service.h"
 #include "components/history/core/browser/history_model_worker.h"
 #include "components/history/core/browser/history_service.h"
@@ -500,9 +502,8 @@ ChromeSyncClient::GetControllerDelegateForModelType(syncer::ModelType type) {
           ->GetControllerDelegateOnUIThread();
     }
     case syncer::USER_CONSENTS:
-      // TODO(vitaliii): Return the real delegate once it is wired to the
-      // consent auditor.
-      return base::WeakPtr<syncer::ModelTypeControllerDelegate>();
+      return ConsentAuditorFactory::GetForProfile(profile_)
+          ->GetControllerDelegateOnUIThread();
     case syncer::USER_EVENTS:
       return browser_sync::UserEventServiceFactory::GetForProfile(profile_)
           ->GetSyncBridge()

chrome/browser/sync/profile_sync_service_factory.cc

@@ -13,6 +13,7 @@
 #include "chrome/browser/autofill/personal_data_manager_factory.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/consent_auditor/consent_auditor_factory.h"
 #include "chrome/browser/defaults.h"
 #include "chrome/browser/dom_distiller/dom_distiller_service_factory.h"
 #include "chrome/browser/gcm/gcm_profile_service_factory.h"
@@ -129,6 +130,7 @@ ProfileSyncServiceFactory::ProfileSyncServiceFactory()
   DependsOn(BookmarkUndoServiceFactory::GetInstance());
   DependsOn(browser_sync::UserEventServiceFactory::GetInstance());
   DependsOn(ChromeSigninClientFactory::GetInstance());
+  DependsOn(ConsentAuditorFactory::GetInstance());
   DependsOn(dom_distiller::DomDistillerServiceFactory::GetInstance());
   DependsOn(GaiaCookieManagerServiceFactory::GetInstance());
   DependsOn(gcm::GCMProfileServiceFactory::GetInstance());

chrome/browser/sync/profile_sync_service_factory_unittest.cc

@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/command_line.h"
+#include "base/feature_list.h"
 #include "base/task_scheduler/task_scheduler.h"
 #include "build/build_config.h"
 #include "chrome/common/buildflags.h"
@@ -17,6 +18,7 @@
 #include "components/browser_sync/profile_sync_service.h"
 #include "components/sync/base/model_type.h"
 #include "components/sync/driver/data_type_controller.h"
+#include "components/sync/driver/sync_driver_switches.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -90,7 +92,9 @@ class ProfileSyncServiceFactoryTest : public testing::Test {
     datatypes.push_back(syncer::SUPERVISED_USER_WHITELISTS);
     datatypes.push_back(syncer::TYPED_URLS);
     datatypes.push_back(syncer::USER_EVENTS);
-    // TODO(vitaliii): Add USER_CONSENTS, when the type is enabled by default.
+    if (base::FeatureList::IsEnabled(switches::kSyncUserConsentSeparateType)) {
+      datatypes.push_back(syncer::USER_CONSENTS);
+    }
 
     return datatypes;
   }

chrome/browser/sync/user_event_service_factory.cc

@@ -41,7 +41,8 @@ UserEventServiceFactory::UserEventServiceFactory()
   // TODO(vitaliii): This is missing
   // DependsOn(ProfileSyncServiceFactory::GetInstance()), which we can't
   // simply add because ProfileSyncServiceFactory itself depends on this
-  // factory.
+  // factory. This won't be relevant anymore once the separate consents datatype
+  // is fully launched.
 }
 
 UserEventServiceFactory::~UserEventServiceFactory() {}

components/browser_sync/profile_sync_components_factory_impl.cc

@@ -254,10 +254,7 @@ ProfileSyncComponentsFactoryImpl::CreateCommonDataTypeControllers(
         syncer::USER_EVENTS, sync_client_, ui_thread_));
   }
 
-  // TODO(vitaliii): Enable consents once their controller delegate is wired
-  // properly.
-  if (false) {
-    // Consents should always be enabled.
+  if (base::FeatureList::IsEnabled(switches::kSyncUserConsentSeparateType)) {
     controllers.push_back(std::make_unique<ModelTypeController>(
         syncer::USER_CONSENTS, sync_client_, ui_thread_));
   }

components/consent_auditor/consent_auditor.cc

@@ -58,15 +58,20 @@ UserConsentTypes::ConsentStatus StatusToProtoEnum(
 
 }  // namespace
 
-ConsentAuditor::ConsentAuditor(PrefService* pref_service,
+ConsentAuditor::ConsentAuditor(
+    PrefService* pref_service,
+    std::unique_ptr<syncer::ConsentSyncBridge> consent_sync_bridge,
     syncer::UserEventService* user_event_service,
     const std::string& app_version,
     const std::string& app_locale)
     : pref_service_(pref_service),
+      consent_sync_bridge_(std::move(consent_sync_bridge)),
       user_event_service_(user_event_service),
       app_version_(app_version),
       app_locale_(app_locale) {
   DCHECK(pref_service_);
+  // TODO(vitaliii): Don't require user_event_service when the separate datatype
+  // is enabled.
   DCHECK(user_event_service_);
 }
 
@@ -152,4 +157,12 @@ void ConsentAuditor::RecordLocalConsent(const std::string& feature,
   consents->SetKey(feature, std::move(record));
 }
 
+base::WeakPtr<syncer::ModelTypeControllerDelegate>
+ConsentAuditor::GetControllerDelegateOnUIThread() {
+  if (consent_sync_bridge_) {
+    return consent_sync_bridge_->GetControllerDelegateOnUIThread();
+  }
+  return base::WeakPtr<syncer::ModelTypeControllerDelegate>();
+}
+
 }  // namespace consent_auditor

components/consent_auditor/consent_auditor.h

@@ -10,9 +10,12 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "components/consent_auditor/consent_sync_bridge.h"
 #include "components/keyed_service/core/keyed_service.h"
 
 namespace syncer {
+class ModelTypeControllerDelegate;
 class UserEventService;
 }
 
@@ -51,6 +54,7 @@ enum class ConsentStatus { NOT_GIVEN, GIVEN };
 class ConsentAuditor : public KeyedService {
  public:
   ConsentAuditor(PrefService* pref_service,
+                 std::unique_ptr<syncer::ConsentSyncBridge> consent_sync_bridge,
                  syncer::UserEventService* user_event_service,
                  const std::string& app_version,
                  const std::string& app_locale);
@@ -81,6 +85,10 @@ class ConsentAuditor : public KeyedService {
                           const std::string& description_text,
                           const std::string& confirmation_text);
 
+  // Returns the underlying Sync integration point.
+  base::WeakPtr<syncer::ModelTypeControllerDelegate>
+  GetControllerDelegateOnUIThread();
+
  private:
   std::unique_ptr<sync_pb::UserEventSpecifics> ConstructUserConsent(
       const std::string& account_id,
@@ -90,6 +98,7 @@ class ConsentAuditor : public KeyedService {
       ConsentStatus status);
 
   PrefService* pref_service_;
+  std::unique_ptr<syncer::ConsentSyncBridge> consent_sync_bridge_;
   syncer::UserEventService* user_event_service_;
   std::string app_version_;
   std::string app_locale_;

components/consent_auditor/consent_auditor_unittest.cc

@@ -6,10 +6,12 @@
 
 #include <memory>
 
+#include "base/memory/weak_ptr.h"
 #include "base/test/scoped_feature_list.h"
 #include "components/consent_auditor/pref_names.h"
 #include "components/prefs/testing_pref_service.h"
 #include "components/sync/driver/sync_driver_switches.h"
+#include "components/sync/model/fake_model_type_controller_delegate.h"
 #include "components/sync/user_events/fake_user_event_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -65,6 +67,30 @@ void LoadEntriesFromLocalConsentRecord(const base::Value* consents,
   *locale = locale_entry->GetString();
 }
 
+class FakeConsentSyncBridge : public syncer::ConsentSyncBridge {
+ public:
+  ~FakeConsentSyncBridge() override = default;
+
+  // ConsentSyncBridge implementation.
+  void RecordConsent(
+      std::unique_ptr<sync_pb::UserConsentSpecifics> specifics) override {
+    NOTIMPLEMENTED();
+  }
+
+  base::WeakPtr<syncer::ModelTypeControllerDelegate>
+  GetControllerDelegateOnUIThread() override {
+    return delegate_;
+  }
+
+  void SetControllerDelegateOnUIThread(
+      base::WeakPtr<syncer::ModelTypeControllerDelegate> delegate) {
+    delegate_ = delegate;
+  }
+
+ private:
+  base::WeakPtr<syncer::ModelTypeControllerDelegate> delegate_;
+};
+
 }  // namespace
 
 class ConsentAuditorTest : public testing::Test {
@@ -78,20 +104,23 @@ class ConsentAuditorTest : public testing::Test {
     BuildConsentAuditor();
   }
 
+  // TODO(vitaliii): Add a real builder class instead.
   void BuildConsentAuditor() {
     consent_auditor_ = std::make_unique<ConsentAuditor>(
-        pref_service_.get(), user_event_service_.get(), app_version_,
-        app_locale_);
+        pref_service_.get(), std::move(consent_sync_bridge_),
+        user_event_service_.get(), app_version_, app_locale_);
   }
 
   // These have no effect before |BuildConsentAuditor|.
   void SetAppVersion(const std::string& new_app_version) {
     app_version_ = new_app_version;
   }
-
   void SetAppLocale(const std::string& new_app_locale) {
     app_locale_ = new_app_locale;
   }
+  void SetConsentSyncBridge(std::unique_ptr<syncer::ConsentSyncBridge> bridge) {
+    consent_sync_bridge_ = std::move(bridge);
+  }
 
   ConsentAuditor* consent_auditor() { return consent_auditor_.get(); }
   PrefService* pref_service() const { return pref_service_.get(); }
@@ -105,6 +134,7 @@ class ConsentAuditorTest : public testing::Test {
   std::unique_ptr<syncer::FakeUserEventService> user_event_service_;
   std::string app_version_;
   std::string app_locale_;
+  std::unique_ptr<syncer::ConsentSyncBridge> consent_sync_bridge_;
 };
 
 TEST_F(ConsentAuditorTest, LocalConsentPrefRepresentation) {
@@ -222,4 +252,31 @@ TEST_F(ConsentAuditorTest, RecordGaiaConsent) {
   EXPECT_EQ(kCurrentAppLocale, consent.locale());
 }
 
+TEST_F(ConsentAuditorTest, ShouldReturnNoSyncDelegateWhenNoBridge) {
+  SetConsentSyncBridge(nullptr);
+  BuildConsentAuditor();
+
+  // There is no bridge (i.e. separate sync type for consents is disabled),
+  // thus, there should be no delegate as well.
+  EXPECT_EQ(nullptr, consent_auditor()->GetControllerDelegateOnUIThread());
+}
+
+TEST_F(ConsentAuditorTest, ShouldReturnSyncDelegateWhenBridgePresent) {
+  auto fake_bridge = std::make_unique<FakeConsentSyncBridge>();
+
+  syncer::FakeModelTypeControllerDelegate fake_delegate(
+      syncer::ModelType::USER_CONSENTS);
+  auto expected_delegate_ptr = fake_delegate.GetWeakPtr();
+  DCHECK(expected_delegate_ptr);
+  fake_bridge->SetControllerDelegateOnUIThread(expected_delegate_ptr);
+
+  SetConsentSyncBridge(std::move(fake_bridge));
+  BuildConsentAuditor();
+
+  // There is a bridge (i.e. separate sync type for consents is enabled), thus,
+  // there should be a delegate as well.
+  EXPECT_EQ(expected_delegate_ptr.get(),
+            consent_auditor()->GetControllerDelegateOnUIThread().get());
+}
+
 }  // namespace consent_auditor

components/consent_auditor/fake_consent_auditor.cc

@@ -12,9 +12,10 @@ FakeConsentAuditor::FakeConsentAuditor(
     PrefService* pref_service,
     syncer::UserEventService* user_event_service)
     : ConsentAuditor(pref_service,
+                     /*consent_sync_bridge=*/nullptr,
                      user_event_service,
-                     std::string(),
-                     std::string()) {}
+                     /*app_version=*/std::string(),
+                     /*app_locale=*/std::string()) {}
 
 FakeConsentAuditor::~FakeConsentAuditor() {}
 

components/sync/driver/sync_driver_switches.cc

@@ -49,6 +49,11 @@ const base::Feature kSyncUserFieldTrialEvents{"SyncUserFieldTrialEvents",
 const base::Feature kSyncUserConsentEvents{"SyncUserConsentEvents",
                                            base::FEATURE_ENABLED_BY_DEFAULT};
 
+// Emit user consents through a separate sync type USER_CONSENTS instead of
+// USER_EVENTS. This feature does not override kSyncUserConsentEvents.
+const base::Feature kSyncUserConsentSeparateType{
+    "SyncUserConsentSeparateType", base::FEATURE_DISABLED_BY_DEFAULT};
+
 // Gates registration for user language detection events.
 const base::Feature kSyncUserLanguageDetectionEvents{
     "SyncUserLanguageDetectionEvents", base::FEATURE_DISABLED_BY_DEFAULT};

components/sync/driver/sync_driver_switches.h

@@ -23,6 +23,7 @@ extern const base::Feature kSyncClearDataOnPassphraseEncryption;
 extern const base::Feature kSyncUserEvents;
 extern const base::Feature kSyncUserFieldTrialEvents;
 extern const base::Feature kSyncUserConsentEvents;
+extern const base::Feature kSyncUserConsentSeparateType;
 extern const base::Feature kSyncUserLanguageDetectionEvents;
 extern const base::Feature kSyncUserTranslationEvents;
 extern const base::Feature kSyncUSSBookmarks;

ios/chrome/browser/consent_auditor/consent_auditor_factory.cc

@@ -50,8 +50,10 @@ std::unique_ptr<KeyedService> ConsentAuditorFactory::BuildServiceInstanceFor(
       ios::ChromeBrowserState::FromBrowserState(browser_state);
   syncer::UserEventService* const user_event_service =
       IOSUserEventServiceFactory::GetForBrowserState(ios_browser_state);
+  // TODO(crbug.com/840357): Wire ConsentSyncBridge behind a feature.
   return std::make_unique<consent_auditor::ConsentAuditor>(
-      ios_browser_state->GetPrefs(), user_event_service,
+      ios_browser_state->GetPrefs(), /*consent_sync_bridge=*/nullptr,
+      user_event_service,
       // The browser version and locale do not change runtime, so we can pass
       // them directly.
       version_info::GetVersionNumber(),

ios/chrome/browser/sync/profile_sync_service_factory_unittest.cc

@@ -9,11 +9,13 @@
 #include <vector>
 
 #include "base/command_line.h"
+#include "base/feature_list.h"
 #include "base/task_scheduler/task_scheduler.h"
 #include "components/browser_sync/browser_sync_switches.h"
 #include "components/browser_sync/profile_sync_service.h"
 #include "components/sync/base/model_type.h"
 #include "components/sync/driver/data_type_controller.h"
+#include "components/sync/driver/sync_driver_switches.h"
 #include "ios/chrome/browser/browser_state/test_chrome_browser_state.h"
 #include "ios/web/public/test/test_web_thread_bundle.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -64,8 +66,9 @@ class ProfileSyncServiceFactoryTest : public PlatformTest {
     datatypes.push_back(syncer::PROXY_TABS);
     datatypes.push_back(syncer::TYPED_URLS);
     datatypes.push_back(syncer::USER_EVENTS);
-    // TODO(crbug.com/840357): Add USER_CONSENTS, when the type is enabled by
-    // default.
+    if (base::FeatureList::IsEnabled(switches::kSyncUserConsentSeparateType)) {
+      datatypes.push_back(syncer::USER_CONSENTS);
+    }
 
     return datatypes;
   }

ios/chrome/browser/ui/authentication/chrome_signin_view_controller_unittest.mm

@@ -100,6 +100,7 @@ class FakeConsentAuditor : public consent_auditor::ConsentAuditor {
                      const std::string& app_version,
                      const std::string& app_locale)
       : ConsentAuditor(pref_service,
+                       /*consent_sync_bridge=*/nullptr,
                        user_event_service,
                        app_version,
                        app_locale) {}