[USS] Change the place where SharedModelTypeProcessor got created

sync/api/model_type_service.cc

Index: sync/api/model_type_service.cc
diff --git a/sync/api/model_type_service.cc b/sync/api/model_type_service.cc
index 87df14bf6011dc559b98e90082d0e2d29f1235d7..cfdf8b408dc35ee497619334ed03f1c5002abe57 100644
--- a/sync/api/model_type_service.cc
+++ b/sync/api/model_type_service.cc
@@ -4,9 +4,13 @@
 
 #include "sync/api/model_type_service.h"
 
+#include "sync/internal_api/public/shared_model_type_processor.h"
+
 namespace syncer_v2 {
 
-ModelTypeService::ModelTypeService() {}
+ModelTypeService::ModelTypeService(
+    const SharedProcessorFactory& shared_processor_factory)
+    : shared_processor_factory_(shared_processor_factory) {}
 
 ModelTypeService::~ModelTypeService() {}
 
@@ -14,15 +18,26 @@ ModelTypeChangeProcessor* ModelTypeService::change_processor() const {
   return change_processor_.get();
 }
 
-void ModelTypeService::set_change_processor(
-    scoped_ptr<ModelTypeChangeProcessor> change_processor) {
-  DCHECK(!change_processor_);
-  change_processor_.swap(change_processor);
-  OnChangeProcessorSet();
+ModelTypeChangeProcessor* ModelTypeService::create_change_processor() {

[maxbogue, 2016/03/22 21:40:12]

This function is doing nontrivial logic, which means it should be named
CreateChangeProcessor. I also think you can just return void here since any
callers should be able to access the processor via change_processor(). It's
cleaner to only have one way to access the pointer.

[Gang Wu, 2016/03/24 15:56:26]

Done.

+  if (!change_processor_.get()) {

[maxbogue, 2016/03/22 21:40:12]

I think you should just either just DCHECK(!change_processor_) and make callers
check change_processor() or name the function GetOrCreateChangeProcessor. I
would prefer the former, as it makes it more explicit when the creation is
happening.

[skym, 2016/03/22 22:18:17]

+1 to GetOrCreateChangeProcessor. Checking change_procesor() is clunky, and
every service would have to do this. Also, you might be able to make
change_processor() protected now that the controller should only call the
GetOrCreateChangeProcessor() method.

[Gang Wu, 2016/03/24 15:56:26]

Done.

[Gang Wu, 2016/03/24 15:56:26]

Done.

+    change_processor_.reset(
+        shared_processor_factory_.Run(type(), this).release());
+    if (!change_processor_.get()) {

[maxbogue, 2016/03/22 21:40:12]

Again, better to just DCHECK(change_processor_).

[Gang Wu, 2016/03/24 15:56:26]

Done.

+      LOG(WARNING) << "Cannot initalize ModelTypeChangeProcessor.";
+    }
+    OnChangeProcessorSet();
+  }
+  return change_processor_.get();
 }
 
 void ModelTypeService::clear_change_processor() {
   change_processor_.reset();
 }
 
+void ModelTypeService::OnSyncStarting(const StartCallback& start_callback) {
+  // TODO(gangwu): should not cast here, find a better way to call.
+  static_cast<SharedModelTypeProcessor*>(create_change_processor())

[maxbogue, 2016/03/22 21:40:12]

OnSyncStarting should just be moved into the ModelTypeChangeProcessor interface.
Please either do so or have the TODO here say that.

[Gang Wu, 2016/03/24 15:56:26]

Done.

+      ->OnSyncStarting(start_callback);
+}
+
 }  // namespace syncer_v2