Various small cleanups in ModelAssociationManager

Tightening up DCHECKs, removing dead code paths, updating comments.

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

components/sync/driver/model_association_manager.cc

@@ -7,8 +7,7 @@
 #include <stddef.h>
 #include <stdint.h>
 
-#include <algorithm>
-#include <functional>
+#include <map>
 #include <utility>
 
 #include "base/barrier_closure.h"
@@ -16,10 +15,7 @@
 #include "base/bind_helpers.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_functions.h"
-#include "base/stl_util.h"
-#include "base/trace_event/trace_event.h"
 #include "components/sync/base/model_type.h"
-#include "components/sync/base/sync_stop_metadata_fate.h"
 
 namespace syncer {
 
@@ -62,9 +58,9 @@ const int64_t kAssociationTimeOutInSeconds = 600;
 ModelAssociationManager::ModelAssociationManager(
     const DataTypeController::TypeMap* controllers,
     ModelAssociationManagerDelegate* processor)
-    : state_(IDLE),
-      controllers_(controllers),
+    : controllers_(controllers),
       delegate_(processor),
+      state_(IDLE),
       configure_status_(DataTypeManager::UNKNOWN),
       notified_about_ready_for_configure_(false) {}
 
@@ -90,7 +86,7 @@ void ModelAssociationManager::Initialize(ModelTypeSet desired_types,
   for (ModelType type : desired_types) {
     auto dtc_iter = controllers_->find(type);
     if (dtc_iter != controllers_->end()) {
-      DataTypeController* dtc = dtc_iter->second.get();
+      const DataTypeController* dtc = dtc_iter->second.get();
       // Controllers in a FAILED state should have been filtered out by the
       // DataTypeManager.
       DCHECK_NE(dtc->state(), DataTypeController::FAILED);
@@ -132,12 +128,12 @@ void ModelAssociationManager::Initialize(ModelTypeSet desired_types,
     }
   }
 
-  // Run LoadEnabledTypes() only after all relevant types are stopped.
+  // Run LoadDesiredTypes() only after all relevant types are stopped.
   // TODO(mastiz): Add test coverage to this waiting logic, including the
   // case where the datatype is STOPPING when this function is called.
   base::RepeatingClosure barrier_closure = base::BarrierClosure(
       types_to_stop.size(),
-      base::BindOnce(&ModelAssociationManager::LoadEnabledTypes,
+      base::BindOnce(&ModelAssociationManager::LoadDesiredTypes,
                      weak_ptr_factory_.GetWeakPtr()));
 
   for (const auto& dtc_and_reason : types_to_stop) {
@@ -183,7 +179,7 @@ void ModelAssociationManager::StopDatatypeImpl(
   dtc->Stop(shutdown_reason, std::move(callback));
 }
 
-void ModelAssociationManager::LoadEnabledTypes() {
+void ModelAssociationManager::LoadDesiredTypes() {
   // Load in kStartOrder.
   for (ModelType type : kStartOrder) {
     if (!desired_types_.Has(type))
@@ -235,14 +231,8 @@ void ModelAssociationManager::StartAssociationAsync(
 
   // Associate types that are already loaded in specified order.
   for (ModelType type : kStartOrder) {
-    if (!associating_types_.Has(type) || !loaded_types_.Has(type))
-      continue;
-
-    DataTypeController* dtc = controllers_->find(type)->second.get();
-    TRACE_EVENT_ASYNC_BEGIN1("sync", "ModelAssociation", dtc, "DataType",
-                             ModelTypeToString(type));
-
-    MarkDataTypeAssociationDone(type);
+    if (associating_types_.Has(type) && loaded_types_.Has(type))
+      MarkDataTypeAssociationDone(type);
   }
 }
 
@@ -269,7 +259,6 @@ void ModelAssociationManager::Stop(ShutdownReason shutdown_reason) {
   if (state_ == ASSOCIATING) {
     if (configure_status_ == DataTypeManager::OK)
       configure_status_ = DataTypeManager::ABORTED;
-    DVLOG(1) << "ModelAssociationManager: Calling OnModelAssociationDone";
     ModelAssociationDone(IDLE);
   } else {
     DCHECK(associating_types_.Empty());
@@ -301,12 +290,12 @@ void ModelAssociationManager::ModelLoadCallback(ModelType type,
   loaded_types_.Put(type);
   NotifyDelegateIfReadyForConfigure();
   if (associating_types_.Has(type)) {
-    DataTypeController* dtc = controllers_->find(type)->second.get();
+    const DataTypeController* dtc = controllers_->find(type)->second.get();
     // If initial sync was done for this datatype then
     // NotifyDelegateIfReadyForConfigure possibly already triggered model
     // association and StartAssociating was already called for this type. To
     // ensure StartAssociating is called only once only make a call if state is
-    // MODEL_LOADED.
+    // MODEL_LOADED (i.e. not RUNNING yet).
     // TODO(pavely): Add test for this scenario in DataTypeManagerImpl
     // unittests.
     // TODO(crbug.com/647505): The above sounds quite broken (will
@@ -320,41 +309,25 @@ void ModelAssociationManager::ModelLoadCallback(ModelType type,
 }
 
 void ModelAssociationManager::MarkDataTypeAssociationDone(ModelType type) {
+  DCHECK_EQ(state_, ASSOCIATING);
   DCHECK(desired_types_.Has(type));
   DCHECK(associating_types_.Has(type));
   DCHECK(loaded_types_.Has(type));
   DCHECK(!associated_types_.Has(type));
 
   associated_types_.Put(type);
+  associating_types_.Remove(type);
 
-  // TODO(crbug.com/647505): Should we check this *before* adding the type to
-  // |associated_types_|?
-  if (state_ != ASSOCIATING)
-    return;
-
-  TRACE_EVENT_ASYNC_END1("sync", "ModelAssociation",
-                         controllers_->find(type)->second.get(), "DataType",
-                         ModelTypeToString(type));
-
-  // Track the merge results if we succeeded or an association failure
-  // occurred.
   if (ProtocolTypes().Has(type))
     delegate_->OnSingleDataTypeAssociationDone(type);
 
-  associating_types_.Remove(type);
-
   if (associating_types_.Empty())
     ModelAssociationDone(INITIALIZED);
 }
 
 void ModelAssociationManager::ModelAssociationDone(State new_state) {
-  DCHECK_NE(IDLE, state_);
-
-  if (state_ == INITIALIZED) {
-    // No associations are currently happening. Just reset the state.
-    state_ = new_state;
-    return;
-  }
+  DCHECK_EQ(ASSOCIATING, state_);
+  DCHECK_NE(ASSOCIATING, new_state);
 
   DVLOG(1) << "Model association complete for "
            << ModelTypeSetToString(requested_types_);
@@ -395,11 +368,9 @@ void ModelAssociationManager::NotifyDelegateIfReadyForConfigure() {
   if (notified_about_ready_for_configure_)
     return;
 
-  for (ModelType type : desired_types_) {
-    if (!loaded_types_.Has(type)) {
-      // At least one type is not ready.
-      return;
-    }
+  if (!loaded_types_.HasAll(desired_types_)) {
+    // At least one type is not ready.
+    return;
   }
 
   notified_about_ready_for_configure_ = true;

components/sync/driver/model_association_manager.h

@@ -21,23 +21,13 @@ namespace syncer {
 
 struct ConfigureContext;
 
-// TODO(crbug.com/1102837): Association was a Directory concept, this class
-// should disappear or be refactored.
-//
-// |ModelAssociationManager| does the heavy lifting for doing the actual model
-// association. It instructs DataTypeControllers to load models, start
-// associating and stopping. Since the operations are async it uses an
-// interface to inform DataTypeManager the results of the operations.
-// This class is owned by DataTypeManager.
-// |ModelAssociationManager| association functions are async. The results of
-// those operations are passed back via this interface.
+// Interface for ModelAssociationManager to pass the results of async operations
+// back to DataTypeManager.
 class ModelAssociationManagerDelegate {
  public:
-  // Called when all desired types are ready to be configured with
-  // ModelTypeConfigurer. Data type is ready when its progress marker is
-  // available to configurer. Directory data types are always ready, their
-  // progress markers are read from directory. USS data type controllers need to
-  // load model and read data type context first.
+  // Called when all desired types are loaded, i.e. are ready to be configured
+  // with ModelTypeConfigurer. A data type is ready when its progress marker is
+  // available, which is the case once the local model has been loaded.
   // This function is called at most once after each call to
   // ModelAssociationManager::Initialize().
   virtual void OnAllDataTypesReadyForConfigure() = 0;
@@ -47,8 +37,8 @@ class ModelAssociationManagerDelegate {
   virtual void OnSingleDataTypeAssociationDone(ModelType type) = 0;
 
   // Called when the ModelAssociationManager has decided it must stop |type|,
-  // likely because it is no longer a desired data type or sync is shutting
-  // down.
+  // likely because it is no longer a desired data type, sync is shutting down,
+  // or some error occurred during loading.
   virtual void OnSingleDataTypeWillStop(ModelType type,
                                         const SyncError& error) = 0;
 
@@ -56,9 +46,15 @@ class ModelAssociationManagerDelegate {
   // association for all desired types and has nothing left to do.
   virtual void OnModelAssociationDone(
       const DataTypeManager::ConfigureResult& result) = 0;
-  virtual ~ModelAssociationManagerDelegate() {}
+
+  virtual ~ModelAssociationManagerDelegate() = default;
 };
 
+// |ModelAssociationManager| instructs DataTypeControllers to load models and
+// to stop (DataTypeManager is responsible for activating/deactivating data
+// types). Since the operations are async it uses an interface to inform
+// DataTypeManager of the results of the operations.
+// This class is owned by DataTypeManager.
 // TODO(crbug.com/1102837): Association was a Directory concept, this class
 // should disappear or be refactored.
 class ModelAssociationManager {
@@ -78,14 +74,13 @@ class ModelAssociationManager {
                           ModelAssociationManagerDelegate* delegate);
   virtual ~ModelAssociationManager();
 
-  // Initializes the state to do the model association in future. This
-  // should be called before communicating with sync server. A subsequent call
-  // of Initialize is only allowed if the ModelAssociationManager has invoked
-  // |OnModelAssociationDone| on the |ModelAssociationManagerDelegate|. After
-  // this call, there should be several calls to StartAssociationAsync()
-  // to associate subset of |desired_types| which must be a subset of
-  // |preferred_types|.
-  // |preferred_types| contains types selected by user.
+  // Stops any data types that are *not* in |desired_types|, then kicks off
+  // loading of all |desired_types|. A subsequent Initialize() call is only
+  // allowed after the ModelAssociationManager has invoked
+  // OnModelAssociationDone() on the delegate. After this call, there should be
+  // several calls to StartAssociationAsync() to associate subsets of
+  // |desired_types|, which itself must be a subset of |preferred_types|.
+  // |preferred_types| contains all types selected by the user.
   void Initialize(ModelTypeSet desired_types,
                   ModelTypeSet preferred_types,
                   const ConfigureContext& context);
@@ -110,10 +105,10 @@ class ModelAssociationManager {
 
  private:
   // Start loading non-running types that are in |desired_types_|.
-  void LoadEnabledTypes();
+  void LoadDesiredTypes();
 
-  // Callback that will be invoked when the models finish loading. This callback
-  // will be passed to |LoadModels| function.
+  // Callback that will be invoked when the model for |type| finishes loading.
+  // This callback is passed to |LoadModels| function.
   void ModelLoadCallback(ModelType type, const SyncError& error);
 
   void MarkDataTypeAssociationDone(ModelType type);
@@ -129,13 +124,17 @@ class ModelAssociationManager {
                         DataTypeController* dtc,
                         DataTypeController::StopCallback callback);
 
-  // Calls delegate's OnAllDataTypesReadyForConfigure when all datatypes from
-  // desired_types_ are ready for configure. Ensures that for every call to
-  // Initialize callback is called at most once.
-  // Datatype is ready if either it doesn't require LoadModels before configure
-  // or LoadModels successfully finished.
+  // Calls delegate's OnAllDataTypesReadyForConfigure if all datatypes from
+  // |desired_types_| are loaded. Ensures that OnAllDataTypesReadyForConfigure
+  // is called at most once for every call to Initialize().
   void NotifyDelegateIfReadyForConfigure();
 
+  // Set of all registered controllers.
+  const DataTypeController::TypeMap* const controllers_;
+
+  // The delegate in charge of handling model association results.
+  ModelAssociationManagerDelegate* const delegate_;
+
   State state_;
 
   ConfigureContext configure_context_;
@@ -143,11 +142,12 @@ class ModelAssociationManager {
   // Data types that are enabled.
   ModelTypeSet desired_types_;
 
-  // Data types that are requested to associate.
+  // Data types that are requested to associate. Non-empty iff |state_| is
+  // ASSOCIATING.
   ModelTypeSet requested_types_;
 
-  // Data types currently being associated, including types waiting for model
-  // load.
+  // Data types currently being associated, including types still waiting for
+  // model load. Non-empty iff |state_| is ASSOCIATING.
   ModelTypeSet associating_types_;
 
   // Data types that are loaded, i.e. ready to associate.
@@ -157,13 +157,8 @@ class ModelAssociationManager {
   // reconfiguration if not disabled.
   ModelTypeSet associated_types_;
 
-  // Set of all registered controllers.
-  const DataTypeController::TypeMap* controllers_;
-
-  // The processor in charge of handling model association results.
-  ModelAssociationManagerDelegate* delegate_;
-
-  // Timer to track and limit how long a datatype takes to model associate.
+  // Timer to track and limit how long a data type still takes to load after
+  // StartAssociationAsync is called.
   base::OneShotTimer timer_;
 
   DataTypeManager::ConfigureStatus configure_status_;