Cleanup + rename ModelAssociationManager::TypeStartCallback

TypeStartCallback was misnamed; it's not used as a callback. This CL
renames it to MarkDataTypeAssociationDone which is closer to what it
does/means.
It also removes an unreachable branch within the method (replaced by
DCHECKs).

Bug: 647505
Change-Id: I43e041c20ff14a71260c85c4717d8c2e62ed0708
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2466098Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#816170}

components/sync/driver/model_association_manager.cc

@@ -68,7 +68,7 @@ ModelAssociationManager::ModelAssociationManager(
       configure_status_(DataTypeManager::UNKNOWN),
       notified_about_ready_for_configure_(false) {}
 
-ModelAssociationManager::~ModelAssociationManager() {}
+ModelAssociationManager::~ModelAssociationManager() = default;
 
 void ModelAssociationManager::Initialize(ModelTypeSet desired_types,
                                          ModelTypeSet preferred_types,
@@ -185,8 +185,7 @@ void ModelAssociationManager::StopDatatypeImpl(
 
 void ModelAssociationManager::LoadEnabledTypes() {
   // Load in kStartOrder.
-  for (size_t i = 0; i < base::size(kStartOrder); i++) {
-    ModelType type = kStartOrder[i];
+  for (ModelType type : kStartOrder) {
     if (!desired_types_.Has(type))
       continue;
 
@@ -203,6 +202,7 @@ void ModelAssociationManager::LoadEnabledTypes() {
                               weak_ptr_factory_.GetWeakPtr()));
     }
   }
+  // It's possible that all models are already loaded.
   NotifyDelegateIfReadyForConfigure();
 }
 
@@ -233,9 +233,8 @@ void ModelAssociationManager::StartAssociationAsync(
                base::BindOnce(&ModelAssociationManager::ModelAssociationDone,
                               weak_ptr_factory_.GetWeakPtr(), INITIALIZED));
 
-  // Start association of types that are loaded in specified order.
-  for (size_t i = 0; i < base::size(kStartOrder); i++) {
-    ModelType type = kStartOrder[i];
+  // Associate types that are already loaded in specified order.
+  for (ModelType type : kStartOrder) {
     if (!associating_types_.Has(type) || !loaded_types_.Has(type))
       continue;
 
@@ -243,7 +242,7 @@ void ModelAssociationManager::StartAssociationAsync(
     TRACE_EVENT_ASYNC_BEGIN1("sync", "ModelAssociation", dtc, "DataType",
                              ModelTypeToString(type));
 
-    TypeStartCallback(type);
+    MarkDataTypeAssociationDone(type);
   }
 }
 
@@ -310,26 +309,26 @@ void ModelAssociationManager::ModelLoadCallback(ModelType type,
     // MODEL_LOADED.
     // TODO(pavely): Add test for this scenario in DataTypeManagerImpl
     // unittests.
+    // TODO(crbug.com/647505): The above sounds quite broken (will
+    // MarkDataTypeAssociationDone never get called in that case?!), and also
+    // outdated (StartAssociating doesn't exist anymore). Can we just move the
+    // NotifyDelegateIfReadyForConfigure call below?
     if (dtc->state() == DataTypeController::MODEL_LOADED) {
-      TypeStartCallback(type);
+      MarkDataTypeAssociationDone(type);
     }
   }
 }
 
-void ModelAssociationManager::TypeStartCallback(ModelType type) {
-  // This happens for example if a type disables itself after initial
-  // configuration.
-  if (!desired_types_.Has(type)) {
-    // It's possible all types failed to associate, in which case association
-    // is complete.
-    if (state_ == ASSOCIATING && associating_types_.Empty())
-      ModelAssociationDone(INITIALIZED);
-    return;
-  }
-
+void ModelAssociationManager::MarkDataTypeAssociationDone(ModelType type) {
+  DCHECK(desired_types_.Has(type));
+  DCHECK(associating_types_.Has(type));
+  DCHECK(loaded_types_.Has(type));
   DCHECK(!associated_types_.Has(type));
+
   associated_types_.Put(type);
 
+  // TODO(crbug.com/647505): Should we check this *before* adding the type to
+  // |associated_types_|?
   if (state_ != ASSOCIATING)
     return;
 

components/sync/driver/model_association_manager.h

@@ -104,10 +104,6 @@ class ModelAssociationManager {
                     ShutdownReason shutdown_reason,
                     SyncError error);
 
-  // This is used for TESTING PURPOSE ONLY. The test case can inspect
-  // and modify the timer.
-  // TODO(sync) : This would go away if we made this class be able to do
-  // Dependency injection. crbug.com/129212.
   base::OneShotTimer* GetTimerForTesting();
 
   State state() const { return state_; }
@@ -120,8 +116,7 @@ class ModelAssociationManager {
   // will be passed to |LoadModels| function.
   void ModelLoadCallback(ModelType type, const SyncError& error);
 
-  // TODO(crbug.com/647505): Consider removing or renaming this function.
-  void TypeStartCallback(ModelType type);
+  void MarkDataTypeAssociationDone(ModelType type);
 
   // Called when all requested types are associated or association times out.
   // Will clean up any unfinished types, and update |state_| to be |new_state|