Clean up comments in DataTypeManagerImpl/ModelLoadManager

This removes some usage of "association" terminology, which is a concept
that doesn't exist anymore. It also adds a few TODOs for further
"proper" cleanups.

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

components/sync/driver/data_type_manager_impl.cc

@@ -476,6 +476,8 @@ void DataTypeManagerImpl::DownloadCompleted(
     needs_reconfigure_ = true;
   }
 
+  // If a reconfigure was requested while this download was ongoing, process it
+  // now.
   if (needs_reconfigure_) {
     download_types_queue_ = base::queue<ModelTypeSet>();
     ProcessReconfigure();
@@ -486,8 +488,7 @@ void DataTypeManagerImpl::DownloadCompleted(
   download_types_queue_.pop();
 
   // Those types that were already downloaded (non first sync/error types)
-  // should already be associating. Just kick off association of the newly
-  // downloaded types if necessary.
+  // are already done. Just finalize the newly downloaded types if necessary.
   if (!association_types_queue_.empty()) {
     association_types_queue_.back().first_sync_types = first_sync_types;
     association_types_queue_.back().download_ready_time = base::Time::Now();
@@ -780,8 +781,7 @@ void DataTypeManagerImpl::StopImpl(ShutdownReason reason) {
   // Invalidate weak pointer to drop download callbacks.
   weak_ptr_factory_.InvalidateWeakPtrs();
 
-  // Stop all data types. This may trigger association callback but the
-  // callback will do nothing because state is set to STOPPING above.
+  // Stop all data types.
   model_load_manager_.Stop(reason);
 
   // Individual data type controllers might still be STOPPING, but we don't

components/sync/driver/data_type_manager_impl.h

@@ -109,7 +109,7 @@ class DataTypeManagerImpl : public DataTypeManager,
                                 DataTypeConfigStateMap* state_map);
 
   // Prepare the parameters for the configurer's configuration. Returns the set
-  // of types that are already ready for association.
+  // of types that are already ready for configuration.
   ModelTypeSet PrepareConfigureParams(
       ModelTypeConfigurer::ConfigureParams* params);
 
@@ -129,10 +129,10 @@ class DataTypeManagerImpl : public DataTypeManager,
   // was an actual change.
   bool UpdatePreconditionError(ModelType type);
 
-  // Post a task to reconfigure when no downloading or association are running.
+  // Starts a reconfiguration if it's required and no downloads are running.
   void ProcessReconfigure();
 
-  // Programmatically force reconfiguration of data type (if needed).
+  // Programmatically force reconfiguration of all data types (if needed).
   void ForceReconfiguration();
 
   void Restart();
@@ -155,13 +155,15 @@ class DataTypeManagerImpl : public DataTypeManager,
 
   // Start download of next set of types in |download_types_queue_| (if
   // any exist, does nothing otherwise).
-  // Will kick off association of any new ready types.
+  // Will kick off configuration of any new ready types.
   void StartNextDownload(ModelTypeSet high_priority_types_before);
 
   // Start association of next batch of data types after association of
   // previous batch finishes. |group| controls which set of types within
-  // an AssociationTypesInfo to associate. Does nothing if model associator
-  // is busy performing association.
+  // an AssociationTypesInfo to associate.
+  // TODO(crbug.com/1102837): Simplify and rename this. "Association" doesn't
+  // exist anymore; all this does is record configuration stats and eventually
+  // update |state_|.
   void StartNextAssociation(AssociationGroup group);
 
   void StopImpl(ShutdownReason reason);
@@ -202,11 +204,11 @@ class DataTypeManagerImpl : public DataTypeManager,
   // The last time Restart() was called.
   base::Time last_restart_time_;
 
-  // Sync's datatype debug info listener, which we pass model association
+  // Sync's datatype debug info listener, which we pass model configuration
   // statistics to.
   const WeakHandle<DataTypeDebugInfoListener> debug_info_listener_;
 
-  // The manager that handles the model association of the individual types.
+  // The manager that loads the local models of the data types.
   ModelLoadManager model_load_manager_;
 
   // DataTypeManager must have only one observer -- the ProfileSyncService that
@@ -256,7 +258,7 @@ class DataTypeManagerImpl : public DataTypeManager,
   // datatype encryption.
   const DataTypeEncryptionHandler* encryption_handler_;
 
-  // Association and time stats of data type configuration.
+  // Timing stats of data type configuration.
   std::map<ModelType, DataTypeConfigurationStats> configuration_stats_;
 
   // Configuration process is started when ModelLoadManager notifies

components/sync/driver/glue/sync_engine_backend.cc

@@ -414,6 +414,9 @@ void SyncEngineBackend::DoPurgeDisabledTypes(const ModelTypeSet& to_purge) {
     // for Nigori we need to do it here.
     // TODO(crbug.com/922900): try to find better way to implement this logic,
     // it's likely happen only due to BackendMigrator.
+    // TODO(crbug.com/1142771): Evaluate whether this logic is necessary at all.
+    // There's no "purging" logic for any other data type, so likely it's not
+    // necessary for NIGORI either.
     sync_manager_->GetModelTypeConnector()->DisconnectDataType(NIGORI);
     nigori_controller_->Stop(ShutdownReason::DISABLE_SYNC, base::DoNothing());
     LoadAndConnectNigoriController();

components/sync/driver/model_load_manager_unittest.cc

@@ -101,9 +101,8 @@ TEST_F(SyncModelLoadManagerTest, StopAfterFinish) {
   EXPECT_EQ(0, GetController(BOOKMARKS)->model()->clear_metadata_call_count());
 }
 
-// Test that model that failed to load between initialization and association
-// is reported and stopped properly.
-TEST_F(SyncModelLoadManagerTest, ModelLoadFailBeforeAssociationStart) {
+// Test that a model that failed to load is reported and stopped properly.
+TEST_F(SyncModelLoadManagerTest, ModelLoadFail) {
   controllers_[BOOKMARKS] = std::make_unique<FakeDataTypeController>(BOOKMARKS);
   GetController(BOOKMARKS)->model()->SimulateModelError(
       ModelError(FROM_HERE, "Test error"));
@@ -335,7 +334,7 @@ TEST_F(SyncModelLoadManagerTest, StopDataType_NotRunning) {
 // Test that Initialize stops controllers with KEEP_METADATA for preferred
 // types.
 TEST_F(SyncModelLoadManagerTest, KeepsMetadataForPreferredDataType) {
-  // Associate model with two data types.
+  // Initialize the manager with two data types.
   controllers_[BOOKMARKS] = std::make_unique<FakeDataTypeController>(BOOKMARKS);
   controllers_[APPS] = std::make_unique<FakeDataTypeController>(APPS);
   ModelLoadManager model_load_manager(&controllers_, &delegate_);
@@ -370,7 +369,7 @@ TEST_F(SyncModelLoadManagerTest, KeepsMetadataForPreferredDataType) {
 // Test that Initialize stops controllers with CLEAR_METADATA for
 // no-longer-preferred types.
 TEST_F(SyncModelLoadManagerTest, ClearsMetadataForNotPreferredDataType) {
-  // Associate model with two data types.
+  // Initialize the manager with two data types.
   controllers_[BOOKMARKS] = std::make_unique<FakeDataTypeController>(BOOKMARKS);
   controllers_[APPS] = std::make_unique<FakeDataTypeController>(APPS);
   ModelLoadManager model_load_manager(&controllers_, &delegate_);
@@ -404,7 +403,7 @@ TEST_F(SyncModelLoadManagerTest, ClearsMetadataForNotPreferredDataType) {
 }
 
 TEST_F(SyncModelLoadManagerTest, SwitchFromOnDiskToInMemoryRestartsTypes) {
-  // Associate model with two data types.
+  // Initialize the manager with two data types.
   controllers_[BOOKMARKS] = std::make_unique<FakeDataTypeController>(
       BOOKMARKS, /*enable_transport_only_model=*/true);
   controllers_[APPS] = std::make_unique<FakeDataTypeController>(
@@ -451,7 +450,7 @@ TEST_F(SyncModelLoadManagerTest, SwitchFromOnDiskToInMemoryRestartsTypes) {
 
 TEST_F(SyncModelLoadManagerTest,
        SwitchFromTransportOnlyToFullSyncRestartsTypes) {
-  // Associate model with two data types.
+  // Initialize the manager with two data types.
   controllers_[BOOKMARKS] = std::make_unique<FakeDataTypeController>(
       BOOKMARKS, /*enable_transport_only_model=*/true);
   controllers_[APPS] = std::make_unique<FakeDataTypeController>(

components/sync/engine/data_type_debug_info_listener.h

@@ -26,8 +26,8 @@ struct DataTypeConfigurationStats {
   // Time spent on downloading data for first-sync data types.
   base::TimeDelta download_time;
 
-  // Waiting time for association of higher priority types to finish before
-  // asking association manager to associate.
+  // TODO(crbug.com/1102837): The concept of "association" doesn't exist
+  // anymore, and this is effectively always zero now. Get rid of it.
   base::TimeDelta association_wait_time_for_high_priority;
 
   // Types configured before this type.