[User events] Get rid of dependency: UserEventService -> PSS

This CL gets rid of ProfileSyncService usage both in UserEventService
and in its factory. This usage was for checking whether user events
should get synced up. Part of these checks were obsolete (by having
USER_CONSENT a separate data type now) as user consents are always
enabled together with history sync. Part of these checks were moved to
a new controller subclass for USER_EVENTS. This controller notably
checks whether custom passphrase encryption is not enabled.

Bug: 850428
Change-Id: Ie8be4e37c5c4a51d9c3c1f17c74cd9fb7449a52c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1349257Reviewed-by: Markus Heintz <markusheintz@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#649119}

chrome/browser/sync/test/integration/single_client_user_events_sync_test.cc

@@ -7,12 +7,15 @@
 #include "base/bind.h"
 #include "base/macros.h"
 #include "base/test/scoped_feature_list.h"
+#include "base/time/time.h"
+#include "chrome/browser/sync/test/integration/encryption_helper.h"
 #include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
 #include "chrome/browser/sync/test/integration/sessions_helper.h"
 #include "chrome/browser/sync/test/integration/single_client_status_change_checker.h"
 #include "chrome/browser/sync/test/integration/status_change_checker.h"
 #include "chrome/browser/sync/test/integration/sync_integration_test_util.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
+#include "chrome/browser/sync/test/integration/user_events_helper.h"
 #include "chrome/browser/sync/user_event_service_factory.h"
 #include "components/sync/driver/sync_driver_switches.h"
 #include "components/sync/model/model_type_sync_bridge.h"
@@ -21,20 +24,13 @@
 #include "components/variations/variations_associated_data.h"
 
 using fake_server::FakeServer;
-using sync_pb::UserEventSpecifics;
-using sync_pb::SyncEntity;
 using sync_pb::CommitResponse;
