Revert 167280 - [Sync] Add application logic for non-nigori control types.

We introduce logic to apply non-nigori control types. First, we iterate over
any new top level datatype entities, applying those. Then we go through the
rest of the unapplied control datatype updates, applying those. Any conflict
should be just a simple conflict, which we handle by ignoring the local
changes.

Also updates chromiumsync.py to support the new control types (and fixes
the parent folder pattern that was in use).

BUG=122825


Review URL: https://chromiumcodereview.appspot.com/11271009

TBR=zea@chromium.org
Review URL: https://codereview.chromium.org/11359175

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@167282 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 45 insertions, 195 deletions

diff --git a/sync/engine/apply_control_data_updates.cc b/sync/engine/apply_control_data_updates.cc
index 2354c48..d0f182d 100644
--- a/sync/engine/apply_control_data_updates.cc
+++ b/sync/engine/apply_control_data_updates.cc
@@ -24,74 +24,15 @@ using syncable::SERVER_SPECIFICS;
 using syncable::SPECIFICS;
 using syncable::SYNCER;
 
-void ApplyControlDataUpdates(sessions::SyncSession* session) {
-  syncable::Directory* dir = session->context()->directory();
+void ApplyControlDataUpdates(syncable::Directory* dir) {
   syncable::WriteTransaction trans(FROM_HERE, SYNCER, dir);
 
-  std::vector<int64> handles;
-  dir->GetUnappliedUpdateMetaHandles(
-      &trans, ToFullModelTypeSet(ControlTypes()), &handles);
-
-  // First, go through and manually apply any new top level datatype nodes (so
-  // that we don't have to worry about hitting a CONFLICT_HIERARCHY with an
-  // entry because we haven't applied its parent yet).
-  // TODO(sync): if at some point we support control datatypes with actual
-  // hierarchies we'll need to revisit this logic.
-  for (ModelTypeSet::Iterator iter = ControlTypes().First(); iter.Good();
-       iter.Inc()) {
-    syncable::MutableEntry entry(&trans,
-                                 syncable::GET_BY_SERVER_TAG,
-                                 ModelTypeToRootTag(iter.Get()));
-    if (!entry.good())
-      continue;
-    if (!entry.Get(syncable::IS_UNAPPLIED_UPDATE))
-      continue;
-
-    ModelType type = entry.GetServerModelType();
-    if (type == NIGORI) {
-      // Nigori node applications never fail.
-      ApplyNigoriUpdate(&trans,
-                        &entry,
-                        dir->GetCryptographer(&trans));
-    } else {
-      ApplyControlUpdate(&trans,
-                         &entry,
-                         dir->GetCryptographer(&trans));
-    }
-  }
-
-  // Go through the rest of the unapplied control updates, skipping over any
-  // top level folders.
-  for (std::vector<int64>::const_iterator iter = handles.begin();
-       iter != handles.end(); ++iter) {
-    syncable::MutableEntry entry(&trans, syncable::GET_BY_HANDLE, *iter);
-    CHECK(entry.good());
-    ModelType type = entry.GetServerModelType();
-    CHECK(ControlTypes().Has(type));
-    if (!entry.Get(syncable::UNIQUE_SERVER_TAG).empty()) {
-      // We should have already applied all top level control nodes.
-      DCHECK(!entry.Get(syncable::IS_UNAPPLIED_UPDATE));
-      continue;
-    }
-
-    ApplyControlUpdate(&trans,
-                       &entry,
-                       dir->GetCryptographer(&trans));
-  }
-
-  // Set initial sync ended bits for all control types requested.
-  for (ModelTypeSet::Iterator it =
-           session->status_controller().updates_request_types().First();
-       it.Good(); it.Inc()) {
-    if (!IsControlType(it.Get()))
-        continue;
-
-    // This gets persisted to the directory's backing store.
-    dir->set_initial_sync_ended_for_type(it.Get(), true);
+  if (ApplyNigoriUpdates(&trans, dir->GetCryptographer(&trans))) {
+    dir->set_initial_sync_ended_for_type(NIGORI, true);
   }
 }
 
-// Update the nigori handler with the server's nigori node.
+// Update the sync encryption handler with the server's nigori node.
 //
 // If we have a locally modified nigori node, we merge them manually. This
 // handles the case where two clients both set a different passphrase. The
@@ -102,18 +43,24 @@ void ApplyControlDataUpdates(sessions::SyncSession* session) {
 // passphrase, the cryptographer will preserve the encryption keys based on the
 // local passphrase, while the nigori node will preserve the server encryption
 // keys.
-void ApplyNigoriUpdate(syncable::WriteTransaction* const trans,
-                       syncable::MutableEntry* const entry,
-                       Cryptographer* cryptographer) {
-  DCHECK(entry->Get(IS_UNAPPLIED_UPDATE));
+bool ApplyNigoriUpdates(syncable::WriteTransaction* trans,
+                        Cryptographer* cryptographer) {
+  syncable::MutableEntry nigori_node(trans, GET_BY_SERVER_TAG,
+                                     ModelTypeToRootTag(NIGORI));
+
+  // Mainly for unit tests.  We should have a Nigori node by this point.
+  if (!nigori_node.good()) {
+    return false;
+  }
+
+  if (!nigori_node.Get(IS_UNAPPLIED_UPDATE)) {
+    return true;
+  }
 
   // We apply the nigori update regardless of whether there's a conflict or
   // not in order to preserve any new encrypted types or encryption keys.
-  // TODO(zea): consider having this return a bool reflecting whether it was a
-  // valid update or not, and in the case of invalid updates not overwrite the
-  // local data.
   const sync_pb::NigoriSpecifics& nigori =
-      entry->Get(SERVER_SPECIFICS).nigori();
+      nigori_node.Get(SERVER_SPECIFICS).nigori();
   trans->directory()->GetNigoriHandler()->ApplyNigoriUpdate(nigori, trans);
 
   // Make sure any unsynced changes are properly encrypted as necessary.
@@ -135,19 +82,19 @@ void ApplyNigoriUpdate(syncable::WriteTransaction* const trans,
     syncable::ProcessUnsyncedChangesForEncryption(trans);
   }
 
-  if (!entry->Get(IS_UNSYNCED)) {  // Update only.
-    UpdateLocalDataFromServerData(trans, entry);
+  if (!nigori_node.Get(IS_UNSYNCED)) {  // Update only.
+    UpdateLocalDataFromServerData(trans, &nigori_node);
   } else {  // Conflict.
     const sync_pb::EntitySpecifics& server_specifics =
-        entry->Get(SERVER_SPECIFICS);
+        nigori_node.Get(SERVER_SPECIFICS);
     const sync_pb::NigoriSpecifics& server_nigori = server_specifics.nigori();
     const sync_pb::EntitySpecifics& local_specifics =
-        entry->Get(SPECIFICS);
+        nigori_node.Get(SPECIFICS);
     const sync_pb::NigoriSpecifics& local_nigori = local_specifics.nigori();
 
     // We initialize the new nigori with the server state, and will override
     // it as necessary below.
-    sync_pb::EntitySpecifics new_specifics = entry->Get(SERVER_SPECIFICS);
+    sync_pb::EntitySpecifics new_specifics = nigori_node.Get(SERVER_SPECIFICS);
     sync_pb::NigoriSpecifics* new_nigori = new_specifics.mutable_nigori();
 
     // If the cryptographer is not ready, another client set a new encryption
@@ -198,35 +145,18 @@ void ApplyNigoriUpdate(syncable::WriteTransaction* const trans,
         new_nigori,
         trans);
 
-    entry->Put(SPECIFICS, new_specifics);
+    nigori_node.Put(SPECIFICS, new_specifics);
     DVLOG(1) << "Resolving simple conflict, merging nigori nodes: "
-             << entry;
+             << nigori_node;
 
-    conflict_util::OverwriteServerChanges(entry);
+    conflict_util::OverwriteServerChanges(&nigori_node);
 
     UMA_HISTOGRAM_ENUMERATION("Sync.ResolveSimpleConflict",
                               ConflictResolver::NIGORI_MERGE,
                               ConflictResolver::CONFLICT_RESOLUTION_SIZE);
   }
-}
-
-void ApplyControlUpdate(syncable::WriteTransaction* const trans,
-                        syncable::MutableEntry* const entry,
-                        Cryptographer* cryptographer) {
-  DCHECK_NE(entry->GetServerModelType(), NIGORI);
-  DCHECK(entry->Get(IS_UNAPPLIED_UPDATE));
-  if (entry->Get(IS_UNSYNCED)) {
-      // We just let the server win all conflicts with control types.
-    DVLOG(1) << "Ignoring local changes for control update.";
-    conflict_util::IgnoreLocalChanges(entry);
-    UMA_HISTOGRAM_ENUMERATION("Sync.ResolveSimpleConflict",
-                              ConflictResolver::OVERWRITE_LOCAL,
-                              ConflictResolver::CONFLICT_RESOLUTION_SIZE);
-  }
 
-  UpdateAttemptResponse response = AttemptToUpdateEntry(
-      trans, entry, cryptographer);
-  DCHECK_EQ(SUCCESS, response);
+  return true;
 }
 
 }  // namespace syncer
diff --git a/sync/engine/apply_control_data_updates.h b/sync/engine/apply_control_data_updates.h
index 8d19f47..efe2cc8 100644
--- a/sync/engine/apply_control_data_updates.h
+++ b/sync/engine/apply_control_data_updates.h
@@ -9,22 +9,13 @@ namespace syncer {
 
 class Cryptographer;
 
-namespace sessions {
-class SyncSession;
-}
-
 namespace syncable {
 class Directory;
-class MutableEntry;
 class WriteTransaction;
 }
 
-void ApplyControlDataUpdates(sessions::SyncSession* session);
-void ApplyNigoriUpdate(syncable::WriteTransaction* trans,
-                       syncable::MutableEntry* const entry,
-                       Cryptographer* cryptographer);
-void ApplyControlUpdate(syncable::WriteTransaction* const trans,
-                        syncable::MutableEntry* const entry,
+void ApplyControlDataUpdates(syncable::Directory* dir);
+bool ApplyNigoriUpdates(syncable::WriteTransaction* trans,
                         Cryptographer* cryptographer);
 
 }  // namespace syncer
diff --git a/sync/engine/apply_control_data_updates_unittest.cc b/sync/engine/apply_control_data_updates_unittest.cc
index 09c5e11..aa00865 100644
--- a/sync/engine/apply_control_data_updates_unittest.cc
+++ b/sync/engine/apply_control_data_updates_unittest.cc
@@ -41,14 +41,12 @@ class ApplyControlDataUpdatesTest : public SyncerCommandTest {
     workers()->push_back(make_scoped_refptr(new FakeModelWorker(GROUP_UI)));
     workers()->push_back(
         make_scoped_refptr(new FakeModelWorker(GROUP_PASSWORD)));
+    (*mutable_routing_info())[BOOKMARKS] = GROUP_UI;
+    (*mutable_routing_info())[PASSWORDS] = GROUP_PASSWORD;
     (*mutable_routing_info())[NIGORI] = GROUP_PASSIVE;
-    (*mutable_routing_info())[EXPERIMENTS] = GROUP_PASSIVE;
     SyncerCommandTest::SetUp();
     entry_factory_.reset(new TestEntryFactory(directory()));
 
-    session()->mutable_status_controller()->set_updates_request_types(
-        ControlTypes());
-
     syncable::ReadTransaction trans(FROM_HERE, directory());
   }
 
@@ -92,7 +90,7 @@ TEST_F(ApplyControlDataUpdatesTest, NigoriUpdate) {
       ModelTypeToRootTag(NIGORI), specifics, true);
   EXPECT_FALSE(cryptographer->has_pending_keys());
 
-  ApplyControlDataUpdates(session());
+  ApplyControlDataUpdates(directory());
 
   EXPECT_FALSE(cryptographer->is_ready());
   EXPECT_TRUE(cryptographer->has_pending_keys());
@@ -172,7 +170,7 @@ TEST_F(ApplyControlDataUpdatesTest, EncryptUnsyncedChanges) {
     EXPECT_EQ(2*batch_s+1, handles.size());
   }
 
-  ApplyControlDataUpdates(session());
+  ApplyControlDataUpdates(directory());
 
   EXPECT_FALSE(cryptographer->has_pending_keys());
   EXPECT_TRUE(cryptographer->is_ready());
@@ -200,7 +198,7 @@ TEST_F(ApplyControlDataUpdatesTest, EncryptUnsyncedChanges) {
     entry.Put(syncable::IS_UNAPPLIED_UPDATE, true);
   }
 
-  ApplyControlDataUpdates(session());
+  ApplyControlDataUpdates(directory());
 
   EXPECT_FALSE(cryptographer->has_pending_keys());
   EXPECT_TRUE(cryptographer->is_ready());
@@ -287,7 +285,7 @@ TEST_F(ApplyControlDataUpdatesTest, CannotEncryptUnsyncedChanges) {
     EXPECT_EQ(2*batch_s+1, handles.size());
   }
 
-  ApplyControlDataUpdates(session());
+  ApplyControlDataUpdates(directory());
 
   EXPECT_FALSE(cryptographer->is_ready());
   EXPECT_TRUE(cryptographer->has_pending_keys());
@@ -365,7 +363,7 @@ TEST_F(ApplyControlDataUpdatesTest,
 
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_TRUE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
-  ApplyControlDataUpdates(session());
+  ApplyControlDataUpdates(directory());
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
 
@@ -444,7 +442,7 @@ TEST_F(ApplyControlDataUpdatesTest,
 
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_TRUE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
-  ApplyControlDataUpdates(session());
+  ApplyControlDataUpdates(directory());
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
 
@@ -518,7 +516,7 @@ TEST_F(ApplyControlDataUpdatesTest,
 
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_TRUE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
-  ApplyControlDataUpdates(session());
+  ApplyControlDataUpdates(directory());
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
 
@@ -598,7 +596,7 @@ TEST_F(ApplyControlDataUpdatesTest,
 
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_TRUE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
-  ApplyControlDataUpdates(session());
+  ApplyControlDataUpdates(directory());
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
 
@@ -681,7 +679,7 @@ TEST_F(ApplyControlDataUpdatesTest,
 
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_TRUE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
-  ApplyControlDataUpdates(session());
+  ApplyControlDataUpdates(directory());
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
 
@@ -762,7 +760,7 @@ TEST_F(ApplyControlDataUpdatesTest,
 
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_TRUE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
-  ApplyControlDataUpdates(session());
+  ApplyControlDataUpdates(directory());
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
 
@@ -843,7 +841,7 @@ TEST_F(ApplyControlDataUpdatesTest,
 
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_TRUE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
-  ApplyControlDataUpdates(session());
+  ApplyControlDataUpdates(directory());
   EXPECT_TRUE(entry_factory_->GetIsUnsyncedForItem(nigori_handle));
   EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(nigori_handle));
 
@@ -870,72 +868,4 @@ TEST_F(ApplyControlDataUpdatesTest,
   }
 }
 
-// Check that we can apply a simple control datatype node successfully.
-TEST_F(ApplyControlDataUpdatesTest, ControlApply) {
-  EXPECT_FALSE(directory()->initial_sync_ended_types().Has(EXPERIMENTS));
-
-  std::string experiment_id = "experiment";
-  sync_pb::EntitySpecifics specifics;
-  specifics.mutable_experiments()->mutable_keystore_encryption()->
-      set_enabled(true);
-  int64 experiment_handle = entry_factory_->CreateUnappliedNewItem(
-      experiment_id, specifics, false);
-  ApplyControlDataUpdates(session());
-
-  EXPECT_TRUE(directory()->initial_sync_ended_types().Has(EXPERIMENTS));
-  EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(experiment_handle));
-  EXPECT_TRUE(
-      entry_factory_->GetLocalSpecificsForItem(experiment_handle).
-          experiments().keystore_encryption().enabled());
-}
-
-// Verify that we apply top level folders before their children.
-TEST_F(ApplyControlDataUpdatesTest, ControlApplyParentBeforeChild) {
-  EXPECT_FALSE(directory()->initial_sync_ended_types().Has(EXPERIMENTS));
-
-  std::string parent_id = "parent";
-  std::string experiment_id = "experiment";
-  sync_pb::EntitySpecifics specifics;
-  specifics.mutable_experiments()->mutable_keystore_encryption()->
-      set_enabled(true);
-  int64 experiment_handle = entry_factory_->CreateUnappliedNewItemWithParent(
-      experiment_id, specifics, parent_id);
-  int64 parent_handle = entry_factory_->CreateUnappliedNewItem(
-      parent_id, specifics, true);
-  ApplyControlDataUpdates(session());
-
-  EXPECT_TRUE(directory()->initial_sync_ended_types().Has(EXPERIMENTS));
-  EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(parent_handle));
-  EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(experiment_handle));
-  EXPECT_TRUE(
-      entry_factory_->GetLocalSpecificsForItem(experiment_handle).
-          experiments().keystore_encryption().enabled());
-}
-
-// Verify that we handle control datatype conflicts by preserving the server
-// data.
-TEST_F(ApplyControlDataUpdatesTest, ControlConflict) {
-  EXPECT_FALSE(directory()->initial_sync_ended_types().Has(EXPERIMENTS));
-
-  std::string experiment_id = "experiment";
-  sync_pb::EntitySpecifics local_specifics, server_specifics;
-  server_specifics.mutable_experiments()->mutable_keystore_encryption()->
-      set_enabled(true);
-  local_specifics.mutable_experiments()->mutable_keystore_encryption()->
-      set_enabled(false);
-  int64 experiment_handle = entry_factory_->CreateSyncedItem(
-      experiment_id, EXPERIMENTS, false);
-  entry_factory_->SetServerSpecificsForItem(experiment_handle,
-                                            server_specifics);
-  entry_factory_->SetLocalSpecificsForItem(experiment_handle,
-                                           local_specifics);
-  ApplyControlDataUpdates(session());
-
-  EXPECT_TRUE(directory()->initial_sync_ended_types().Has(EXPERIMENTS));
-  EXPECT_FALSE(entry_factory_->GetIsUnappliedForItem(experiment_handle));
-  EXPECT_TRUE(
-      entry_factory_->GetLocalSpecificsForItem(experiment_handle).
-          experiments().keystore_encryption().enabled());
-}
-
 }  // namespace syncer
diff --git a/sync/engine/syncer.cc b/sync/engine/syncer.cc
index 90d193a..22010ee 100644
--- a/sync/engine/syncer.cc
+++ b/sync/engine/syncer.cc
@@ -144,7 +144,7 @@ bool Syncer::SyncShare(sessions::SyncSession* session,
       }
       case APPLY_UPDATES: {
         // These include encryption updates that should be applied early.
-        ApplyControlDataUpdates(session);
+        ApplyControlDataUpdates(session->context()->directory());
 
         ApplyUpdatesAndResolveConflictsCommand apply_updates;
         apply_updates.Execute(session);
diff --git a/sync/engine/syncer_util.cc b/sync/engine/syncer_util.cc
index b88bc7c..cdc1b7a 100644
--- a/sync/engine/syncer_util.cc
+++ b/sync/engine/syncer_util.cc
@@ -209,7 +209,7 @@ UpdateAttemptResponse AttemptToUpdateEntry(
     // We can't decrypt this node yet.
     DVLOG(1) << "Received an undecryptable "
              << ModelTypeToString(entry->GetServerModelType())
-             << " update, returning conflict_encryption.";
+             << " update, returning encryption_conflict.";
     return CONFLICT_ENCRYPTION;
   } else if (specifics.has_password() &&
              entry->Get(UNIQUE_SERVER_TAG).empty()) {
@@ -217,7 +217,7 @@ UpdateAttemptResponse AttemptToUpdateEntry(
     const sync_pb::PasswordSpecifics& password = specifics.password();
     if (!cryptographer->CanDecrypt(password.encrypted())) {
       DVLOG(1) << "Received an undecryptable password update, returning "
-               << "conflict_encryption.";
+               << "encryption_conflict.";
       return CONFLICT_ENCRYPTION;
     }
   }
@@ -232,7 +232,6 @@ UpdateAttemptResponse AttemptToUpdateEntry(
     // different ways we deal with it once here to reduce the amount of code and
     // potential errors.
     if (!parent.good() || parent.Get(IS_DEL) || !parent.Get(IS_DIR)) {
-      DVLOG(1) <<  "Entry has bad parent, returning conflict_hierarchy.";
       return CONFLICT_HIERARCHY;
     }
     if (entry->Get(PARENT_ID) != new_parent) {