Refactor ModelTypeController state transitions

More explicit state handling is introduced with a switch, and some dead
code is replaced with a DCHECK.

Bug: 855375
Change-Id: I644d26e63bf51bf0cb56362afa0fa990f75e8be7
Reviewed-on: https://chromium-review.googlesource.com/1111708Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569579}

components/sync/driver/model_association_manager.cc

@@ -180,6 +180,7 @@ void ModelAssociationManager::LoadEnabledTypes() {
     auto dtc_iter = controllers_->find(type);
     DCHECK(dtc_iter != controllers_->end());
     DataTypeController* dtc = dtc_iter->second.get();
+    DCHECK_NE(DataTypeController::STOPPING, dtc->state());
     if (dtc->state() == DataTypeController::NOT_RUNNING) {
       DCHECK(!loaded_types_.Has(dtc->type()));
       DCHECK(!associated_types_.Has(dtc->type()));

components/sync/driver/model_type_controller.cc

@@ -108,14 +108,9 @@ void ModelTypeController::LoadModels(
     const ModelLoadCallback& model_load_callback) {
   DCHECK(CalledOnValidThread());
   DCHECK(!model_load_callback.is_null());
-  model_load_callback_ = model_load_callback;
+  DCHECK_EQ(NOT_RUNNING, state_);
 
-  if (state() != NOT_RUNNING) {
-    LoadModelsDone(RUNTIME_ERROR,
-                   SyncError(FROM_HERE, SyncError::DATATYPE_ERROR,
-                             "Model already running", type()));
-    return;
-  }
+  model_load_callback_ = model_load_callback;
 
   DVLOG(1) << "Sync starting for " << ModelTypeToString(type());
   state_ = MODEL_STARTING;
@@ -169,6 +164,7 @@ void ModelTypeController::LoadModelsDone(ConfigureResult result,
     DVLOG(1) << "Sync start completed for " << ModelTypeToString(type());
   } else {
     RecordStartFailure(result);
+    state_ = FAILED;
   }
 
   if (!model_load_callback_.is_null()) {
@@ -241,21 +237,31 @@ void ModelTypeController::DeactivateDataType(ModelTypeConfigurer* configurer) {
 void ModelTypeController::Stop(SyncStopMetadataFate metadata_fate) {
   DCHECK(CalledOnValidThread());
 
-  if (state() == NOT_RUNNING)
-    return;
-
-  // Only call StopSync if the delegate is ready to handle it (controller is
-  // in loaded state).
-  if (state() == MODEL_LOADED || state() == RUNNING) {
-    DVLOG(1) << "Stopping sync for " << ModelTypeToString(type());
-    PostModelTask(FROM_HERE,
-                  base::BindOnce(&StopSyncHelperOnModelThread, metadata_fate));
-  } else {
-    DCHECK_EQ(MODEL_STARTING, state_);
-    DVLOG(1) << "Shortcutting stop for " << ModelTypeToString(type())
-             << " because it's still starting";
-    // TODO(mastiz): Enter STOPPING state here and/or queue pending stops,
-    // together with |metadata_fate|.
+  switch (state()) {
+    case ASSOCIATING:
+    case STOPPING:
+      // We don't really use these states in this class.
+      NOTREACHED();
+      break;
+
+    case NOT_RUNNING:
+    case FAILED:
+      // Nothing to stop. |metadata_fate| might require CLEAR_METADATA,
+      // which could lead to leaking sync metadata, but it doesn't seem a
+      // realistic scenario (disable sync during shutdown?).
+      return;
+
+    case MODEL_STARTING:
+      DLOG(WARNING) << "Shortcutting stop for " << ModelTypeToString(type())
+                    << " because it's still starting";
+      break;
+
+    case MODEL_LOADED:
+    case RUNNING:
+      DVLOG(1) << "Stopping sync for " << ModelTypeToString(type());
+      PostModelTask(FROM_HERE, base::BindOnce(&StopSyncHelperOnModelThread,
+                                              metadata_fate));
+      break;
   }
 
   state_ = NOT_RUNNING;

components/sync/driver/model_type_controller_unittest.cc

@@ -312,16 +312,6 @@ TEST_F(ModelTypeControllerTest, LoadModelsOnBackendThread) {
   ExpectProcessorConnected(false);
 }
 
-TEST_F(ModelTypeControllerTest, LoadModelsTwice) {
-  LoadModels();
-  RunAllTasks();
-  EXPECT_EQ(DataTypeController::MODEL_LOADED, controller()->state());
-  EXPECT_FALSE(load_models_last_error().IsSet());
-  // A second LoadModels call should set the error.
-  LoadModels();
-  EXPECT_TRUE(load_models_last_error().IsSet());
-}
-
 TEST_F(ModelTypeControllerTest, Activate) {
   LoadModels();
   RunAllTasks();