Let ModelTypeService create SharedModelTypeProcessor instead

of ModelTypeController.
Only change the place where to create, still need to figure
out where to trigger.

BUG=547087

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

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

8 files changed, 72 insertions, 62 deletions

diff --git a/components/sync_driver/device_info_model_type_controller.cc b/components/sync_driver/device_info_model_type_controller.cc
index 0e6a6c2..cdd0863 100644
--- a/components/sync_driver/device_info_model_type_controller.cc
+++ b/components/sync_driver/device_info_model_type_controller.cc
@@ -23,8 +23,6 @@ DeviceInfoModelTypeController::DeviceInfoModelTypeController(
                             syncer::DEVICE_INFO,
                             sync_client),
       local_device_info_provider_(local_device_info_provider) {
-  // TODO(gangwu): find a better place to initial processor.
-  InitializeProcessor();
 }
 
 DeviceInfoModelTypeController::~DeviceInfoModelTypeController() {}
diff --git a/components/sync_driver/device_info_service.cc b/components/sync_driver/device_info_service.cc
index 6a63dcb..84cd707 100644
--- a/components/sync_driver/device_info_service.cc
+++ b/components/sync_driver/device_info_service.cc
@@ -42,8 +42,10 @@ using WriteBatch = ModelTypeStore::WriteBatch;
 
 DeviceInfoService::DeviceInfoService(
     sync_driver::LocalDeviceInfoProvider* local_device_info_provider,
-    const StoreFactoryFunction& callback)
-    : local_device_info_provider_(local_device_info_provider),
+    const StoreFactoryFunction& callback,
+    const ChangeProcessorFactory& change_processor_factory)
+    : ModelTypeService(change_processor_factory, syncer::DEVICE_INFO),
+      local_device_info_provider_(local_device_info_provider),
       weak_factory_(this) {
   DCHECK(local_device_info_provider);
 
@@ -370,6 +372,9 @@ void DeviceInfoService::OnReadAllMetadata(
     Result result,
     scoped_ptr<RecordList> metadata_records,
     const std::string& global_metadata) {
+  if (metadata_records->size() > 0 || !global_metadata.empty()) {
+    GetOrCreateChangeProcessor();
+  }
   if (!change_processor()) {
     // This datatype was disabled while this read was oustanding.
     return;
@@ -421,7 +426,8 @@ void DeviceInfoService::TryReconcileLocalAndStored() {
 }
 
 void DeviceInfoService::TryLoadAllMetadata() {
-  if (has_data_loaded_ && change_processor()) {
+  // TODO(skym): Fix the double load metadata issue.
+  if (has_data_loaded_) {
     store_->ReadAllMetadata(base::Bind(&DeviceInfoService::OnReadAllMetadata,
                                        weak_factory_.GetWeakPtr()));
   }
diff --git a/components/sync_driver/device_info_service.h b/components/sync_driver/device_info_service.h
index 4ffc466..bc32e17 100644
--- a/components/sync_driver/device_info_service.h
+++ b/components/sync_driver/device_info_service.h
@@ -46,7 +46,8 @@ class DeviceInfoService : public syncer_v2::ModelTypeService,
 
   DeviceInfoService(
       sync_driver::LocalDeviceInfoProvider* local_device_info_provider,
-      const StoreFactoryFunction& callback);
+      const StoreFactoryFunction& callback,
+      const ChangeProcessorFactory& change_processor_factory);
   ~DeviceInfoService() override;
 
   // ModelTypeService implementation.
diff --git a/components/sync_driver/device_info_service_unittest.cc b/components/sync_driver/device_info_service_unittest.cc
index 4cd69c6..f15aa13 100644
--- a/components/sync_driver/device_info_service_unittest.cc
+++ b/components/sync_driver/device_info_service_unittest.cc
@@ -135,6 +135,8 @@ class FakeModelTypeChangeProcessor : public ModelTypeChangeProcessor {
     std::swap(metadata_, batch);
   }
 
+  void OnSyncStarting(const StartCallback& callback) override {}
+
   const std::map<std::string, scoped_ptr<EntityData>>& put_map() const {
     return put_map_;
   }
@@ -163,7 +165,13 @@ class DeviceInfoServiceTest : public testing::Test,
 
   void OnDeviceInfoChange() override { change_count_++; }
 
- protected:
+  scoped_ptr<ModelTypeChangeProcessor> CreateModelTypeChangeProcessor(
+      syncer::ModelType type,
+      ModelTypeService* service) {
+    processor_ = new FakeModelTypeChangeProcessor();
+    return make_scoped_ptr(processor_);
+  }
+
   DeviceInfoServiceTest()
       : change_count_(0),
         store_(ModelTypeStoreTestUtil::CreateInMemoryStoreForTest()),
@@ -182,7 +190,9 @@ class DeviceInfoServiceTest : public testing::Test,
     service_.reset(new DeviceInfoService(
         local_device_.get(),
         base::Bind(&ModelTypeStoreTestUtil::MoveStoreToCallback,
-                   base::Passed(&store_))));
+                   base::Passed(&store_)),
+        base::Bind(&DeviceInfoServiceTest::CreateModelTypeChangeProcessor,
+                   base::Unretained(this))));
     service_->AddObserver(this);
   }
 
