[USS] Change the place where SharedModelTypeProcessor got created

components/sync_driver/non_blocking_data_type_controller.cc

Index: components/sync_driver/non_blocking_data_type_controller.cc
diff --git a/components/sync_driver/non_blocking_data_type_controller.cc b/components/sync_driver/non_blocking_data_type_controller.cc
index f2996ab1e97325dfd0b5577de28efb704b36a651..a28f35747a87fed20fa1d339f6ec033253de2442 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"
@@ -53,11 +55,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,
@@ -65,6 +64,31 @@ 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;
+  }
+
+  model_type_service->OnSyncStarting(
+      base::Bind(&NonBlockingDataTypeController::OnProcessorStarted, this));
+
+  syncer_v2::ModelTypeChangeProcessor* model_type_change_processor =

[skym, 2016/03/22 15:56:26]

This order of operations makes me really uneasy. If the processor already
exists, it's going to invoke our callback in task, and that will happen before
we get around to assigning this member field. Looks like our current logic is
safe but we're kind of setting a trap for future selves.

Is there a reason we need the processor synchronously here? Does it make more
sense to push that into the callback and remove the change_processor() accessor?
What do others think? I'm not sure what's better.

[Gang Wu, 2016/03/22 17:47:50]

Yes, I agree.
This order is not good. And also will cause ModelTypeService to cast processor
from ModelTypeChangeProcessor to SharedModelTypeProcessor(see
model_type_service.cc).I think maybe we can keep previous order.
1. controller ask service for processor, if not exist, please create one.
2. controller call OnSyncStarting.
what others think?

[skym, 2016/03/22 19:02:06]

Your approach at first makes sense, all the right pieces of data go to the right
places. But if you take a step back, it becomes a little awkward. I think the
idea we had last week during our meeting was that this should be a signal to the
service that we are starting sync, and it should be loading the metadata right
now if it hasn't already. Right now we have the OnChangeProcessorSet method that
kind of performs this role, but it's really ugly.

+      model_type_service->change_processor();
+  if (model_type_change_processor == NULL) {
+    LOG(WARNING) << "Cannot initalize ModelTypeChangeProcessor.";
+    return;
+  }
+  type_processor_ = static_cast<syncer_v2::SharedModelTypeProcessor*>(

[skym, 2016/03/22 15:56:26]

What happens if this cast fails? Should there be another check?

[Gang Wu, 2016/03/22 17:47:50]

I am not familiar with static_cast, but I think static_cast is not necessary to
check.
maybe change to dynamic_cast? since dynamic_cast will return null if failed to
cast.

[skym, 2016/03/22 19:02:06]

Yeah, after doing some reading it sounds like dynamic_cast is the right thing to
do here. We shouldn't static_casting from base to child classes.

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

After I use dynamic_cast, I receive a compiler error. then realize if we want to
use dynamic_cast, we need to enable rtti for compiler. And after checking Google
C++ style guide, it does not recommend to use dynamic_cast for non-unittest. and
since we are sure to always create SharedModelTypeProcessor,  I think
static_cast is good to use her.

+                        model_type_change_processor)
+                        ->AsWeakPtrForUI();
+}
+
 void NonBlockingDataTypeController::LoadModelsDone(
     ConfigureResult result,
     const syncer::SyncError& error) {
@@ -222,31 +246,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