+using sync_pb::SyncEntity;
+using sync_pb::UserEventSpecifics;
+using user_events_helper::CreateTestEvent;
 
 namespace {
 
-UserEventSpecifics CreateTestEvent(int microseconds) {
-  UserEventSpecifics specifics;
-  specifics.set_event_time_usec(microseconds);
-  specifics.set_navigation_id(microseconds);
-  specifics.mutable_test_event();
-  return specifics;
-}
-
 CommitResponse::ResponseType BounceType(
     CommitResponse::ResponseType type,
     const syncer::LoopbackServerEntity& entity) {
@@ -56,65 +52,6 @@ CommitResponse::ResponseType TransientErrorFirst(
   }
 }
 
-class UserEventEqualityChecker : public SingleClientStatusChangeChecker {
- public:
-  UserEventEqualityChecker(syncer::ProfileSyncService* service,
-                           FakeServer* fake_server,
-                           std::vector<UserEventSpecifics> expected_specifics)
-      : SingleClientStatusChangeChecker(service), fake_server_(fake_server) {
-    for (const UserEventSpecifics& specifics : expected_specifics) {
-      expected_specifics_.insert(std::pair<int64_t, UserEventSpecifics>(
-          specifics.event_time_usec(), specifics));
-    }
-  }
-
-  bool IsExitConditionSatisfied() override {
-    std::vector<SyncEntity> entities =
-        fake_server_->GetSyncEntitiesByModelType(syncer::USER_EVENTS);
-
-    // |entities.size()| is only going to grow, if |entities.size()| ever
-    // becomes bigger then all hope is lost of passing, stop now.
-    EXPECT_GE(expected_specifics_.size(), entities.size());
-
-    if (expected_specifics_.size() > entities.size()) {
-      return false;
-    }
-
-    // Number of events on server matches expected, exit condition is satisfied.
-    // Let's verify that content matches as well. It is safe to modify
-    // |expected_specifics_|.
-    for (const SyncEntity& entity : entities) {
-      UserEventSpecifics server_specifics = entity.specifics().user_event();
-      auto iter = expected_specifics_.find(server_specifics.event_time_usec());
-      // We don't expect to encounter id matching events with different values,
-      // this isn't going to recover so fail the test case now.
-      EXPECT_TRUE(expected_specifics_.end() != iter);
-      if (expected_specifics_.end() == iter) {
-        return false;
-      }
-      // TODO(skym): This may need to change if we start updating navigation_id
-      // based on what sessions data is committed, and end up committing the
-      // same event multiple times.
-      EXPECT_EQ(iter->second.navigation_id(), server_specifics.navigation_id());
-      EXPECT_EQ(iter->second.event_case(), server_specifics.event_case());
-
-      expected_specifics_.erase(iter);
-    }
-
-    return true;
-  }
-
-  std::string GetDebugMessage() const override {
-    return "Waiting server side USER_EVENTS to match expected.";
-  }
-
- private:
-  FakeServer* fake_server_;
-  std::multimap<int64_t, UserEventSpecifics> expected_specifics_;
-
-  DISALLOW_COPY_AND_ASSIGN(UserEventEqualityChecker);
-};
-
 // A more simplistic version of UserEventEqualityChecker that only checks the
 // case of the events. This is helpful if you do not know (or control) some of
 // the fields of the events that are created.
@@ -191,15 +128,17 @@ IN_PROC_BROWSER_TEST_F(SingleClientUserEventsSyncTest, Sanity) {
       GetFakeServer()->GetSyncEntitiesByModelType(syncer::USER_EVENTS).size());
   syncer::UserEventService* event_service =
       browser_sync::UserEventServiceFactory::GetForProfile(GetProfile(0));
-  const UserEventSpecifics specifics = CreateTestEvent(0);
+  const UserEventSpecifics specifics = CreateTestEvent(base::Time());
   event_service->RecordUserEvent(specifics);
   EXPECT_TRUE(ExpectUserEvents({specifics}));
 }
 
 IN_PROC_BROWSER_TEST_F(SingleClientUserEventsSyncTest, RetrySequential) {
   ASSERT_TRUE(SetupSync());
-  const UserEventSpecifics specifics1 = CreateTestEvent(1);
-  const UserEventSpecifics specifics2 = CreateTestEvent(2);
+  const UserEventSpecifics specifics1 =
+      CreateTestEvent(base::Time() + base::TimeDelta::FromMicroseconds(1));
+  const UserEventSpecifics specifics2 =
+      CreateTestEvent(base::Time() + base::TimeDelta::FromMicroseconds(2));
   syncer::UserEventService* event_service =
       browser_sync::UserEventServiceFactory::GetForProfile(GetProfile(0));
 
@@ -225,8 +164,10 @@ IN_PROC_BROWSER_TEST_F(SingleClientUserEventsSyncTest, RetrySequential) {
 IN_PROC_BROWSER_TEST_F(SingleClientUserEventsSyncTest, RetryParallel) {
   ASSERT_TRUE(SetupSync());
   bool first = true;
-  const UserEventSpecifics specifics1 = CreateTestEvent(1);
-  const UserEventSpecifics specifics2 = CreateTestEvent(2);
+  const UserEventSpecifics specifics1 =
+      CreateTestEvent(base::Time() + base::TimeDelta::FromMicroseconds(1));
+  const UserEventSpecifics specifics2 =
+      CreateTestEvent(base::Time() + base::TimeDelta::FromMicroseconds(2));
   UserEventSpecifics retry_specifics;
 
   syncer::UserEventService* event_service =
@@ -245,9 +186,12 @@ IN_PROC_BROWSER_TEST_F(SingleClientUserEventsSyncTest, RetryParallel) {
 }
 
 IN_PROC_BROWSER_TEST_F(SingleClientUserEventsSyncTest, NoHistory) {
-  const UserEventSpecifics test_event1 = CreateTestEvent(1);
-  const UserEventSpecifics test_event2 = CreateTestEvent(2);
-  const UserEventSpecifics test_event3 = CreateTestEvent(3);
+  const UserEventSpecifics test_event1 =
+      CreateTestEvent(base::Time() + base::TimeDelta::FromMicroseconds(1));
+  const UserEventSpecifics test_event2 =
+      CreateTestEvent(base::Time() + base::TimeDelta::FromMicroseconds(2));
+  const UserEventSpecifics test_event3 =
+      CreateTestEvent(base::Time() + base::TimeDelta::FromMicroseconds(3));
 
   ASSERT_TRUE(SetupSync());
   syncer::UserEventService* event_service =
@@ -270,7 +214,8 @@ IN_PROC_BROWSER_TEST_F(SingleClientUserEventsSyncTest, NoHistory) {
 }
 
 IN_PROC_BROWSER_TEST_F(SingleClientUserEventsSyncTest, NoSessions) {
-  const UserEventSpecifics specifics = CreateTestEvent(1);
+  const UserEventSpecifics specifics =
+      CreateTestEvent(base::Time() + base::TimeDelta::FromMicroseconds(1));
   ASSERT_TRUE(SetupSync());
   ASSERT_TRUE(GetClient(0)->DisableSyncForDatatype(syncer::PROXY_TABS));
   syncer::UserEventService* event_service =
@@ -283,15 +228,17 @@ IN_PROC_BROWSER_TEST_F(SingleClientUserEventsSyncTest, NoSessions) {
 }
 
 IN_PROC_BROWSER_TEST_F(SingleClientUserEventsSyncTest, Encryption) {
-  const UserEventSpecifics test_event1 = CreateTestEvent(1);
-  const UserEventSpecifics test_event2 = CreateTestEvent(2);
+  const UserEventSpecifics test_event1 =
+      CreateTestEvent(base::Time() + base::TimeDelta::FromMicroseconds(1));
+  const UserEventSpecifics test_event2 =
+      CreateTestEvent(base::Time() + base::TimeDelta::FromMicroseconds(2));
 
   ASSERT_TRUE(SetupSync());
   syncer::UserEventService* event_service =
       browser_sync::UserEventServiceFactory::GetForProfile(GetProfile(0));
   event_service->RecordUserEvent(test_event1);
-  ASSERT_TRUE(EnableEncryption(0));
   EXPECT_TRUE(ExpectUserEvents({test_event1}));
+  ASSERT_TRUE(EnableEncryption(0));
   event_service->RecordUserEvent(test_event2);
 
   // Just checking that we don't see test_event2 isn't very convincing yet,

chrome/browser/sync/test/integration/two_client_user_events_sync_test.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/time/time.h"
+#include "chrome/browser/sync/test/integration/bookmarks_helper.h"
+#include "chrome/browser/sync/test/integration/encryption_helper.h"
+#include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
+#include "chrome/browser/sync/test/integration/sync_integration_test_util.h"
+#include "chrome/browser/sync/test/integration/sync_test.h"
+#include "chrome/browser/sync/test/integration/user_events_helper.h"
+#include "chrome/browser/sync/user_event_service_factory.h"
+#include "components/sync/protocol/user_event_specifics.pb.h"
+#include "components/sync/user_events/user_event_service.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+using sync_pb::UserEventSpecifics;
+
+const int kEncryptingClientId = 0;
+const int kDecryptingClientId = 1;
+
+class TwoClientUserEventsSyncTest : public SyncTest {
+ public:
+  TwoClientUserEventsSyncTest() : SyncTest(TWO_CLIENT) {}
+  ~TwoClientUserEventsSyncTest() override {}
+
+  bool ExpectNoUserEvent(int index) {
+    return UserEventEqualityChecker(GetSyncService(index), GetFakeServer(),
+                                    /*expected_specifics=*/{})
+        .Wait();
+  }
+
+  bool WaitForPassphraseRequiredState(int index, bool desired_state) {
+    return PassphraseRequiredStateChecker(GetSyncService(index), desired_state)
+        .Wait();
+  }
+
+  bool WaitForBookmarksToMatchVerifier() {
+    return BookmarksMatchVerifierChecker().Wait();
+  }
+
+  void AddTestBookmarksToClient(int index) {
+    ASSERT_TRUE(
+        bookmarks_helper::AddURL(index, 0, "What are you syncing about?",
+                                 GURL("https://google.com/synced-bookmark-1")));
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TwoClientUserEventsSyncTest);
+};
+
+IN_PROC_BROWSER_TEST_F(TwoClientUserEventsSyncTest,
+                       SetPassphraseAndRecordEventAndThenSetupSync) {
+  ASSERT_TRUE(SetupClients());
+  ASSERT_TRUE(GetClient(kEncryptingClientId)->SetupSync());
+
+  // Set up a sync client with custom passphrase to get the data encrypted on
+  // the server.
+  GetSyncService(kEncryptingClientId)
+      ->GetUserSettings()
+      ->SetEncryptionPassphrase("hunter2");
+  ASSERT_TRUE(
+      PassphraseAcceptedChecker(GetSyncService(kEncryptingClientId)).Wait());
+
+  // Record a user event on the second client before setting up sync (before
+  // knowing it will be encrypted). This event should not get recorded while
+  // starting up sync because the user has custom passphrase setup.
+  syncer::UserEventService* event_service =
+      browser_sync::UserEventServiceFactory::GetForProfile(GetProfile(0));
+  event_service->RecordUserEvent(user_events_helper::CreateTestEvent(
+      base::Time() + base::TimeDelta::FromMicroseconds(1)));
+
+  // Set up sync on the second client.
+  ASSERT_TRUE(
+      GetClient(kDecryptingClientId)
+          ->SetupSyncNoWaitForCompletion(syncer::UserSelectableTypes()));
+  // The second client asks the user to provide a password for decryption.
+  ASSERT_TRUE(
+      PassphraseRequiredChecker(GetSyncService(kDecryptingClientId)).Wait());
+  // Provide the password.
+  ASSERT_TRUE(GetSyncService(kDecryptingClientId)
+                  ->GetUserSettings()
+                  ->SetDecryptionPassphrase("hunter2"));
+  // Check it is accepted and finish the setup.
+  ASSERT_TRUE(
+      PassphraseAcceptedChecker(GetSyncService(kDecryptingClientId)).Wait());
+  GetClient(kDecryptingClientId)->FinishSyncSetup();
+
+  // Just checking that we don't see the recorded test event isn't very
+  // convincing yet, because it may simply not have reached the server yet. So
+  // let's send something else on the second client through the system that we
+  // can wait on before checking. Bookmark data was picked fairly arbitrarily.
+  AddTestBookmarksToClient(kDecryptingClientId);
+  ASSERT_TRUE(WaitForBookmarksToMatchVerifier());
+
+  // Finally, make sure no user event got sent to the server.
+  EXPECT_TRUE(ExpectNoUserEvent(kDecryptingClientId));
+}
+
+}  // namespace

chrome/browser/sync/test/integration/user_events_helper.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/sync/test/integration/user_events_helper.h"
+
+#include <string>
+#include <vector>
+
+#include "chrome/browser/sync/test/integration/single_client_status_change_checker.h"
+#include "components/sync/test/fake_server/fake_server.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using fake_server::FakeServer;
+using sync_pb::SyncEntity;
+using sync_pb::UserEventSpecifics;
+
+namespace user_events_helper {
+
+UserEventSpecifics CreateTestEvent(base::Time time) {
+  UserEventSpecifics specifics;
+  specifics.set_event_time_usec(
+      time.ToDeltaSinceWindowsEpoch().InMicroseconds());
+  specifics.set_navigation_id(time.ToDeltaSinceWindowsEpoch().InMicroseconds());
+  specifics.mutable_test_event();
+  return specifics;
+}
+
+}  // namespace user_events_helper
+
+UserEventEqualityChecker::UserEventEqualityChecker(
+    syncer::ProfileSyncService* service,
+    FakeServer* fake_server,
+    std::vector<UserEventSpecifics> expected_specifics)
+    : SingleClientStatusChangeChecker(service), fake_server_(fake_server) {
+  for (const UserEventSpecifics& specifics : expected_specifics) {
+    expected_specifics_.emplace(specifics.event_time_usec(), specifics);
+  }
+}
+
+UserEventEqualityChecker::~UserEventEqualityChecker() = default;
+
+bool UserEventEqualityChecker::IsExitConditionSatisfied() {
+  std::vector<SyncEntity> entities =
+      fake_server_->GetSyncEntitiesByModelType(syncer::USER_EVENTS);
+
+  // |entities.size()| is only going to grow, if |entities.size()| ever
+  // becomes bigger then all hope is lost of passing, stop now.
+  EXPECT_GE(expected_specifics_.size(), entities.size());
+
+  if (expected_specifics_.size() > entities.size()) {
+    return false;
+  }
+
+  // Number of events on server matches expected, exit condition is satisfied.
+  // Let's verify that content matches as well. It is safe to modify
+  // |expected_specifics_|.
+  for (const SyncEntity& entity : entities) {
+    UserEventSpecifics server_specifics = entity.specifics().user_event();
+    auto iter = expected_specifics_.find(server_specifics.event_time_usec());
+    // We don't expect to encounter id matching events with different values,
+    // this isn't going to recover so fail the test case now.
+    EXPECT_TRUE(expected_specifics_.end() != iter);
+    if (expected_specifics_.end() == iter) {
+      return false;
+    }
+    // TODO(skym): This may need to change if we start updating navigation_id
+    // based on what sessions data is committed, and end up committing the
+    // same event multiple times.
+    EXPECT_EQ(iter->second.navigation_id(), server_specifics.navigation_id());
+    EXPECT_EQ(iter->second.event_case(), server_specifics.event_case());
+
+    expected_specifics_.erase(iter);
+  }
+
+  return true;
+}
+
+std::string UserEventEqualityChecker::GetDebugMessage() const {
+  return "Waiting server side USER_EVENTS to match expected.";
+}

chrome/browser/sync/test/integration/user_events_helper.h

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_SYNC_TEST_INTEGRATION_USER_EVENTS_HELPER_H_
+#define CHROME_BROWSER_SYNC_TEST_INTEGRATION_USER_EVENTS_HELPER_H_
+
+#include <string>
+
+#include "base/time/time.h"
+#include "chrome/browser/sync/test/integration/single_client_status_change_checker.h"
+#include "components/sync/protocol/user_event_specifics.pb.h"
+
+namespace fake_server {
+class FakeServer;
+}
+
+namespace user_events_helper {
+
+// Creates a test user event with specified event time.
+sync_pb::UserEventSpecifics CreateTestEvent(base::Time time);
+
+}  // namespace user_events_helper
+
+class UserEventEqualityChecker : public SingleClientStatusChangeChecker {
+ public:
+  UserEventEqualityChecker(
+      syncer::ProfileSyncService* service,
+      fake_server::FakeServer* fake_server,
+      std::vector<sync_pb::UserEventSpecifics> expected_specifics);
+  ~UserEventEqualityChecker() override;
+
+  bool IsExitConditionSatisfied() override;
+
+  std::string GetDebugMessage() const override;
+
+ private:
+  fake_server::FakeServer* fake_server_;
+  std::multimap<int64_t, sync_pb::UserEventSpecifics> expected_specifics_;
+
+  DISALLOW_COPY_AND_ASSIGN(UserEventEqualityChecker);
+};
+
+#endif  // CHROME_BROWSER_SYNC_TEST_INTEGRATION_USER_EVENTS_HELPER_H_

chrome/browser/sync/user_event_service_factory.cc

@@ -11,7 +11,6 @@
 #include "chrome/browser/profiles/incognito_helpers.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/model_type_store_service_factory.h"
-#include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/browser/sync/session_sync_service_factory.h"
 #include "chrome/common/channel_info.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
@@ -45,25 +44,17 @@ UserEventServiceFactory::UserEventServiceFactory()
           BrowserContextDependencyManager::GetInstance()) {
   DependsOn(ModelTypeStoreServiceFactory::GetInstance());
   DependsOn(SessionSyncServiceFactory::GetInstance());
-  // TODO(vitaliii): This is missing
-  // DependsOn(ProfileSyncServiceFactory::GetInstance()), which we can't
-  // simply add because ProfileSyncServiceFactory itself depends on this
-  // factory. This won't be relevant anymore once the separate consents datatype
-  // is fully launched.
 }
 
 UserEventServiceFactory::~UserEventServiceFactory() {}
 
 KeyedService* UserEventServiceFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
-  Profile* profile = Profile::FromBrowserContext(context);
-  syncer::SyncService* sync_service =
-      ProfileSyncServiceFactory::GetForProfile(profile);
-  if (!syncer::UserEventServiceImpl::MightRecordEvents(
-          context->IsOffTheRecord(), sync_service)) {
+  if (context->IsOffTheRecord()) {
     return new syncer::NoOpUserEventService();
   }
 
+  Profile* profile = Profile::FromBrowserContext(context);
   syncer::OnceModelTypeStoreFactory store_factory =
       ModelTypeStoreServiceFactory::GetForProfile(profile)->GetStoreFactory();
 
@@ -75,7 +66,7 @@ KeyedService* UserEventServiceFactory::BuildServiceInstanceFor(
   auto bridge = std::make_unique<syncer::UserEventSyncBridge>(
       std::move(store_factory), std::move(change_processor),
       SessionSyncServiceFactory::GetForProfile(profile)->GetGlobalIdMapper());
-  return new syncer::UserEventServiceImpl(sync_service, std::move(bridge));
+  return new syncer::UserEventServiceImpl(std::move(bridge));
 }
 
 content::BrowserContext* UserEventServiceFactory::GetBrowserContextToUse(

chrome/test/BUILD.gn

@@ -5470,6 +5470,8 @@ if (!is_android && !is_fuchsia) {
       "../browser/sync/test/integration/typed_urls_helper.h",
       "../browser/sync/test/integration/updated_progress_marker_checker.cc",
       "../browser/sync/test/integration/updated_progress_marker_checker.h",
+      "../browser/sync/test/integration/user_events_helper.cc",
+      "../browser/sync/test/integration/user_events_helper.h",
       "../browser/sync/test/integration/wallet_helper.cc",
       "../browser/sync/test/integration/wallet_helper.h",
     ]
@@ -5573,6 +5575,7 @@ if (!is_android && !is_fuchsia) {
       "../browser/sync/test/integration/two_client_sessions_sync_test.cc",
       "../browser/sync/test/integration/two_client_themes_sync_test.cc",
       "../browser/sync/test/integration/two_client_typed_urls_sync_test.cc",
+      "../browser/sync/test/integration/two_client_user_events_sync_test.cc",
       "../browser/sync/test/integration/two_client_uss_sync_test.cc",
       "../browser/sync/test/integration/two_client_wallet_sync_test.cc",
     ]

components/browser_sync/profile_sync_components_factory_impl.cc

@@ -41,6 +41,7 @@
 #include "components/sync/model/model_type_store_service.h"
 #include "components/sync/model_impl/forwarding_model_type_controller_delegate.h"
 #include "components/sync/model_impl/proxy_model_type_controller_delegate.h"
+#include "components/sync/user_events/user_event_model_type_controller.h"
 #include "components/sync_bookmarks/bookmark_change_processor.h"
 #include "components/sync_bookmarks/bookmark_data_type_controller.h"
 #include "components/sync_bookmarks/bookmark_model_associator.h"
@@ -397,8 +398,16 @@ ProfileSyncComponentsFactoryImpl::CreateCommonDataTypeControllers(
   }
 
   if (!disabled_types.Has(syncer::USER_EVENTS)) {
-    controllers.push_back(CreateModelTypeControllerForModelRunningOnUIThread(
-        syncer::USER_EVENTS));
+    // TODO(crbug.com/867801): Switch to forwarding delegate.
+    controllers.push_back(
+        std::make_unique<syncer::UserEventModelTypeController>(
+            sync_service,
+            std::make_unique<syncer::ProxyModelTypeControllerDelegate>(
+                ui_thread_,
+                base::BindRepeating(&browser_sync::BrowserSyncClient::
+                                        GetControllerDelegateForModelType,
+                                    base::Unretained(sync_client_),
+                                    syncer::USER_EVENTS))));
   }
 
   if (!disabled_types.Has(syncer::SEND_TAB_TO_SELF) &&

components/sync/BUILD.gn

@@ -563,6 +563,8 @@ jumbo_static_library("sync") {
     "user_events/fake_user_event_service.h",
     "user_events/no_op_user_event_service.cc",
     "user_events/no_op_user_event_service.h",
+    "user_events/user_event_model_type_controller.cc",
+    "user_events/user_event_model_type_controller.h",
     "user_events/user_event_service.h",
     "user_events/user_event_service_impl.cc",
     "user_events/user_event_service_impl.h",

components/sync/user_events/user_event_model_type_controller.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/sync/user_events/user_event_model_type_controller.h"
+
+#include <utility>
+
+#include "base/bind.h"
+#include "components/sync/driver/sync_service.h"
+#include "components/sync/driver/sync_user_settings.h"
+
+namespace syncer {
+
+UserEventModelTypeController::UserEventModelTypeController(
+    SyncService* sync_service,
+    std::unique_ptr<ModelTypeControllerDelegate> delegate_on_disk)
+    : ModelTypeController(syncer::USER_EVENTS, std::move(delegate_on_disk)),
+      sync_service_(sync_service) {
+  DCHECK(sync_service_);
+  sync_service_->AddObserver(this);
+}
+
+UserEventModelTypeController::~UserEventModelTypeController() {
+  sync_service_->RemoveObserver(this);
+}
+
+bool UserEventModelTypeController::ReadyForStart() const {
+  return !sync_service_->GetUserSettings()->IsUsingSecondaryPassphrase();
+}
+
+void UserEventModelTypeController::OnStateChanged(syncer::SyncService* sync) {
+  // Just disable if we start encrypting; we do not care about re-enabling it
+  // during run-time.
+  if (ReadyForStart()) {
+    return;
+  }
+
+  // This results in stopping the data type (and is a no-op if the data type is
+  // already stopped because of being unready).
+  sync->ReadyForStartChanged(type());
+}
+
+}  // namespace syncer

components/sync/user_events/user_event_model_type_controller.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_SYNC_USER_EVENTS_USER_EVENT_MODEL_TYPE_CONTROLLER_H_
+#define COMPONENTS_SYNC_USER_EVENTS_USER_EVENT_MODEL_TYPE_CONTROLLER_H_
+
+#include <memory>
+
+#include "components/sync/driver/model_type_controller.h"
+#include "components/sync/driver/sync_service_observer.h"
+
+namespace syncer {
+
+class ModelTypeControllerDelegate;
+class SyncService;
+
+class UserEventModelTypeController : public syncer::ModelTypeController,
+                                     public syncer::SyncServiceObserver {
+ public:
+  // |sync_service| must not be null and must outlive this object.
+  UserEventModelTypeController(
+      SyncService* sync_service,
+      std::unique_ptr<ModelTypeControllerDelegate> delegate_on_disk);
+  ~UserEventModelTypeController() override;
+
+  // syncer::DataTypeController implementation.
+  bool ReadyForStart() const override;
+
+  // syncer::SyncServiceObserver implementation.
+  void OnStateChanged(syncer::SyncService* sync) override;
+
+ private:
+  SyncService* sync_service_;
+
+  DISALLOW_COPY_AND_ASSIGN(UserEventModelTypeController);
+};
+
+}  // namespace syncer
+
+#endif  // COMPONENTS_SYNC_USER_EVENTS_USER_EVENT_MODEL_TYPE_CONTROLLER_H_

components/sync/user_events/user_event_service_impl.cc

@@ -10,7 +10,6 @@
 #include "base/stl_util.h"
 #include "base/time/time.h"
 #include "components/sync/driver/sync_driver_switches.h"
-#include "components/sync/driver/sync_service.h"
 #include "components/sync/driver/sync_user_settings.h"
 #include "components/sync/user_events/user_event_sync_bridge.h"
 
@@ -61,13 +60,9 @@ bool NavigationPresenceValid(UserEventSpecifics::EventCase event_case,
 }  // namespace
 
 UserEventServiceImpl::UserEventServiceImpl(
-    SyncService* sync_service,
     std::unique_ptr<UserEventSyncBridge> bridge)
-    : sync_service_(sync_service),
-      bridge_(std::move(bridge)),
-      session_id_(base::RandUint64()) {
+    : bridge_(std::move(bridge)), session_id_(base::RandUint64()) {
   DCHECK(bridge_);
-  DCHECK(sync_service_);
 }
 
 UserEventServiceImpl::~UserEventServiceImpl() {}
@@ -92,32 +87,6 @@ ModelTypeSyncBridge* UserEventServiceImpl::GetSyncBridge() {
   return bridge_.get();
 }
 
-// static
-bool UserEventServiceImpl::MightRecordEvents(bool off_the_record,
-                                             SyncService* sync_service) {
-  return !off_the_record && sync_service;
-}
-
-bool UserEventServiceImpl::CanRecordHistory() {
-  // Before the engine is initialized, Sync doesn't know if there is a
-  // secondary passphrase. Similarly, unless the Sync feature is enabled,
-  // GetPreferredDataTypes() isn't meaningful.
-  return sync_service_->IsEngineInitialized() &&
-         !sync_service_->GetUserSettings()->IsUsingSecondaryPassphrase() &&
-         sync_service_->IsSyncFeatureEnabled() &&
-         sync_service_->GetPreferredDataTypes().Has(HISTORY_DELETE_DIRECTIVES);
-}
-
-bool UserEventServiceImpl::IsUserEventsDatatypeEnabled() {
-  // Before the engine is initialized, Sync doesn't know if there is a
-  // secondary passphrase. Similarly, unless the Sync feature is enabled,
-  // GetPreferredDataTypes() isn't meaningful.
-  return sync_service_->IsEngineInitialized() &&
-         !sync_service_->GetUserSettings()->IsUsingSecondaryPassphrase() &&
-         sync_service_->IsSyncFeatureEnabled() &&
-         sync_service_->GetPreferredDataTypes().Has(USER_EVENTS);
-}
-
 bool UserEventServiceImpl::ShouldRecordEvent(
     const UserEventSpecifics& specifics) {
   if (specifics.event_case() == UserEventSpecifics::EVENT_NOT_SET) {
@@ -129,21 +98,6 @@ bool UserEventServiceImpl::ShouldRecordEvent(
     return false;
   }
 
-  // TODO(vitaliii): Checking HISTORY_DELETE_DIRECTIVES directly should not be
-  // needed once USER_CONSENTS are fully launched. Then USER_EVENTS datatype
-  // should depend on History in Sync layers instead of here.
-  if (specifics.has_navigation_id() && !CanRecordHistory()) {
-    return false;
-  }
-
-  // TODO(vitaliii): Checking USER_EVENTS directly should not be needed once
-  // https://crbug.com/830535 is fixed. Then disabling USER_EVENTS should be
-  // honored by the processor and it should drop all events.
-  if (!IsUserEventsDatatypeEnabled()) {
-    DCHECK(!specifics.has_user_consent());
-    return false;
-  }
-
   return true;
 }
 

components/sync/user_events/user_event_service_impl.h

@@ -17,13 +17,11 @@
 namespace syncer {
 
 class ModelTypeSyncBridge;
-class SyncService;
 class UserEventSyncBridge;
 
 class UserEventServiceImpl : public UserEventService {
  public:
-  UserEventServiceImpl(SyncService* sync_service,
-                       std::unique_ptr<UserEventSyncBridge> bridge);
+  explicit UserEventServiceImpl(std::unique_ptr<UserEventSyncBridge> bridge);
   ~UserEventServiceImpl() override;
 
   // KeyedService implementation.
@@ -35,20 +33,10 @@ class UserEventServiceImpl : public UserEventService {
   void RecordUserEvent(const sync_pb::UserEventSpecifics& specifics) override;
   ModelTypeSyncBridge* GetSyncBridge() override;
 
-  // Checks known (and immutable) conditions that should not change at runtime.
-  static bool MightRecordEvents(bool off_the_record, SyncService* sync_service);
-
  private:
-  // Whether allowed to record events that link to navigation data.
-  bool CanRecordHistory();
-
-  bool IsUserEventsDatatypeEnabled();
-
   // Checks dynamic or event specific conditions.
   bool ShouldRecordEvent(const sync_pb::UserEventSpecifics& specifics);
 
-  SyncService* sync_service_;
-
   std::unique_ptr<UserEventSyncBridge> bridge_;
 
   // Holds onto a random number for the duration of this execution of chrome. On

components/sync/user_events/user_event_service_impl_unittest.cc

@@ -86,47 +86,16 @@ class UserEventServiceImplTest : public testing::Test {
   TestGlobalIdMapper mapper_;
 };
 
-TEST_F(UserEventServiceImplTest, MightRecordEvents) {
-  // All conditions are met, might record.
-  EXPECT_TRUE(UserEventServiceImpl::MightRecordEvents(false, sync_service()));
-  // No sync service, will not record.
-  EXPECT_FALSE(UserEventServiceImpl::MightRecordEvents(false, nullptr));
-  // Off the record, will not record.
-  EXPECT_FALSE(UserEventServiceImpl::MightRecordEvents(true, sync_service()));
-}
-
 TEST_F(UserEventServiceImplTest, ShouldRecord) {
-  UserEventServiceImpl service(sync_service(), MakeBridge());
-  EXPECT_CALL(*mock_processor(), Put(_, _, _));
-  service.RecordUserEvent(AsTest(Event()));
-}
-
-TEST_F(UserEventServiceImplTest,
-       ShouldOnlyRecordEventsWithoutNavIdWhenHistorySyncIsDisabled) {
-  sync_service()->SetPreferredDataTypes({USER_EVENTS});
-  UserEventServiceImpl service(sync_service(), MakeBridge());
-
-  // Only record events without navigation ids when history sync is off.
-  EXPECT_CALL(*mock_processor(), Put(_, _, _)).Times(0);
-  service.RecordUserEvent(WithNav(AsTest(Event())));
+  UserEventServiceImpl service(MakeBridge());
   EXPECT_CALL(*mock_processor(), Put(_, _, _));
   service.RecordUserEvent(AsTest(Event()));
 }
 
-TEST_F(UserEventServiceImplTest, ShouldNotRecordWhenPassphraseIsUsed) {
-  sync_service()->SetIsUsingSecondaryPassphrase(true);
-  UserEventServiceImpl service(sync_service(), MakeBridge());
-
-  // Do not record events when a passphrase is used.
-  EXPECT_CALL(*mock_processor(), Put(_, _, _)).Times(0);
-  service.RecordUserEvent(WithNav(AsTest(Event())));
-  service.RecordUserEvent(AsTest(Event()));
-}
-
-TEST_F(UserEventServiceImplTest, ShouldNotRecordWhenEngineIsNotInitialized) {
-  sync_service()->SetTransportState(
-      syncer::SyncService::TransportState::INITIALIZING);
-  UserEventServiceImpl service(sync_service(), MakeBridge());
+TEST_F(UserEventServiceImplTest, ShouldNotRecordWhenSyncIsNotStarted) {
+  ON_CALL(*mock_processor(), IsTrackingMetadata())
+      .WillByDefault(testing::Return(false));
+  UserEventServiceImpl service(MakeBridge());
 
   // Do not record events when the engine is off.
   EXPECT_CALL(*mock_processor(), Put(_, _, _)).Times(0);
@@ -135,7 +104,7 @@ TEST_F(UserEventServiceImplTest, ShouldNotRecordWhenEngineIsNotInitialized) {
 }
 
 TEST_F(UserEventServiceImplTest, ShouldNotRecordEmptyEvents) {
-  UserEventServiceImpl service(sync_service(), MakeBridge());
+  UserEventServiceImpl service(MakeBridge());
 
   // All untyped events should always be ignored.
   EXPECT_CALL(*mock_processor(), Put(_, _, _)).Times(0);
@@ -144,7 +113,7 @@ TEST_F(UserEventServiceImplTest, ShouldNotRecordEmptyEvents) {
 }
 
 TEST_F(UserEventServiceImplTest, ShouldRecordHasNavigationId) {
-  UserEventServiceImpl service(sync_service(), MakeBridge());
+  UserEventServiceImpl service(MakeBridge());
 
   // Verify logic for types that might or might not have a navigation id.
   EXPECT_CALL(*mock_processor(), Put(_, _, _));
@@ -175,24 +144,16 @@ TEST_F(UserEventServiceImplTest, SessionIdIsDifferent) {
             entity_data->specifics.user_event().session_id());
       });
 
-  UserEventServiceImpl service1(sync_service(), MakeBridge());
+  UserEventServiceImpl service1(MakeBridge());
   service1.RecordUserEvent(AsTest(Event()));
 
-  UserEventServiceImpl service2(sync_service(), MakeBridge());
+  UserEventServiceImpl service2(MakeBridge());
   service2.RecordUserEvent(AsTest(Event()));
 
   ASSERT_EQ(2U, put_session_ids.size());
   EXPECT_NE(put_session_ids[0], put_session_ids[1]);
 }
 
-TEST_F(UserEventServiceImplTest, ShouldNotRecordWhenEventsDatatypeIsDisabled) {
-  sync_service()->SetPreferredDataTypes({HISTORY_DELETE_DIRECTIVES});
-  UserEventServiceImpl service(sync_service(), MakeBridge());
-  // USER_EVENTS type is disabled, thus, they should not be recorded.
-  EXPECT_CALL(*mock_processor(), Put(_, _, _)).Times(0);
-  service.RecordUserEvent(AsTest(Event()));
-}
-
 }  // namespace
 
 }  // namespace syncer

ios/chrome/browser/sync/ios_user_event_service_factory.cc

@@ -21,7 +21,6 @@
 #include "ios/chrome/browser/browser_state/browser_state_otr_helper.h"
 #include "ios/chrome/browser/browser_state/chrome_browser_state.h"
 #include "ios/chrome/browser/sync/model_type_store_service_factory.h"
-#include "ios/chrome/browser/sync/profile_sync_service_factory.h"
 #include "ios/chrome/browser/sync/session_sync_service_factory.h"
 #include "ios/chrome/common/channel_info.h"
 #include "ios/web/public/browser_state.h"
@@ -52,16 +51,12 @@ IOSUserEventServiceFactory::~IOSUserEventServiceFactory() {}
 std::unique_ptr<KeyedService>
 IOSUserEventServiceFactory::BuildServiceInstanceFor(
     web::BrowserState* context) const {
-  ios::ChromeBrowserState* browser_state =
-      ios::ChromeBrowserState::FromBrowserState(context);
-
-  syncer::SyncService* sync_service =
-      ProfileSyncServiceFactory::GetForBrowserState(browser_state);
-  if (!syncer::UserEventServiceImpl::MightRecordEvents(
-          browser_state->IsOffTheRecord(), sync_service)) {
+  if (context->IsOffTheRecord()) {
     return std::make_unique<syncer::NoOpUserEventService>();
   }
 
+  ios::ChromeBrowserState* browser_state =
+      ios::ChromeBrowserState::FromBrowserState(context);
   syncer::OnceModelTypeStoreFactory store_factory =
       ModelTypeStoreServiceFactory::GetForBrowserState(browser_state)
           ->GetStoreFactory();
@@ -72,8 +67,7 @@ IOSUserEventServiceFactory::BuildServiceInstanceFor(
               &syncer::ReportUnrecoverableError, ::GetChannel())),
       SessionSyncServiceFactory::GetForBrowserState(browser_state)
           ->GetGlobalIdMapper());
-  return std::make_unique<syncer::UserEventServiceImpl>(sync_service,
-                                                        std::move(bridge));
+  return std::make_unique<syncer::UserEventServiceImpl>(std::move(bridge));
 }
 
 web::BrowserState* IOSUserEventServiceFactory::GetBrowserStateToUse(