[Sync] - Implement isolated model association.

chrome/browser/sync/glue/model_association_manager.cc

Index: chrome/browser/sync/glue/model_association_manager.cc
diff --git a/chrome/browser/sync/glue/model_association_manager.cc b/chrome/browser/sync/glue/model_association_manager.cc
index 18a5e14332f29360fe8f57990c527b340d3678ec..879078b47e4b7ae0257e14a4b7a2b6de0a67ed90 100755
--- a/chrome/browser/sync/glue/model_association_manager.cc
+++ b/chrome/browser/sync/glue/model_association_manager.cc
@@ -19,6 +19,7 @@ using content::BrowserThread;
 using syncable::ModelTypeSet;
 
 namespace browser_sync {
+const int64 kDataTypeLoadWaitTimeInSeconds = 120;
 namespace {
 
 static const syncable::ModelType kStartOrder[] = {
@@ -94,6 +95,21 @@ void ModelAssociationManager::Initialize(
   desired_types_ = desired_types;
   state_ = INITIAILIZED_TO_CONFIGURE;
 
+  // Stop the types that are still loading from the previous configuration.
+  // If they are enabled we will start them here once again.
+
+  for (std::vector<DataTypeController*>::const_iterator it =
+           pending_model_load_.begin();
+       it != pending_model_load_.end();
+       ++it) {
+    (*it)->Stop();
+  }
+
+  pending_model_load_.clear();
+  waiting_to_associate_.clear();
+  currently_associating_ = NULL;
+
+
   // We need to calculate our |needs_start_| and |needs_stop_| list.
   GetControllersNeedingStart(&needs_start_);
   // Sort these according to kStartOrder.
@@ -178,7 +194,8 @@ void ModelAssociationManager::Stop() {
   if (need_to_call_model_association_done) {
     DataTypeManager::ConfigureResult result(DataTypeManager::ABORTED,
                                             desired_types_,
-                                            failed_datatypes_info_);
+                                            failed_datatypes_info_,
+                                            syncable::ModelTypeSet());
     result_processor_->OnModelAssociationDone(result);
   }
 
@@ -209,6 +226,18 @@ bool ModelAssociationManager::GetControllersNeedingStart(
   return found_any;
 }
 
+void ModelAssociationManager::HandleFailedTypes(
+    DataTypeController::StartResult result,
+    const SyncError& error) {
+  failed_datatypes_info_.push_back(error);
+  LOG(ERROR) << "Failed to associate models for "
+             << syncable::ModelTypeToString(error.type());
+  UMA_HISTOGRAM_ENUMERATION("Sync.ConfigureFailed",
+                            error.type(),
+                            syncable::MODEL_TYPE_COUNT);
+
+}
+
 void ModelAssociationManager::TypeStartCallback(
     DataTypeController::StartResult result,
     const SyncError& error) {
@@ -228,12 +257,7 @@ void ModelAssociationManager::TypeStartCallback(
   currently_associating_ = NULL;
 
   if (result == DataTypeController::ASSOCIATION_FAILED) {
-    failed_datatypes_info_.push_back(error);
-    LOG(ERROR) << "Failed to associate models for "
-               << syncable::ModelTypeToString(error.type());
-    UMA_HISTOGRAM_ENUMERATION("Sync.ConfigureFailed",
-                              error.type(),
-                              syncable::MODEL_TYPE_COUNT);
+    HandleFailedTypes(result, error);
   }
 
   // If the type started normally, continue to the next type.
@@ -274,7 +298,8 @@ void ModelAssociationManager::TypeStartCallback(
 
   DataTypeManager::ConfigureResult configure_result(configure_status,
                                                     desired_types_,
-                                                    errors);
+                                                    errors,
+                                                    syncable::ModelTypeSet());
   result_processor_->OnModelAssociationDone(configure_result);
 }
 
@@ -285,10 +310,15 @@ void ModelAssociationManager::LoadModelForNextType() {
     DataTypeController* dtc = needs_start_[0];
     needs_start_.erase(needs_start_.begin());
     // Move from |needs_start_| to |pending_model_load_|.
-    pending_model_load_.push_back(dtc);
+    pending_model_load_.insert(pending_model_load_.begin(), dtc);
+    timer_.Start(FROM_HERE,
+                 base::TimeDelta::FromSeconds(kDataTypeLoadWaitTimeInSeconds),
+                 this,
+                 &ModelAssociationManager::ModelLoadTimedOut);
     dtc->LoadModels(base::Bind(
         &ModelAssociationManager::ModelLoadCallback,
         weak_ptr_factory_.GetWeakPtr()));
+
     return;
   }
 
@@ -299,29 +329,38 @@ void ModelAssociationManager::LoadModelForNextType() {
 
 void ModelAssociationManager::ModelLoadCallback(
     syncable::ModelType type, SyncError error) {
-  DCHECK_EQ(state_, CONFIGURING);
-
-  for (std::vector<DataTypeController*>::iterator it =
-           pending_model_load_.begin();
-        it != pending_model_load_.end();
-        ++it) {
-    if ((*it)->type() == type) {
-      DataTypeController* dtc = *it;
-      pending_model_load_.erase(it);
-      if (!error.IsSet()) {
-        waiting_to_associate_.push_back(dtc);
-        StartAssociatingNextType();
-     } else {
-        // Treat it like a regular error.
-        DCHECK(currently_associating_ == NULL);
-        currently_associating_ = dtc;
-        TypeStartCallback(DataTypeController::ASSOCIATION_FAILED, error);
-     }
-     return;
+  if (state_ == CONFIGURING) {
+    for (std::vector<DataTypeController*>::iterator it =
+             pending_model_load_.begin();
+          it != pending_model_load_.end();
+          ++it) {
+      if ((*it)->type() == type) {
+        // If the type that got loaded was the type that was started

[tim (not reviewing), 2012/05/21 23:18:57]

'If the type that just finished loading was the last type to start, we can
safely stop the timer.'

is that what you're trying to say?  And this is true because the types run
serially and not in parallel -- I'd mention that as a final sentence on your
comment.

[lipalani1, 2012/05/22 01:23:58]

Done.

+        // last, then we could stop the timer.
+        if (it == pending_model_load_.begin()) {
+          timer_.Stop();
+        }
+        DataTypeController* dtc = *it;
+        pending_model_load_.erase(it);
+        if (!error.IsSet()) {
+          waiting_to_associate_.push_back(dtc);
+          StartAssociatingNextType();
+       } else {
+         // Treat it like a regular error.
+         HandleFailedTypes(DataTypeController::ASSOCIATION_FAILED, error);
+       }
+       return;
+      }
     }
+    NOTREACHED();
+    return;
+  }
+  else {
+    // We got a callback on type that we started loading during the

[tim (not reviewing), 2012/05/21 23:18:57]

"This data type finished loading after the deadline imposed by the originating
configuration cycle.  Inform the DataTypeManager that the type has loaded, so
that association may begin."

- don't use DTM / abbreviations in comments, helps for grep etc.

[lipalani1, 2012/05/22 01:23:58]

Done.

+    // previous configuration. Inform the DTM that the type has loaded.
+    result_processor_->OnTypesLoaded();
   }
 
-  NOTREACHED();
 }
 
 
@@ -352,22 +391,47 @@ void ModelAssociationManager::StartAssociatingNextType() {
     DataTypeManager::ConfigureResult configure_result(
         DataTypeManager::CONFIGURE_BLOCKED,
         desired_types_,
-        failed_datatypes_info_);
+        failed_datatypes_info_,
+        syncable::ModelTypeSet());
     state_ = IDLE;
     result_processor_->OnModelAssociationDone(configure_result);
     return;
   }
 
   DataTypeManager::ConfigureStatus configure_status = DataTypeManager::OK;
-  if (!failed_datatypes_info_.empty()) {
+  if (!failed_datatypes_info_.empty() ||
+      !GetTypesWaitingToLoad().Empty()) {
     configure_status = DataTypeManager::PARTIAL_SUCCESS;
   }
   DataTypeManager::ConfigureResult result(configure_status,
                                           desired_types_,
-                                          failed_datatypes_info_);
+                                          failed_datatypes_info_,
+                                          GetTypesWaitingToLoad());
   result_processor_->OnModelAssociationDone(result);
   return;
 }
 
+void ModelAssociationManager::ModelLoadTimedOut() {

[tim (not reviewing), 2012/05/21 23:18:57]

Is this method actually needed?

[lipalani1, 2012/05/22 01:23:58]

Just for clarity purpose... Compiler probably optimizes this any way.
On 2012/05/21 23:18:57, timsteele wrote:

[tim (not reviewing), 2012/05/22 16:50:05]

Hmm, to me the extra indirection makes it less clear.  It's also easier for
someone to sneak in code that does something different in the timeout case, and
I'd like that to be a Big Event / called out in a future code review.

[lipalani1, 2012/05/22 20:20:46]

Done.

+  LoadModelForNextType();
+}
+
+syncable::ModelTypeSet ModelAssociationManager::GetTypesWaitingToLoad() {
+  syncable::ModelTypeSet result;
+  for (std::vector<DataTypeController*>::const_iterator it =
+           pending_model_load_.begin();
+       it != pending_model_load_.end();
+       ++it) {
+    result.Put((*it)->type());
+  }
+  return result;
+}
+
+void ModelAssociationManager::TestSimulateDataTypeLoadTimeout() {

[tim (not reviewing), 2012/05/21 23:18:57]

I think there is a way to do this that doesn't require adding test only
functionality like this and the model_association_manager() accessor, explained
below.  Note otherwise however the style guide stipulates that this method and
the accessor should be suffixed with "ForTesting", to ensure compatibility with
presubmit script.

One better option to remove the accessor by injecting ModelAssociationManager
into the DataTypeManager. From there, I would suggest one of the following

- Have ModelAssociationManager take a Timer on construction, since this is what
you ultimately want control over in your test (common enough). You can use
timer.user_task and timer.stop to get what you want.
- Stick SimulateDataTypeLoadTimeout on a test-only subclass that you would
inject from your test.

Another option is to use FRIEND_TEST_ALL_PREFIXES.  It appears only one test
needs this special behavior, but this is less ideal (especially given that you
have to dig through  a class to get here! I think injection is the right
answer.)

[lipalani1, 2012/05/22 01:23:58]

I think the eventual direction you are advocating is moving towards dependancy
injection. Which I think is cool and all of our code should adhere to.

We will have to change few components including PSS to inject the MAM class into
DTM. (and this is true of most components we have). I think modifying those
components is better as a different change list. and should be done with DI as
the goal. 

The other option is to make the timer injectable from DTM. But the problem with
that is test cases in DTM would still need an accessor.

For now I have taken the suggestion of user_task_ and created a test only
accessor for timer. (The reason this is better is: Accessors  gets us half way
to DI. Instead of injecting we are extracting. However the code that manipulates
the object that is being extracted(or injected) would remain the same even after
the move to DI. Another advantage of this is that we are only using the public
methods of the object extracted(injected) which is slightly preferable than
writing derived classes.)
On 2012/05/21 23:18:57, timsteele wrote:

[tim (not reviewing), 2012/05/22 16:50:05]

It sounds like we're in agreement that it would be better to inject the
parameters, so I guess if you're not doing it here this new code should be
accompanied by a TODO and a bug on file tracking the fact that it is incomplete.

In the meantime, the accessor for model_association_manager() should be called
GetModelAssociationManagerForTesting().  My main concern is other components
enforcing an unnecessary dependency / requirement that the DataTypeManager
exposes ModelAssociationManager.

[lipalani1, 2012/05/22 20:20:46]

I have addressed this. Let me know what you think.
On 2012/05/22 16:50:05, timsteele wrote:

+  timer_.Stop();
+  ModelLoadTimedOut();
+}
+
+
+
 }  // namespace browser_sync