USS: Introduce dependency of SharedModelTypeProcessor on ModelTypeService

BUG=561821

Review URL: https://codereview.chromium.org/1486363002

Cr-Commit-Position: refs/heads/master@{#363066}

components/sync_driver/non_blocking_data_type_controller_unittest.cc

@@ -19,6 +19,7 @@
 #include "sync/engine/commit_queue.h"
 #include "sync/internal_api/public/activation_context.h"
 #include "sync/internal_api/public/shared_model_type_processor.h"
+#include "sync/internal_api/public/test/fake_model_type_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace sync_driver_v2 {
@@ -187,9 +188,9 @@ class NonBlockingDataTypeControllerTest : public testing::Test,
 
  protected:
   void CreateTypeProcessor() {
-    // TODO(pavely): stop passing ModelTypeStore.
-    type_processor_.reset(new syncer_v2::SharedModelTypeProcessor(
-        syncer::DICTIONARY, base::WeakPtr<syncer_v2::ModelTypeStore>()));
+    // TODO(stanisc): Controller should discover the service via SyncClient.
+    type_processor_.reset(
+        new syncer_v2::SharedModelTypeProcessor(syncer::DICTIONARY, &service_));
     type_processor_for_ui_ = type_processor_->AsWeakPtrForUI();
   }
 
@@ -330,6 +331,7 @@ class NonBlockingDataTypeControllerTest : public testing::Test,
   scoped_refptr<base::TestSimpleTaskRunner> sync_thread_runner_;
   MockSyncBackend backend_;
   MockBackendDataTypeConfigurer configurer_;
+  syncer_v2::FakeModelTypeService service_;
 };
 
 TEST_F(NonBlockingDataTypeControllerTest, InitialState) {

sync/BUILD.gn

@@ -554,12 +554,14 @@ static_library("test_support_sync_core") {
 static_library("test_support_sync_internal_api") {
   testonly = true
   sources = [
+    "internal_api/public/test/fake_model_type_service.h",
     "internal_api/public/test/fake_sync_manager.h",
     "internal_api/public/test/null_sync_context_proxy.h",
     "internal_api/public/test/sync_manager_factory_for_profile_sync_test.h",
     "internal_api/public/test/test_entry_factory.h",
     "internal_api/public/test/test_internal_components_factory.h",
     "internal_api/public/test/test_user_share.h",
+    "internal_api/test/fake_model_type_service.cc",
     "internal_api/test/fake_sync_manager.cc",
     "internal_api/test/null_sync_context_proxy.cc",
     "internal_api/test/sync_manager_factory_for_profile_sync_test.cc",

sync/api/model_type_service.h

@@ -13,12 +13,9 @@
 #include "sync/api/entity_change.h"
 #include "sync/api/entity_data.h"
 #include "sync/api/model_type_change_processor.h"
+#include "sync/api/sync_error.h"
 #include "sync/base/sync_export.h"
 
-namespace syncer {
-class SyncError;
-}  // namespace syncer
-
 namespace syncer_v2 {
 
 class DataBatch;

sync/internal_api/public/shared_model_type_processor.h

@@ -12,6 +12,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/threading/non_thread_safe.h"
 #include "sync/api/model_type_change_processor.h"
+#include "sync/api/model_type_service.h"
 #include "sync/api/sync_error.h"
 #include "sync/base/sync_export.h"
 #include "sync/internal_api/public/base/model_type.h"
@@ -23,7 +24,6 @@ namespace syncer_v2 {
 struct ActivationContext;
 class CommitQueue;
 class ModelTypeEntity;
-class ModelTypeStore;
 
 // A sync component embedded on the synced type's thread that helps to handle
 // communication between sync and model type threads.
@@ -32,8 +32,7 @@ class SYNC_EXPORT_PRIVATE SharedModelTypeProcessor
       public ModelTypeChangeProcessor,
       base::NonThreadSafe {
  public:
-  SharedModelTypeProcessor(syncer::ModelType type,
-                           base::WeakPtr<ModelTypeStore> store);
+  SharedModelTypeProcessor(syncer::ModelType type, ModelTypeService* service);
   ~SharedModelTypeProcessor() override;
 
   typedef base::Callback<void(syncer::SyncError, scoped_ptr<ActivationContext>)>
@@ -134,10 +133,10 @@ class SYNC_EXPORT_PRIVATE SharedModelTypeProcessor
   // them across restarts, and keep them in sync with our progress markers.
   UpdateMap pending_updates_map_;
 
-  // Store is supplied by model type implementation. SharedModelTypeProcessor
-  // uses store for persisting sync related data (entity state and data type
-  // state).
-  base::WeakPtr<ModelTypeStore> store_;
+  // ModelTypeService linked to this processor.
+  // The service owns this processor instance so the pointer should never
+  // become invalid.
+  ModelTypeService* const service_;
 
   // We use two different WeakPtrFactories because we want the pointers they
   // issue to have different lifetimes.  When asked to disconnect from the sync

sync/internal_api/public/test/fake_model_type_service.h

+// Copyright 2015 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 SYNC_INTERNAL_API_PUBLIC_TEST_FAKE_MODEL_TYPE_SERVICE_H_
+#define SYNC_INTERNAL_API_PUBLIC_TEST_FAKE_MODEL_TYPE_SERVICE_H_
+
+#include "sync/api/data_batch.h"
+#include "sync/api/metadata_batch.h"
+#include "sync/api/metadata_change_list.h"
+#include "sync/api/model_type_service.h"
+
+namespace syncer_v2 {
+
+// A non-functional implementation of ModelTypeService for
+// testing purposes.
+class FakeModelTypeService : public ModelTypeService {
+ public:
+  FakeModelTypeService();
+  ~FakeModelTypeService() override;
+
+  scoped_ptr<MetadataChangeList> CreateMetadataChangeList() override;
+
+  syncer::SyncError MergeSyncData(
+      scoped_ptr<MetadataChangeList> metadata_change_list,
+      EntityDataList entity_data_list) override;
+
+  syncer::SyncError ApplySyncChanges(
+      scoped_ptr<MetadataChangeList> metadata_change_list,
+      EntityChangeList entity_changes) override;
+
+  void LoadMetadata(MetadataCallback callback) override;
+
+  void GetData(ClientKeyList client_keys, DataCallback callback) override;
+
+  void GetAllData(DataCallback callback) override;
+
+  std::string GetClientTag(const EntityData& entity_data) override;
+};
+
+}  // namespace syncer_v2
+
+#endif  // SYNC_INTERNAL_API_PUBLIC_TEST_FAKE_MODEL_TYPE_SERVICE_H_

sync/internal_api/shared_model_type_processor.cc

@@ -69,15 +69,16 @@ void ModelTypeProcessorProxy::OnUpdateReceived(
 
 }  // namespace
 
-SharedModelTypeProcessor::SharedModelTypeProcessor(
-    syncer::ModelType type,
-    base::WeakPtr<ModelTypeStore> store)
+SharedModelTypeProcessor::SharedModelTypeProcessor(syncer::ModelType type,
+                                                   ModelTypeService* service)
     : type_(type),
       is_enabled_(false),
       is_connected_(false),
-      store_(store),
+      service_(service),
       weak_ptr_factory_for_ui_(this),
-      weak_ptr_factory_for_sync_(this) {}
+      weak_ptr_factory_for_sync_(this) {
+  DCHECK(service);
+}
 
 SharedModelTypeProcessor::~SharedModelTypeProcessor() {}
 
@@ -272,8 +273,8 @@ void SharedModelTypeProcessor::OnUpdateReceived(
     ModelTypeEntity* entity = nullptr;
     EntityMap::const_iterator it = entities_.find(client_tag_hash);
     if (it == entities_.end()) {
-      // TODO(stanisc): crbug/561821: Get client_tag from the service.
-      std::string client_tag = client_tag_hash;
+      // Let the service define |client_tag| based on the entity data.
+      std::string client_tag = service_->GetClientTag(data);
 
       scoped_ptr<ModelTypeEntity> scoped_entity = ModelTypeEntity::CreateNew(
           client_tag, client_tag_hash, data.id, data.creation_time);

sync/internal_api/shared_model_type_processor_unittest.cc

@@ -11,6 +11,7 @@
 #include "sync/internal_api/public/activation_context.h"
 #include "sync/internal_api/public/base/model_type.h"
 #include "sync/internal_api/public/non_blocking_sync_common.h"
+#include "sync/internal_api/public/test/fake_model_type_service.h"
 #include "sync/protocol/sync.pb.h"
 #include "sync/syncable/syncable_util.h"
 #include "sync/test/engine/mock_commit_queue.h"
@@ -41,7 +42,8 @@ static const syncer::ModelType kModelType = syncer::PREFERENCES;
 // - Writes to permanent storage. (TODO)
 // - Callbacks into the model. (TODO)
 // - Requests to the sync thread.  Tested with MockCommitQueue.
-class SharedModelTypeProcessorTest : public ::testing::Test {
+class SharedModelTypeProcessorTest : public ::testing::Test,
+                                     public FakeModelTypeService {
  public:
   SharedModelTypeProcessorTest();
   ~SharedModelTypeProcessorTest() override;
@@ -129,6 +131,9 @@ class SharedModelTypeProcessorTest : public ::testing::Test {
   void StartDone(syncer::SyncError error,
                  scoped_ptr<ActivationContext> context);
 
+  // FakeModelTypeService overrides.
+  std::string GetClientTag(const EntityData& entity_data) override;
+
   // This sets ThreadTaskRunnerHandle on the current thread, which the type
   // processor will pick up as the sync task runner.
   base::MessageLoop sync_loop_;
@@ -145,9 +150,7 @@ class SharedModelTypeProcessorTest : public ::testing::Test {
 SharedModelTypeProcessorTest::SharedModelTypeProcessorTest()
     : mock_queue_(new MockCommitQueue()),
       mock_queue_ptr_(mock_queue_),
-      type_processor_(
-          new SharedModelTypeProcessor(kModelType,
-                                       base::WeakPtr<ModelTypeStore>())) {}
+      type_processor_(new SharedModelTypeProcessor(kModelType, this)) {}
 
 SharedModelTypeProcessorTest::~SharedModelTypeProcessorTest() {}
 
@@ -338,6 +341,12 @@ SharedModelTypeProcessorTest::GenerateEncryptedSpecifics(
   return specifics;
 }
 
+std::string SharedModelTypeProcessorTest::GetClientTag(
+    const EntityData& entity_data) {
+  // The tag is the preference name - see GenerateSpecifics.
+  return entity_data.specifics.preference().name();
+}
+
 size_t SharedModelTypeProcessorTest::GetNumCommitRequestLists() {
   return mock_queue_->GetNumCommitRequestLists();
 }

sync/internal_api/sync_context_proxy_impl_unittest.cc

@@ -11,6 +11,7 @@
 #include "sync/internal_api/public/base/model_type.h"
 #include "sync/internal_api/public/shared_model_type_processor.h"
 #include "sync/internal_api/public/sync_context.h"
+#include "sync/internal_api/public/test/fake_model_type_service.h"
 #include "sync/internal_api/sync_context_proxy_impl.h"
 #include "sync/sessions/model_type_registry.h"
 #include "sync/test/engine/mock_nudge_handler.h"
@@ -19,7 +20,7 @@
 
 namespace syncer_v2 {
 
-class SyncContextProxyImplTest : public ::testing::Test {
+class SyncContextProxyImplTest : public ::testing::Test, FakeModelTypeService {
  public:
   SyncContextProxyImplTest()
       : sync_task_runner_(base::ThreadTaskRunnerHandle::Get()),
@@ -54,8 +55,7 @@ class SyncContextProxyImplTest : public ::testing::Test {
   }
 
   scoped_ptr<SharedModelTypeProcessor> CreateModelTypeProcessor() {
-    return make_scoped_ptr(new SharedModelTypeProcessor(
-        syncer::THEMES, base::WeakPtr<ModelTypeStore>()));
+    return make_scoped_ptr(new SharedModelTypeProcessor(syncer::THEMES, this));
   }
 
  private:

sync/internal_api/test/fake_model_type_service.cc

+// Copyright 2015 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 "sync/internal_api/public/test/fake_model_type_service.h"
+
+namespace syncer_v2 {
+
+FakeModelTypeService::FakeModelTypeService() {}
+
+FakeModelTypeService::~FakeModelTypeService() {}
+
+scoped_ptr<MetadataChangeList>
+FakeModelTypeService::CreateMetadataChangeList() {
+  return scoped_ptr<MetadataChangeList>();
+}
+
+syncer::SyncError FakeModelTypeService::MergeSyncData(
+    scoped_ptr<MetadataChangeList> metadata_change_list,
+    EntityDataList entity_data_list) {
+  return syncer::SyncError();
+}
+
+syncer::SyncError FakeModelTypeService::ApplySyncChanges(
+    scoped_ptr<MetadataChangeList> metadata_change_list,
+    EntityChangeList entity_changes) {
+  return syncer::SyncError();
+}
+
+void FakeModelTypeService::LoadMetadata(MetadataCallback callback) {}
+
+void FakeModelTypeService::GetData(ClientKeyList client_keys,
+                                   DataCallback callback) {}
+
+void FakeModelTypeService::GetAllData(DataCallback callback) {}
+
+std::string FakeModelTypeService::GetClientTag(const EntityData& entity_data) {
+  return std::string();
+}
+
+}  // namespace syncer_v2

sync/sessions/model_type_registry_unittest.cc

@@ -9,6 +9,7 @@
 #include "sync/internal_api/public/activation_context.h"
 #include "sync/internal_api/public/base/model_type.h"
 #include "sync/internal_api/public/shared_model_type_processor.h"
+#include "sync/internal_api/public/test/fake_model_type_service.h"
 #include "sync/sessions/model_type_registry.h"
 #include "sync/test/engine/fake_model_worker.h"
 #include "sync/test/engine/mock_nudge_handler.h"
@@ -17,17 +18,8 @@
 
 namespace syncer {
 
-namespace {
-
-scoped_ptr<syncer_v2::SharedModelTypeProcessor> MakeModelTypeProcessor(
-    ModelType type) {
-  return make_scoped_ptr(new syncer_v2::SharedModelTypeProcessor(
-      type, base::WeakPtr<syncer_v2::ModelTypeStore>()));
-}
-
-}  // namespace
-
-class ModelTypeRegistryTest : public ::testing::Test {
+class ModelTypeRegistryTest : public ::testing::Test,
+                              syncer_v2::FakeModelTypeService {
  public:
   ModelTypeRegistryTest();
   void SetUp() override;
@@ -54,6 +46,12 @@ class ModelTypeRegistryTest : public ::testing::Test {
     return context.Pass();
   }
 
+ protected:
+  scoped_ptr<syncer_v2::SharedModelTypeProcessor> MakeModelTypeProcessor(
+      ModelType type) {
+    return make_scoped_ptr(new syncer_v2::SharedModelTypeProcessor(type, this));
+  }
+
  private:
   syncable::Directory* directory();
 

sync/sync_tests.gypi

@@ -183,12 +183,14 @@
         'test_support_sync_core',
       ],
       'sources': [
+        'internal_api/public/test/fake_model_type_service.h',
         'internal_api/public/test/fake_sync_manager.h',
         'internal_api/public/test/null_sync_context_proxy.h',
         'internal_api/public/test/sync_manager_factory_for_profile_sync_test.h',
         'internal_api/public/test/test_entry_factory.h',
         'internal_api/public/test/test_internal_components_factory.h',
         'internal_api/public/test/test_user_share.h',
+        'internal_api/test/fake_model_type_service.cc',
         'internal_api/test/fake_sync_manager.cc',
         'internal_api/test/null_sync_context_proxy.cc',
         'internal_api/test/sync_manager_factory_for_profile_sync_test.cc',