[User events sync] Switch to forwarding controller delegate

For communication between the controller of a sync data type (running
always on the UI sequence) and the actual sync processor (running
potentially on backend sequences) we use proxy delegates that take care
of posting tasks across sequences. This is not needed for data types
that run on the UI sequence.

This CL makes use of the already existing
ForwardingModelTypeControllerDelegate for the USER_EVENTS sync data
type.

This has many caveats because the delegate gets fetched immediately
in some browser unit-tests and thus:
 - The FakeUserEventService has to provide a fake bridge and a fake
processor (that provides a null delegate). This in turn revealed a
previous BUILD.gn hack so the fake user event service needs to move
into a separate test_support build target.
 - on ios, the SessionSyncService has to provide a non-null
GlobalIdMapper to make the UserEventService constructor happy.

Bug: 867801
Change-Id: I47845732e9076e21544fba296323088750fff411
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1349690
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Commit-Queue: Gauthier Ambard <gambard@chromium.org>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Auto-Submit: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#649812}

chrome/test/BUILD.gn

@@ -3131,6 +3131,7 @@ test("unit_tests") {
     "//components/subresource_filter/core/browser:test_support",
     "//components/sync:test_support_driver",
     "//components/sync:test_support_model",
+    "//components/sync:test_support_user_events",
     "//components/sync_sessions:test_support",
     "//components/ukm/content",
     "//components/version_info:generate_version_info",

components/browser_sync/profile_sync_components_factory_impl.cc

@@ -402,12 +402,10 @@ ProfileSyncComponentsFactoryImpl::CreateCommonDataTypeControllers(
     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))));