@@ -196,9 +206,10 @@ class DeviceInfoServiceTest : public testing::Test,
   // Creates a new processor and sets it on the service. We typically need to
   // pump in this scenario because metadata is going to need to be loading from
   // the store and given to the processor, which is async.
-  void SetProcessorAndPump() {
-    processor_ = new FakeModelTypeChangeProcessor();
-    service()->set_change_processor(make_scoped_ptr(processor_));
+  void CreateProcessorAndPump() {
+    // TODO(skym): Shouldn't need to directly force processor creation anymore.
+    EXPECT_EQ(processor_, service_->GetOrCreateChangeProcessor());
+    ASSERT_TRUE(processor_);
     base::RunLoop().RunUntilIdle();
   }
 
@@ -380,7 +391,7 @@ TEST_F(DeviceInfoServiceTest, TestInitStoreThenProc) {
   AssertEqual(specifics, *all_device_info[0]);
   AssertEqual(specifics, *service()->GetDeviceInfo("tag").get());
 
-  SetProcessorAndPump();
+  CreateProcessorAndPump();
   ASSERT_TRUE(processor()->metadata());
   ASSERT_EQ(state.encryption_key_name(),
             processor()->metadata()->GetDataTypeState().encryption_key_name());
@@ -401,7 +412,7 @@ TEST_F(DeviceInfoServiceTest, TestInitProcBeforeStoreFinishes) {
   // processor is attached and ready before our store init is fully completed.
   ASSERT_EQ(0u, service()->GetAllDeviceInfo().size());
 
-  SetProcessorAndPump();
+  CreateProcessorAndPump();
   ASSERT_TRUE(processor()->metadata());
   ASSERT_EQ(state.encryption_key_name(),
             processor()->metadata()->GetDataTypeState().encryption_key_name());
@@ -534,7 +545,7 @@ TEST_F(DeviceInfoServiceTest, ApplySyncChangesStore) {
   base::RunLoop().RunUntilIdle();
   ShutdownService();
   InitializeAndPump();
-  SetProcessorAndPump();
+  CreateProcessorAndPump();
 
   scoped_ptr<DeviceInfo> info = service()->GetDeviceInfo(tag);
   ASSERT_TRUE(info);
@@ -585,7 +596,7 @@ TEST_F(DeviceInfoServiceTest, MergeBeforeInit) {
 
 TEST_F(DeviceInfoServiceTest, MergeEmpty) {
   InitializeAndPump();
-  SetProcessorAndPump();
+  CreateProcessorAndPump();
   const SyncError error = service()->MergeSyncData(
       service()->CreateMetadataChangeList(), EntityDataMap());
   EXPECT_FALSE(error.IsSet());
@@ -607,7 +618,7 @@ TEST_F(DeviceInfoServiceTest, MergeWithData) {
                             base::Bind(&AssertResultIsSuccess));
 
   InitializeAndPump();
-  SetProcessorAndPump();
+  CreateProcessorAndPump();
 
   EntityDataMap remote_input;
   remote_input[conflict_remote.cache_guid()] =
@@ -646,7 +657,7 @@ TEST_F(DeviceInfoServiceTest, MergeWithData) {
 
 TEST_F(DeviceInfoServiceTest, MergeLocalGuid) {
   InitializeAndPump();
-  SetProcessorAndPump();
+  CreateProcessorAndPump();
 
   // Service should ignore this because it uses the local device's guid.
   DeviceInfoSpecifics specifics(
diff --git a/components/sync_driver/non_blocking_data_type_controller.cc b/components/sync_driver/non_blocking_data_type_controller.cc
index 202ad37..ba4a2f7 100644
--- a/components/sync_driver/non_blocking_data_type_controller.cc
+++ b/components/sync_driver/non_blocking_data_type_controller.cc
@@ -12,6 +12,8 @@
 #include "base/single_thread_task_runner.h"
 #include "components/sync_driver/backend_data_type_configurer.h"
 #include "components/sync_driver/sync_client.h"
+#include "sync/api/model_type_change_processor.h"
+#include "sync/api/model_type_service.h"
 #include "sync/api/sync_error.h"
 #include "sync/api/sync_merge_result.h"
 #include "sync/internal_api/public/activation_context.h"
@@ -59,11 +61,8 @@ void NonBlockingDataTypeController::LoadModels(
   // Start the type processor on the model thread.
   if (!RunOnModelThread(
           FROM_HERE,
-          base::Bind(
-              &syncer_v2::SharedModelTypeProcessor::OnSyncStarting,
-              type_processor(),
-              base::Bind(&NonBlockingDataTypeController::OnProcessorStarted,
-                         this)))) {
+          base::Bind(&NonBlockingDataTypeController::LoadModelsOnModelThread,
+                     this))) {
     LoadModelsDone(
         UNRECOVERABLE_ERROR,
         syncer::SyncError(FROM_HERE, syncer::SyncError::DATATYPE_ERROR,
@@ -71,6 +70,26 @@ void NonBlockingDataTypeController::LoadModels(
   }
 }
 
+void NonBlockingDataTypeController::LoadModelsOnModelThread() {
+  base::WeakPtr<syncer_v2::ModelTypeService> model_type_service =
+      sync_client_->GetModelTypeServiceForType(type());
+  if (!model_type_service.get()) {
+    LOG(WARNING) << "ModelTypeService destroyed before "
+                    "ModelTypeController was started.";
+    // TODO(gangwu): Add SyncError and then call start_callback with it. also
+    // set an error state to |state_|.
+    return;
+  }
+
+  syncer_v2::ModelTypeChangeProcessor* model_type_change_processor =
+      model_type_service->OnSyncStarting(
+          base::Bind(&NonBlockingDataTypeController::OnProcessorStarted, this));
+  DCHECK(model_type_change_processor);
+  type_processor_ = static_cast<syncer_v2::SharedModelTypeProcessor*>(
+                        model_type_change_processor)
+                        ->AsWeakPtrForUI();
+}
+
 void NonBlockingDataTypeController::LoadModelsDone(
     ConfigureResult result,
     const syncer::SyncError& error) {
@@ -228,31 +247,4 @@ syncer::ModelType NonBlockingDataTypeController::type() const {
   return model_type_;
 }
 
-void NonBlockingDataTypeController::InitializeProcessorOnModelThread() {
-  base::WeakPtr<syncer_v2::ModelTypeService> model_type_service =
-      sync_client_->GetModelTypeServiceForType(type());
-  if (!model_type_service.get()) {
-    LOG(WARNING) << "ModelTypeService destroyed before "
-                    "ModelTypeController was started.";
-    // TODO(gangwu): Add SyncError and then call start_callback with it. also
-    // set an error state to |state_|.
-  }
-
-  scoped_ptr<syncer_v2::SharedModelTypeProcessor> shared_model_type_processor(
-      make_scoped_ptr(new syncer_v2::SharedModelTypeProcessor(
-          type(), model_type_service.get())));
-  type_processor_ = shared_model_type_processor->AsWeakPtrForUI();
-  model_type_service->set_change_processor(
-      std::move(shared_model_type_processor));
-}
-
-void NonBlockingDataTypeController::InitializeProcessor() {
-  DCHECK(BelongsToUIThread());
-  RunOnModelThread(
-      FROM_HERE,
-      base::Bind(
-          &NonBlockingDataTypeController::InitializeProcessorOnModelThread,
-          this));
-}
-
 }  // namespace sync_driver_v2
diff --git a/components/sync_driver/non_blocking_data_type_controller.h b/components/sync_driver/non_blocking_data_type_controller.h
index eeeaeb9..3d426aa 100644
--- a/components/sync_driver/non_blocking_data_type_controller.h
+++ b/components/sync_driver/non_blocking_data_type_controller.h
@@ -81,15 +81,11 @@ class NonBlockingDataTypeController : public sync_driver::DataTypeController {
   virtual void RunOnUIThread(const tracked_objects::Location& from_here,
                              const base::Closure& task) = 0;
 
-  // The function will create SharedModelTypeProcessor on model thread.
-  void InitializeProcessor();
-
  private:
   void RecordStartFailure(ConfigureResult result) const;
   void RecordUnrecoverableError();
   void ReportLoadModelError(ConfigureResult result,
                             const syncer::SyncError& error);
-  void InitializeProcessorOnModelThread();
 
   // If the DataType controller is waiting for models to load, once the models
   // are loaded this function should be called to let the base class
@@ -109,6 +105,10 @@ class NonBlockingDataTypeController : public sync_driver::DataTypeController {
       syncer::SyncError error,
       scoped_ptr<syncer_v2::ActivationContext> activation_context);
 
+  // The function LoadModels() will call this function to do some works which
+  // need to be done on model thread.
+  void LoadModelsOnModelThread();
+
   // Model Type for this controller
   syncer::ModelType model_type_;
 
@@ -125,6 +125,9 @@ class NonBlockingDataTypeController : public sync_driver::DataTypeController {
   // callback and must temporarily own it until ActivateDataType is called.
   scoped_ptr<syncer_v2::ActivationContext> activation_context_;
 
+  // TODO(gangwu): We can complete remove holding this pointer since the only
+  // place we use it is when we stop the data type. We can let ModelTypeService
+  // to stop the processor.
   // A weak pointer to the actual SharedModelTypeProcessor
   base::WeakPtr<syncer_v2::SharedModelTypeProcessor> type_processor_;
 
diff --git a/components/sync_driver/non_ui_model_type_controller_unittest.cc b/components/sync_driver/non_ui_model_type_controller_unittest.cc
index a309c36..edf8ce4 100644
--- a/components/sync_driver/non_ui_model_type_controller_unittest.cc
+++ b/components/sync_driver/non_ui_model_type_controller_unittest.cc
@@ -54,8 +54,6 @@ class TestNonUIModelTypeController : public NonUIModelTypeController {
     return model_task_runner_->PostTask(from_here, task);
   }
 
-  void InitializeProcessorInTest() { InitializeProcessor(); }
-
  private:
   ~TestNonUIModelTypeController() override {}
 
@@ -167,7 +165,6 @@ class NonUIModelTypeControllerTest : public testing::Test,
     controller_ = new TestNonUIModelTypeController(
         ui_loop_.task_runner(), model_thread_runner_, base::Closure(),
         syncer::DICTIONARY, this);
-    controller_->InitializeProcessorInTest();
     InitializeTypeProcessor();
   }
 
@@ -186,7 +183,10 @@ class NonUIModelTypeControllerTest : public testing::Test,
   void InitializeTypeProcessor() {
     if (!model_thread_runner_ ||
         model_thread_runner_->BelongsToCurrentThread()) {
-      type_processor_ = controller_->get_type_processor();
+      // TODO(crbug.com/543407): Move the processor stuff out.
+      type_processor_ =
+          service_->SetUpProcessor(new syncer_v2::SharedModelTypeProcessor(
+              syncer::DICTIONARY, service_.get()));
     } else {
       model_thread_runner_->PostTask(
           FROM_HERE,
diff --git a/components/sync_driver/ui_model_type_controller_unittest.cc b/components/sync_driver/ui_model_type_controller_unittest.cc
index 5a9878b..2625200 100644
--- a/components/sync_driver/ui_model_type_controller_unittest.cc
+++ b/components/sync_driver/ui_model_type_controller_unittest.cc
@@ -39,8 +39,6 @@ class TestUIModelTypeController : public UIModelTypeController {
                               model_type,
                               sync_client) {}
 
-  void InitializeProcessorInTest() { InitializeProcessor(); }
-
  private:
   ~TestUIModelTypeController() override {}
 };
@@ -145,10 +143,11 @@ class UIModelTypeControllerTest : public testing::Test,
   void SetUp() override {
     controller_ = new TestUIModelTypeController(
         ui_loop_.task_runner(), base::Closure(), syncer::DEVICE_INFO, this);
-    controller_->InitializeProcessorInTest();
+
+    // TODO(crbug.com/543407): Move the processor stuff out.
     type_processor_ =
-        ((syncer_v2::SharedModelTypeProcessor*)service_.change_processor())
-            ->AsWeakPtrForUI();
+        service_.SetUpProcessor(new syncer_v2::SharedModelTypeProcessor(
+            syncer::DEVICE_INFO, &service_));
   }
 
   void TearDown() override {