[USS] Uncouple ModelTypeControllerDelegate from bridges - part 1

This CL unblocks
https://chromium-review.googlesource.com/c/chromium/src/+/102589 that
separates the controller delegate interface from the bridges and makes
ClientTagBasedModelTypeProcessor directly implement the interface.

Since the processor needs to be able to inform the bridge that sync
is starting (before the bridge is ready to sync, i.e. before it gets
the pointer to the bridge), this CL passes the (preliminary) bridge
pointer to the processor during construction of the bridge.

This CL is admittedly hacky, it only minimizes the CL size and thus
the speed for landing the blocked CL. After the CL gets landed, we
need to remove this hack and introduce a proper solution, probably by
swapping the ownership of the bridge and the processor.

Bug: 819993
Change-Id: Ie1950e6626a3ab58a583d8fbe502ba52c34319e0
Reviewed-on: https://chromium-review.googlesource.com/1042707
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556793}

components/sync/model/fake_model_type_change_processor.cc

@@ -44,6 +44,9 @@ void FakeModelTypeChangeProcessor::UpdateStorageKey(
 void FakeModelTypeChangeProcessor::UntrackEntity(
     const EntityData& entity_data) {}
 
+void FakeModelTypeChangeProcessor::OnModelStarting(
+    ModelTypeSyncBridge* bridge) {}
+
 void FakeModelTypeChangeProcessor::ModelReadyToSync(
     ModelTypeSyncBridge* bridge,
     std::unique_ptr<MetadataBatch> batch) {}

components/sync/model/fake_model_type_change_processor.h

@@ -35,6 +35,7 @@ class FakeModelTypeChangeProcessor : public ModelTypeChangeProcessor {
                         const std::string& storage_key,
                         MetadataChangeList* metadata_change_list) override;
   void UntrackEntity(const EntityData& entity_data) override;
+  void OnModelStarting(ModelTypeSyncBridge* bridge) override;
   void ModelReadyToSync(ModelTypeSyncBridge* bridge,
                         std::unique_ptr<MetadataBatch> batch) override;
   void OnSyncStarting(const ModelErrorHandler& error_handler,

components/sync/model/mock_model_type_change_processor.cc

@@ -44,6 +44,10 @@ class ForwardingModelTypeChangeProcessor : public ModelTypeChangeProcessor {
     other_->UntrackEntity(entity_data);
   }
 
+  void OnModelStarting(ModelTypeSyncBridge* bridge) override {
+    other_->OnModelStarting(bridge);
+  }
+
   void ModelReadyToSync(ModelTypeSyncBridge* bridge,
                         std::unique_ptr<MetadataBatch> batch) override {
     other_->ModelReadyToSync(bridge, std::move(batch));
@@ -117,6 +121,9 @@ void MockModelTypeChangeProcessor::DelegateCallsByDefaultTo(
   ON_CALL(*this, UntrackEntity(_))
       .WillByDefault(
           Invoke(delegate, &ModelTypeChangeProcessor::UntrackEntity));
+  ON_CALL(*this, OnModelStarting(_))
+      .WillByDefault(
+          Invoke(delegate, &ModelTypeChangeProcessor::OnModelStarting));
   ON_CALL(*this, DoModelReadyToSync(_, _))
       .WillByDefault(
           Invoke([delegate](ModelTypeSyncBridge* bridge, MetadataBatch* batch) {

components/sync/model/mock_model_type_change_processor.h

@@ -36,6 +36,7 @@ class MockModelTypeChangeProcessor : public ModelTypeChangeProcessor {
                     const std::string& storage_key,
                     MetadataChangeList* metadata_change_list));
   MOCK_METHOD1(UntrackEntity, void(const EntityData& entity_data));
+  MOCK_METHOD1(OnModelStarting, void(ModelTypeSyncBridge* bridge));
   // TODO(crbug.com/729950): Use unique_ptr here directly once move-only
   // arguments are supported in gMock.
   MOCK_METHOD2(DoModelReadyToSync,

components/sync/model/model_type_change_processor.h

@@ -60,6 +60,14 @@ class ModelTypeChangeProcessor {
   // change_processor()->Delete() instead.
   virtual void UntrackEntity(const EntityData& entity_data) = 0;
 
+  // Pass the pointer to the processor so that the processor can notify the
+  // bridge that sync is starting.
+  // This is a intermediate hack needed for fixing https://crbug.com/819993.
+  // TODO(jkrcal): Remove this hack (either by removing the bridge pointer from
+  // ModelReadyToSync or, preferrably, by swapping the ownership of the bridge
+  // and the processor).
+  virtual void OnModelStarting(ModelTypeSyncBridge* bridge) = 0;
+
   // The |bridge| is expected to call this exactly once unless it encounters an
   // error. Ideally ModelReadyToSync() is called as soon as possible during
   // initialization, and must be called before invoking either Put() or

components/sync/model/model_type_sync_bridge.cc

@@ -15,6 +15,7 @@ ModelTypeSyncBridge::ModelTypeSyncBridge(
     std::unique_ptr<ModelTypeChangeProcessor> change_processor)
     : change_processor_(std::move(change_processor)) {
   DCHECK(change_processor_);
+  change_processor_->OnModelStarting(this);
 }
 
 ModelTypeSyncBridge::~ModelTypeSyncBridge() {}

components/sync/model_impl/client_tag_based_model_type_processor.cc

@@ -81,14 +81,22 @@ void ClientTagBasedModelTypeProcessor::OnSyncStarting(
   ConnectIfReady();
 }
 
+void ClientTagBasedModelTypeProcessor::OnModelStarting(
+    ModelTypeSyncBridge* bridge) {
+  DCHECK(bridge);
+  bridge_ = bridge;
+}
+
 void ClientTagBasedModelTypeProcessor::ModelReadyToSync(
     ModelTypeSyncBridge* bridge,
     std::unique_ptr<MetadataBatch> batch) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(entities_.empty());
-  DCHECK(bridge);
+  DCHECK(!model_ready_to_sync_);
+  // The |bridge| argument is ignored here because it is already stored as
+  // |bridge_| by OnModelStarting().
 
-  bridge_ = bridge;
+  model_ready_to_sync_ = true;
 
   // The model already experienced an error; abort;
   if (model_error_)
@@ -119,7 +127,13 @@ void ClientTagBasedModelTypeProcessor::ModelReadyToSync(
 }
 
 bool ClientTagBasedModelTypeProcessor::IsModelReadyOrError() const {
-  return model_error_ || bridge_;
+  return model_error_ || model_ready_to_sync_;
+}
+
+bool ClientTagBasedModelTypeProcessor::IsAllowingChanges() const {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  // Changes can be handled correctly even before pending data is loaded.
+  return model_ready_to_sync_;
 }
 
 void ClientTagBasedModelTypeProcessor::ConnectIfReady() {
@@ -141,12 +155,6 @@ void ClientTagBasedModelTypeProcessor::ConnectIfReady() {
   start_callback_.Reset();
 }
 
-bool ClientTagBasedModelTypeProcessor::IsAllowingChanges() const {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  // Changes can be handled correctly even before pending data is loaded.
-  return bridge_ != nullptr;
-}
-
 bool ClientTagBasedModelTypeProcessor::IsConnected() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return !!worker_;
@@ -155,7 +163,7 @@ bool ClientTagBasedModelTypeProcessor::IsConnected() const {
 void ClientTagBasedModelTypeProcessor::DisableSync() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   // Disabling sync for a type never happens before the model is ready to sync.
-  DCHECK(bridge_);
+  DCHECK(model_ready_to_sync_);
 
   std::unique_ptr<MetadataChangeList> change_list =
       bridge_->CreateMetadataChangeList();
@@ -179,7 +187,7 @@ void ClientTagBasedModelTypeProcessor::DisableSync() {
     case ModelTypeSyncBridge::DisableSyncResponse::kModelNoLongerReadyToSync:
       // Model not ready to sync, so wait until the bridge calls
       // ModelReadyToSync().
-      bridge_ = nullptr;
+      model_ready_to_sync_ = false;
       break;
   }
 }
@@ -990,10 +998,10 @@ void ClientTagBasedModelTypeProcessor::RemoveEntity(
 }
 
 void ClientTagBasedModelTypeProcessor::ResetState() {
-  // This should reset all mutable fields (except for |bridge_| while is null
-  // while the bridge is not ready to sync).
+  // This should reset all mutable fields (except for |bridge_|).
   worker_.reset();
   model_error_.reset();
+  model_ready_to_sync_ = false;
   entities_.clear();
   storage_key_to_tag_hash_.clear();
   model_type_state_ = sync_pb::ModelTypeState();

components/sync/model_impl/client_tag_based_model_type_processor.h

@@ -48,10 +48,6 @@ class ClientTagBasedModelTypeProcessor : public ModelTypeProcessor,
                                    bool commit_only);
   ~ClientTagBasedModelTypeProcessor() override;
 
-  // Whether the processor is allowing changes to its model type. If this is
-  // false, the bridge should not allow any changes to its data.
-  bool IsAllowingChanges() const;
-
   // Returns true if the handshake with sync thread is complete.
   bool IsConnected() const;
 
@@ -65,6 +61,7 @@ class ClientTagBasedModelTypeProcessor : public ModelTypeProcessor,
                         const std::string& storage_key,
                         MetadataChangeList* metadata_change_list) override;
   void UntrackEntity(const EntityData& entity_data) override;
+  void OnModelStarting(ModelTypeSyncBridge* bridge) override;
   void ModelReadyToSync(ModelTypeSyncBridge* bridge,
                         std::unique_ptr<MetadataBatch> batch) override;
   void OnSyncStarting(const ModelErrorHandler& error_handler,
@@ -95,6 +92,10 @@ class ClientTagBasedModelTypeProcessor : public ModelTypeProcessor,
   // Returns true if the model is ready or encountered an error.
   bool IsModelReadyOrError() const;
 
+  // Whether the processor is allowing changes to its model type. If this is
+  // false, the bridge should not allow any changes to its data.
+  bool IsAllowingChanges() const;
+
   // If preconditions are met, inform sync that we are ready to connect.
   void ConnectIfReady();
 
@@ -225,6 +226,10 @@ class ClientTagBasedModelTypeProcessor : public ModelTypeProcessor,
   // and so it can be passed to sync if it happened prior to sync being ready.
   base::Optional<ModelError> model_error_;
 
+  // Whether the model has initialized its internal state for sync (and provided
+  // metadata).
+  bool model_ready_to_sync_ = false;
+
   ////////////////
   // Sync state //
   ////////////////

components/sync_sessions/session_sync_bridge_unittest.cc

@@ -184,6 +184,11 @@ class SessionSyncBridgeTest : public ::testing::Test {
   ~SessionSyncBridgeTest() override {}
 
   void InitializeBridge() {
+    real_processor_ =
+        std::make_unique<syncer::ClientTagBasedModelTypeProcessor>(
+            syncer::SESSIONS, /*dump_stack=*/base::DoNothing(),
+            /*commit_only=*/false);
+    mock_processor_.DelegateCallsByDefaultTo(real_processor_.get());
     // Instantiate the bridge.
     bridge_ = std::make_unique<SessionSyncBridge>(
         &mock_sync_sessions_client_, &mock_sync_prefs_,
@@ -192,11 +197,6 @@ class SessionSyncBridgeTest : public ::testing::Test {
         syncer::ModelTypeStoreTestUtil::FactoryForForwardingStore(store_.get()),
         mock_foreign_sessions_updated_callback_.Get(),
         mock_processor_.CreateForwardingProcessor());
-    real_processor_ =
-        std::make_unique<syncer::ClientTagBasedModelTypeProcessor>(
-            syncer::SESSIONS, /*dump_stack=*/base::DoNothing(),
-            /*commit_only=*/false);
-    mock_processor_.DelegateCallsByDefaultTo(real_processor_.get());
   }
 
   void ShutdownBridge() {