Merge 108620 - Add only types with empty progress marker to config requests. This fixes a bug where in case of a newly enabled datatype we would end up adding all the datatypes to the config request.

With this change Nigori would not be part of the requested types unless it has progress marker token to be empty. So we should not get migration error on config requests at all. In case we do it would be a unrecoverable error which is fine.


BUG=102557
TEST=


Review URL: http://codereview.chromium.org/8439057

TBR=lipalani@chromium.org
Review URL: http://codereview.chromium.org/8477016

git-svn-id: svn://svn.chromium.org/chrome/branches/912/src@108729 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 50 insertions, 6 deletions

diff --git a/chrome/browser/sync/glue/sync_backend_host.cc b/chrome/browser/sync/glue/sync_backend_host.cc
index 0534770..51ac4a4d 100644
--- a/chrome/browser/sync/glue/sync_backend_host.cc
+++ b/chrome/browser/sync/glue/sync_backend_host.cc
@@ -964,14 +964,25 @@ void SyncBackendHost::FinishConfigureDataTypesOnFrontendLoop() {
 
   if (pending_config_mode_state_->added_types.empty() &&
       !core_->sync_manager()->InitialSyncEndedForAllEnabledTypes()) {
-    SLOG(WARNING) << "No new types, but initial sync not finished."
-                  << "Possible sync db corruption / removal.";
+
+    syncable::ModelTypeSet enabled_types;
+    ModelSafeRoutingInfo routing_info;
+    registrar_->GetModelSafeRoutingInfo(&routing_info);
+    for (ModelSafeRoutingInfo::const_iterator i = routing_info.begin();
+         i != routing_info.end(); ++i) {
+      enabled_types.insert(i->first);
+    }
+
     // TODO(tim): Log / UMA / count this somehow?
-    // TODO(tim): If no added types, we could (should?) config only for
-    // types that are needed... but this is a rare corruption edge case or
-    // implies the user mucked around with their syncdb, so for now do all.
+    // Add only the types with empty progress markers. Note: it is possible
+    // that some types have their initial_sync_ended be false but with non
+    // empty progress marker. Which is ok as the rest of the changes would
+    // be downloaded on a regular nudge and initial_sync_ended should be set
+    // to true. However this is a very corner case. So it is not explicitly
+    // handled.
     pending_config_mode_state_->added_types =
-        pending_config_mode_state_->types_to_add;
+        sync_api::GetTypesWithEmptyProgressMarkerToken(enabled_types,
+                                                       GetUserShare());
   }
 
   // If we've added types, we always want to request a nudge/config (even if
@@ -987,6 +998,11 @@ void SyncBackendHost::FinishConfigureDataTypesOnFrontendLoop() {
     syncable::ModelTypeSet types_to_config =
         pending_download_state_->added_types;
     if (IsNigoriEnabled()) {
+      // Note: Nigori is the only type that gets added with a nonempty
+      // progress marker during config. If the server returns a migration
+      // error then we will go into unrecoverable error. We dont handle it
+      // explicitly because server might help us out here by not sending a
+      // migraiton error for nigori during config.
       types_to_config.insert(syncable::NIGORI);
     }
     SVLOG(1) << "Types " << ModelTypeSetToString(types_to_config)
diff --git a/chrome/browser/sync/internal_api/sync_manager.cc b/chrome/browser/sync/internal_api/sync_manager.cc
index 5b68289..5d3a394 100644
--- a/chrome/browser/sync/internal_api/sync_manager.cc
+++ b/chrome/browser/sync/internal_api/sync_manager.cc
@@ -2094,4 +2094,28 @@ bool InitialSyncEndedForTypes(syncable::ModelTypeSet types,
   return true;
 }
 
+syncable::ModelTypeSet GetTypesWithEmptyProgressMarkerToken(
+    const syncable::ModelTypeSet types,
+    sync_api::UserShare* share) {
+  syncable::ScopedDirLookup lookup(share->dir_manager.get(),
+                                   share->name);
+  if (!lookup.good()) {
+    NOTREACHED() << "ScopedDirLookup failed for "
+                  << "GetTypesWithEmptyProgressMarkerToken";
+    return syncable::ModelTypeSet();
+  }
+
+  syncable::ModelTypeSet result;
+  for (syncable::ModelTypeSet::const_iterator i = types.begin();
+       i != types.end(); ++i) {
+    sync_pb::DataTypeProgressMarker marker;
+    lookup->GetDownloadProgress(*i, &marker);
+
+    if (marker.token().empty())
+      result.insert(*i);
+
+  }
+  return result;
+}
+
 }  // namespace sync_api
diff --git a/chrome/browser/sync/internal_api/sync_manager.h b/chrome/browser/sync/internal_api/sync_manager.h
index 650893d..90955a2 100644
--- a/chrome/browser/sync/internal_api/sync_manager.h
+++ b/chrome/browser/sync/internal_api/sync_manager.h
@@ -569,6 +569,10 @@ class SyncManager {
 
 bool InitialSyncEndedForTypes(syncable::ModelTypeSet types, UserShare* share);
 
+syncable::ModelTypeSet GetTypesWithEmptyProgressMarkerToken(
+    const syncable::ModelTypeSet types,
+    sync_api::UserShare* share);
+
 // Returns the string representation of a PassphraseRequiredReason value.
 std::string PassphraseRequiredReasonToString(PassphraseRequiredReason reason);