+            std::make_unique<syncer::ForwardingModelTypeControllerDelegate>(
+                sync_client_
+                    ->GetControllerDelegateForModelType(syncer::USER_EVENTS)
+                    .get())));
   }
 
   if (!disabled_types.Has(syncer::SEND_TAB_TO_SELF) &&

components/sync/BUILD.gn

@@ -656,8 +656,6 @@ jumbo_static_library("js") {
 
 jumbo_static_library("user_events") {
   sources = [
-    "user_events/fake_user_event_service.cc",
-    "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",
@@ -681,6 +679,24 @@ jumbo_static_library("user_events") {
   ]
 }
 
+static_library("test_support_user_events") {
+  testonly = true
+  sources = [
+    "user_events/fake_user_event_service.cc",
+    "user_events/fake_user_event_service.h",
+  ]
+
+  public_deps = [
+    ":test_support_base",
+  ]
+
+  deps = [
+    ":sync",
+    ":test_support_model",
+    ":user_events",
+  ]
+}
+
 static_library("test_support_base") {
   testonly = true
   sources = [
@@ -1018,6 +1034,7 @@ source_set("unit_tests") {
     ":test_support_driver",
     ":test_support_engine",
     ":test_support_model",
+    ":test_support_user_events",
     ":user_events",
     "//base",
     "//base/test:test_support",

components/sync/model_impl/forwarding_model_type_controller_delegate.cc

@@ -11,7 +11,8 @@ namespace syncer {
 ForwardingModelTypeControllerDelegate::ForwardingModelTypeControllerDelegate(
     ModelTypeControllerDelegate* other)
     : other_(other) {
-  DCHECK(other_);
+  // TODO(crbug.com/895340): Put "DCHECK(other_);"" back once
+  // FakeUserEventService provides a proper non-null test double.
 }
 
 ForwardingModelTypeControllerDelegate::

components/sync/model_impl/forwarding_model_type_controller_delegate.h

@@ -18,7 +18,7 @@ namespace syncer {
 class ForwardingModelTypeControllerDelegate
     : public ModelTypeControllerDelegate {
  public:
-  // |other| must not be null and must outlive this object.
+  // Except for tests, |other| must not be null and must outlive this object.
   explicit ForwardingModelTypeControllerDelegate(
       ModelTypeControllerDelegate* other);
   ~ForwardingModelTypeControllerDelegate() override;

components/sync/user_events/fake_user_event_service.cc

@@ -3,12 +3,14 @@
 // found in the LICENSE file.
 
 #include "components/sync/user_events/fake_user_event_service.h"
+#include "components/sync/model/fake_model_type_change_processor.h"
 
 using sync_pb::UserEventSpecifics;
 
 namespace syncer {
 
-FakeUserEventService::FakeUserEventService() {}
+FakeUserEventService::FakeUserEventService()
+    : fake_bridge_(std::make_unique<FakeModelTypeChangeProcessor>()) {}
 
 FakeUserEventService::~FakeUserEventService() {}
 
@@ -24,7 +26,7 @@ void FakeUserEventService::RecordUserEvent(
 }
 
 ModelTypeSyncBridge* FakeUserEventService::GetSyncBridge() {
-  return nullptr;
+  return &fake_bridge_;
 }
 
 const std::vector<UserEventSpecifics>&

components/sync/user_events/fake_user_event_service.h

@@ -10,13 +10,12 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "components/sync/model/fake_model_type_sync_bridge.h"
 #include "components/sync/protocol/user_event_specifics.pb.h"
 #include "components/sync/user_events/user_event_service.h"
 
 namespace syncer {
 
-class ModelTypeSyncBridge;
-
 // This implementation is intended to be used in unit tests, with public
 // accessors that allow reading all data to verify expectations.
 class FakeUserEventService : public UserEventService {
@@ -28,12 +27,18 @@ class FakeUserEventService : public UserEventService {
   void RecordUserEvent(
       std::unique_ptr<sync_pb::UserEventSpecifics> specifics) override;
   void RecordUserEvent(const sync_pb::UserEventSpecifics& specifics) override;
+  // TODO(crbug.com/895340): This is hard to mock, replace it (in the base
+  // class) by GetControllerDelegate(), then we can get rid of |fake_bridge_|.
+  // Maybe we can also expose a raw pointer to be consumed by
+  // ForwardingModelTypeControllerDelegate and not care about WeakPtrs anymore
+  // (but we need a nice solution for SyncClient).
   ModelTypeSyncBridge* GetSyncBridge() override;
 
   const std::vector<sync_pb::UserEventSpecifics>& GetRecordedUserEvents() const;
 
  private:
   std::vector<sync_pb::UserEventSpecifics> recorded_user_events_;
+  FakeModelTypeSyncBridge fake_bridge_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeUserEventService);
 };

ios/chrome/browser/sync/profile_sync_service_factory.cc

@@ -40,6 +40,7 @@
 #include "ios/chrome/browser/sync/consent_auditor_factory.h"
 #include "ios/chrome/browser/sync/device_info_sync_service_factory.h"
 #include "ios/chrome/browser/sync/ios_chrome_sync_client.h"
+#include "ios/chrome/browser/sync/ios_user_event_service_factory.h"
 #include "ios/chrome/browser/sync/model_type_store_service_factory.h"
 #include "ios/chrome/browser/sync/session_sync_service_factory.h"
 #include "ios/chrome/browser/undo/bookmark_undo_service_factory.h"
@@ -136,6 +137,7 @@ ProfileSyncServiceFactory::ProfileSyncServiceFactory()
   DependsOn(IOSChromeProfileInvalidationProviderFactory::GetInstance());
   DependsOn(
       IOSChromeDeprecatedProfileInvalidationProviderFactory::GetInstance());
+  DependsOn(IOSUserEventServiceFactory::GetInstance());
   DependsOn(ModelTypeStoreServiceFactory::GetInstance());
   DependsOn(ReadingListModelFactory::GetInstance());
   DependsOn(SessionSyncServiceFactory::GetInstance());

ios/chrome/browser/ui/recent_tabs/recent_tabs_coordinator_unittest.mm

@@ -11,6 +11,7 @@
 #include "base/bind.h"
 #include "components/sync/driver/sync_service.h"
 #include "components/sync/model/fake_model_type_controller_delegate.h"
+#include "components/sync/user_events/global_id_mapper.h"
 #include "components/sync_sessions/open_tabs_ui_delegate.h"
 #include "components/sync_sessions/session_sync_service.h"
 #include "ios/chrome/browser/browser_state/test_chrome_browser_state.h"
@@ -102,6 +103,16 @@ class OpenTabsUIDelegateMock : public sync_sessions::OpenTabsUIDelegate {
                bool(const sync_sessions::SyncedSession** local));
 };
 
+class GlobalIdMapperMock : public syncer::GlobalIdMapper {
+ public:
+  GlobalIdMapperMock() {}
+  ~GlobalIdMapperMock() {}
+
+  MOCK_METHOD1(AddGlobalIdChangeObserver,
+               void(syncer::GlobalIdChange callback));
+  MOCK_METHOD1(GetLatestGlobalId, int64_t(int64_t global_id));
+};
+
 class RecentTabsTableCoordinatorTest : public BlockCleanupTest {
  public:
   RecentTabsTableCoordinatorTest()
@@ -157,6 +168,8 @@ class RecentTabsTableCoordinatorTest : public BlockCleanupTest {
     // initialization of SyncSetupServiceMock.
     ON_CALL(*session_sync_service, GetControllerDelegate())
         .WillByDefault(Return(fake_controller_delegate_.GetWeakPtr()));
+    ON_CALL(*session_sync_service, GetGlobalIdMapper())
+        .WillByDefault(Return(&global_id_mapper_));
 
     SyncSetupServiceMock* syncSetupService = static_cast<SyncSetupServiceMock*>(
         SyncSetupServiceFactory::GetForBrowserState(
@@ -191,6 +204,7 @@ class RecentTabsTableCoordinatorTest : public BlockCleanupTest {
 
   syncer::FakeModelTypeControllerDelegate fake_controller_delegate_;
   testing::NiceMock<OpenTabsUIDelegateMock> open_tabs_ui_delegate_;
+  testing::NiceMock<GlobalIdMapperMock> global_id_mapper_;
   std::unique_ptr<TestChromeBrowserState> chrome_browser_state_;
 
   // Must be declared *after* |chrome_browser_state_| so it can outlive it.