sync: Remove the remaining conflict sets code

chrome/browser/sync/engine/apply_updates_command_unittest.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <string>
 
 #include "base/format_macros.h"
 #include "base/location.h"
 #include "base/stringprintf.h"
 #include "chrome/browser/sync/engine/apply_updates_command.h"
@@ skipping 125 matching lines @@
     if (item_id.ServerKnows()) {
       entry.Put(syncable::SERVER_SPECIFICS, default_specifics);
       entry.Put(syncable::SERVER_IS_DIR, is_folder);
       entry.Put(syncable::SERVER_PARENT_ID, parent_id);
       entry.Put(syncable::SERVER_IS_DEL, false);
     }
     if (metahandle_out)
       *metahandle_out = entry.Get(syncable::META_HANDLE);
   }
 
+  // Creates an item that is both unsynced an an unapplied update.  The item_id
+  // parameter must be a server ID.  The metahandle of the created item will be
+  // output to metahandle_out.  Returns the metahandle of the created item in
+  // metahandle_out if not NULL.
+  void CreateUnappliedAndUnsyncedItem(const Id& item_id,

[akalin, 2012/02/02 01:34:39]

you only call this function twice, with very similar args.  I wonder if you can
simplify this a bit.  What about:

int64 CreateUnappliedAndUnsyncedItem(const string& name, syncable::ModelType) {
...
}

returns the metahandle, or an invalid metahandle on failure.  Uses
MakeServer(name) for the item_id.  Uses id_factory_.root() for the parent. 
is_folder = false.

[rlarocque, 2012/02/03 22:31:15]

Yes, that makes sense.  I wrote this function before I used it.  In hindsight,
it would have made sense to think about how it would be used.  

Unfortunately, the GTEST framework doesn't like functions that can fail *and*
return values.  They need to do one thing or the other.  

I decided downgraded the ASSERTs to EXPECTs.  I think this is safe because
ScopedDirLookup can't fail, and I've added code to handle the case where
entry.good() fails.

+                                      const Id& parent_id,
+                                      const string& name,
+                                      bool is_folder,
+                                      syncable::ModelType model_type,
+                                      int64* metahandle_out) {
+    // This function requires that the item_id be a server ID.
+    CHECK(item_id.ServerKnows());

[akalin, 2012/02/02 01:34:39]

you CHECK here but you ASSERT_TRUE later in the function.  I think it's just
better all around to have this return false and move the ASSERT_TRUE to the
callers

[rlarocque, 2012/02/03 22:31:15]

Good point.  I would fix this, but in the latest patch this line no longer
exists.

+
+    // If our caller did not specify a location to store the returned
+    // metahandle, then use our own temporary storage.
+    int64 local_metahandle_out = 0;
+    if (!metahandle_out)
+      metahandle_out = &local_metahandle_out;
+
+    CreateUnsyncedItem(item_id, parent_id, name, is_folder,
+                       model_type, metahandle_out);
+
+    ScopedDirLookup dir(syncdb()->manager(), syncdb()->name());
+    ASSERT_TRUE(dir.good());
+    WriteTransaction trans(FROM_HERE, UNITTEST, dir);
+    MutableEntry entry(&trans, syncable::GET_BY_HANDLE, *metahandle_out);
+    ASSERT_TRUE(entry.good());
+
+    entry.Put(syncable::IS_UNAPPLIED_UPDATE, true);
+    entry.Put(syncable::SERVER_VERSION, next_revision_++);

[akalin, 2012/02/02 01:34:39]

use GetNextRevision() here, too?

[rlarocque, 2012/02/03 22:31:15]

Done.

+  }
+
+  int64 GetNextRevision() {
+    return next_revision_++;
+  }
+
   ApplyUpdatesCommand apply_updates_command_;
   TestIdFactory id_factory_;
  private:
   int64 next_revision_;
   DISALLOW_COPY_AND_ASSIGN(ApplyUpdatesCommandTest);
 };
 
 TEST_F(ApplyUpdatesCommandTest, Simple) {
   string root_server_id = syncable::GetNullId().GetServerId();
   CreateUnappliedNewItemWithParent("parent",
                                    DefaultBookmarkSpecifics(),
                                    root_server_id);
   CreateUnappliedNewItemWithParent("child",
                                    DefaultBookmarkSpecifics(),
                                    "parent");
 
   ExpectGroupToChange(apply_updates_command_, GROUP_UI);
   apply_updates_command_.ExecuteImpl(session());
 
   sessions::StatusController* status = session()->mutable_status_controller();
 
   sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI);
   ASSERT_TRUE(status->update_progress());
   EXPECT_EQ(2, status->update_progress()->AppliedUpdatesSize())

[akalin, 2012/02/02 01:34:39]

comment this test, too

       << "All updates should have been attempted";
   ASSERT_TRUE(status->conflict_progress());
-  EXPECT_EQ(0, status->conflict_progress()->ConflictingItemsSize())
+  EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize())
+      << "Simple update shouldn't result in conflicts";
+  EXPECT_EQ(0, status->conflict_progress()->EncryptionConflictingItemsSize())
+      << "Simple update shouldn't result in conflicts";
+  EXPECT_EQ(0, status->conflict_progress()->HierarchyConflictingItemsSize())
       << "Simple update shouldn't result in conflicts";
   EXPECT_EQ(2, status->update_progress()->SuccessfullyAppliedUpdateCount())
       << "All items should have been successfully applied";
 }
 
 TEST_F(ApplyUpdatesCommandTest, UpdateWithChildrenBeforeParents) {
   // Set a bunch of updates which are difficult to apply in the order
   // they're received due to dependencies on other unseen items.
   string root_server_id = syncable::GetNullId().GetServerId();
   CreateUnappliedNewItemWithParent("a_child_created_first",
@@ skipping 14 matching lines @@
 
   ExpectGroupToChange(apply_updates_command_, GROUP_UI);
   apply_updates_command_.ExecuteImpl(session());
 
   sessions::StatusController* status = session()->mutable_status_controller();
   sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI);
   ASSERT_TRUE(status->update_progress());
   EXPECT_EQ(5, status->update_progress()->AppliedUpdatesSize())
       << "All updates should have been attempted";
   ASSERT_TRUE(status->conflict_progress());
-  EXPECT_EQ(0, status->conflict_progress()->ConflictingItemsSize())
+  EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize())
       << "Simple update shouldn't result in conflicts, even if out-of-order";
   EXPECT_EQ(5, status->update_progress()->SuccessfullyAppliedUpdateCount())
       << "All updates should have been successfully applied";
 }
 
-TEST_F(ApplyUpdatesCommandTest, NestedItemsWithUnknownParent) {
+// Runs the ApplyUpdatesCommand on an item that has both local and remote
+// modifications (IS_UNSYNCED and IS_UNAPPLIED_UPDATE).  We expect the command
+// to detect that this update can't be applied because it is in a CONFLICT
+// state.
+TEST_F(ApplyUpdatesCommandTest, SimpleConflict) {
+  string root_server_id = syncable::GetNullId().GetServerId();

[akalin, 2012/02/02 01:34:39]

this variable doesn't seem to be used

[rlarocque, 2012/02/03 22:31:15]

Done.

+  CreateUnappliedAndUnsyncedItem(id_factory_.MakeServer("item"),
+                                 id_factory_.root(), "item", false,
+                                 syncable::BOOKMARKS, NULL);
+
+  ExpectGroupToChange(apply_updates_command_, GROUP_UI);
+  apply_updates_command_.ExecuteImpl(session());
+
+  sessions::StatusController* status = session()->mutable_status_controller();

[akalin, 2012/02/02 01:34:39]

can you use the non-mutable status_controller accessor instead?  is there a
non-mutable conflict_progress() accessor?

[rlarocque, 2012/02/03 22:31:15]

The ScopedModelSafeGroupRestriction needs to modify the StatusController.

+  sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI);
+  ASSERT_TRUE(status->conflict_progress());
+  EXPECT_EQ(1, status->conflict_progress()->SimpleConflictingItemsSize())
+      << "Unsynced and unapplied item should be a simple conflict";
+}
+
+// Runs the ApplyUpdatesCommand on an item that has both local and remote
+// modifications *and* the remote modification can not be applied without

[akalin, 2012/02/02 01:34:39]

can not -> cannot

[rlarocque, 2012/02/03 22:31:15]

Done.

+// violating the tree constraints.  We expect the command to detect that this
+// update can't be applied and that this situation can't be resolved with the
+// simple conflict processing logic; it is in a CONFLICT_HIERARCHY state.
+TEST_F(ApplyUpdatesCommandTest, HierarchyAndSimpleConflict) {
+  int64 handle = 0;
+  // Create a simply-conflicting item.  It will start with valid parent ids.
+  CreateUnappliedAndUnsyncedItem(id_factory_.MakeServer("orphaned_by_server"),
+                                 id_factory_.root(), "orphaned_by_server",
+                                 false, syncable::BOOKMARKS, &handle);
+  {
+    // Manually set the SERVER_PARENT_ID to bad value.
+    // A bad parent indicates a hierarchy conflict.
+    ScopedDirLookup dir(syncdb()->manager(), syncdb()->name());
+    ASSERT_TRUE(dir.good());
+    WriteTransaction trans(FROM_HERE, UNITTEST, dir);
+    MutableEntry entry(&trans, syncable::GET_BY_HANDLE, handle);
+    ASSERT_TRUE(entry.good());
+
+    entry.Put(syncable::SERVER_PARENT_ID,
+              id_factory_.MakeServer("bogus_parent"));
+  }
+
+  ExpectGroupToChange(apply_updates_command_, GROUP_UI);
+  apply_updates_command_.ExecuteImpl(session());
+
+  sessions::StatusController* status = session()->mutable_status_controller();

[akalin, 2012/02/02 01:34:39]

here too

[akalin, 2012/02/02 01:34:39]

all these lines are just to check HierarchyConflictingItemsSize() and they're
repeated across multiple tests.  Decomp into helper fn?

[rlarocque, 2012/02/03 22:31:15]

I'm on the fence about that.  

If we pass in the expectations, then they would probably end up with something
like this:

ExpectConflictCounts(GROUP_UI, 0, 1, 0, 0)

Which is hard to read if you haven't memorized the function parameter names.

On the other hand, if we return the structs, we would have:

UpdateProgress update_progress = GetUpdateProgress(GROUP_UI);
ConflcitProgress conflict_progress = GetConflictProgress(GROUP_UI);

Which is not shorter and hides what's actually going on, making it a bit harder
to understand.  

Can you think of other arguments in favor of or against these alternatives?

+  sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI);
+
+  EXPECT_EQ(1, status->update_progress()->AppliedUpdatesSize());
+
+  // An update that is both a simple conflict and a hierarchy conflict should be
+  // treated as a hierarchy conflict.
+  ASSERT_TRUE(status->conflict_progress());
+  EXPECT_EQ(1, status->conflict_progress()->HierarchyConflictingItemsSize());
+  EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize());
+}
+
+
+// Runs the ApplyUpdatesCommand on an item with remote modifications that would
+// create a directory loop if the update were applied.  We expect the command to
+// detect that this update can't be applied because it is in a
+// CONFLICT_HIERARCHY state.
+TEST_F(ApplyUpdatesCommandTest, HierarchyConflictDirectoryLoop) {
+  // Item 'X' locally has parent of 'root'.  Server is updating it to have
+  // parent of 'Y'.
+  {
+    ScopedDirLookup dir(syncdb()->manager(), syncdb()->name());
+    ASSERT_TRUE(dir.good());
+    WriteTransaction trans(FROM_HERE, UNITTEST, dir);
+
+    syncable::Id parent_id(id_factory_.root());
+    MutableEntry entry(&trans, syncable::CREATE, parent_id, "X");
+    ASSERT_TRUE(entry.good());
+
+    entry.Put(syncable::ID, id_factory_.MakeServer("X"));
+    entry.Put(syncable::BASE_VERSION, GetNextRevision());
+    entry.Put(syncable::IS_UNSYNCED, false);
+    entry.Put(syncable::NON_UNIQUE_NAME, "X");
+    entry.Put(syncable::IS_DIR, true);
+    entry.Put(syncable::IS_DEL, false);
+    entry.Put(syncable::PARENT_ID, parent_id);
+
+    CHECK(entry.PutPredecessor(id_factory_.root()));

[akalin, 2012/02/02 01:34:39]

ASSERT_TRUE

[rlarocque, 2012/02/03 22:31:15]

Done.

+    entry.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics());
+
+    entry.Put(syncable::SERVER_VERSION, GetNextRevision());
+    entry.Put(syncable::IS_UNAPPLIED_UPDATE, true);
+    entry.Put(syncable::SERVER_NON_UNIQUE_NAME, "X");
+    entry.Put(syncable::SERVER_PARENT_ID, id_factory_.MakeServer("Y"));
+    entry.Put(syncable::SERVER_IS_DIR, true);
+    entry.Put(syncable::SERVER_SPECIFICS, DefaultBookmarkSpecifics());
+  }
+
+  // Item 'Y' is child of 'X'.
+  CreateUnsyncedItem(id_factory_.MakeServer("Y"), id_factory_.MakeServer("X"),
+                     "Y", true, syncable::BOOKMARKS, NULL);
+
+  // If the server's update were applied, we would have X be a child of Y, and Y
+  // as a child of X.  That's a directory loop.  The UpdateApplicator should
+  // prevent the update from being applied and note that this is a hierarchy
+  // conflict.
+
+  ExpectGroupToChange(apply_updates_command_, GROUP_UI);
+  apply_updates_command_.ExecuteImpl(session());
+
+  sessions::StatusController* status = session()->mutable_status_controller();
+  sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI);
+
+  EXPECT_EQ(1, status->update_progress()->AppliedUpdatesSize());
+
+  // This should count as a hierarchy conflict.
+  ASSERT_TRUE(status->conflict_progress());
+  EXPECT_EQ(1, status->conflict_progress()->HierarchyConflictingItemsSize());
+  EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize());
+}
+
+// Runs the ApplyUpdatesCommand on a directory where the server sent us an
+// update to add a child to a locally deleted (and unsynced) parent.  We expect
+// the command to not apply the update and to indicate the update is in a
+// CONFLICT_HIERARCHY state.
+TEST_F(ApplyUpdatesCommandTest, HierarchyConflictDeletedParent) {
+  // Create a locally deleted parent item.
+  int64 parent_handle;
+  CreateUnsyncedItem(Id::CreateFromServerId("parent"), id_factory_.root(),
+                     "parent", true, syncable::BOOKMARKS, &parent_handle);
+  {
+    ScopedDirLookup dir(syncdb()->manager(), syncdb()->name());
+    ASSERT_TRUE(dir.good());
+    WriteTransaction trans(FROM_HERE, UNITTEST, dir);
+    MutableEntry entry(&trans, syncable::GET_BY_HANDLE, parent_handle);
+    entry.Put(syncable::IS_DEL, true);
+  }
+
+  // Create an incoming child from the server.
+  CreateUnappliedNewItemWithParent("child", DefaultBookmarkSpecifics(),
+                                   "parent");
+
+  // The server's update may seem valid to some other client, but on this client
+  // that new item's parent no longer exists.  The update should not be applied
+  // and the update applicator should indicate this is a hierarchy conflict.
+
+  ExpectGroupToChange(apply_updates_command_, GROUP_UI);
+  apply_updates_command_.ExecuteImpl(session());
+
+  sessions::StatusController* status = session()->mutable_status_controller();
+  sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI);
+
+  // This should count as a hierarchy conflict.
+  ASSERT_TRUE(status->conflict_progress());
+  EXPECT_EQ(1, status->conflict_progress()->HierarchyConflictingItemsSize());
+  EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize());
+}
+
+// Runs the ApplyUpdatesCommand on a directory where the server is trying to
+// delete a folder that has a recently added (and unsynced) child.  We expect
+// the command to not apply the update because it is in a CONFLICT_HIERARCHY
+// state.
+TEST_F(ApplyUpdatesCommandTest, HierarchyConflictDeleteNonEmptyDirectory) {
+  // Create a server-deleted directory.
+  {
+    ScopedDirLookup dir(syncdb()->manager(), syncdb()->name());
+    ASSERT_TRUE(dir.good());
+    WriteTransaction trans(FROM_HERE, UNITTEST, dir);
+
+    syncable::Id parent_id(id_factory_.root());
+    MutableEntry entry(&trans, syncable::CREATE, parent_id, "parent");
+    ASSERT_TRUE(entry.good());
+
+    entry.Put(syncable::ID, id_factory_.MakeServer("parent"));
+    entry.Put(syncable::BASE_VERSION, GetNextRevision());

[akalin, 2012/02/02 01:34:39]

i feel like you can decomp this into some helper function that the other block
with lots of entry.Put()s can use.

[rlarocque, 2012/02/03 22:31:15]

That's a good idea.  Implemented in the next patch set.

+    entry.Put(syncable::IS_UNSYNCED, false);
+    entry.Put(syncable::NON_UNIQUE_NAME, "parent");
+    entry.Put(syncable::IS_DIR, true);
+    entry.Put(syncable::IS_DEL, false);
+    entry.Put(syncable::PARENT_ID, parent_id);
+
+    CHECK(entry.PutPredecessor(id_factory_.root()));
+    entry.Put(syncable::SPECIFICS, DefaultBookmarkSpecifics());
+
+    entry.Put(syncable::SERVER_VERSION, GetNextRevision());
+    entry.Put(syncable::IS_UNAPPLIED_UPDATE, true);
+    entry.Put(syncable::SERVER_NON_UNIQUE_NAME, "parent");
+    entry.Put(syncable::SERVER_PARENT_ID, parent_id);
+    entry.Put(syncable::SERVER_IS_DIR, true);
+    entry.Put(syncable::SERVER_IS_DEL, true);
+    entry.Put(syncable::SERVER_SPECIFICS, DefaultBookmarkSpecifics());
+  }
+
+  // Create a local child of the server-deleted directory.
+  CreateUnsyncedItem(id_factory_.MakeServer("child"),
+                     id_factory_.MakeServer("parent"), "child", false,
+                     syncable::BOOKMARKS, NULL);
+
+  // The server's request to delete the directory must be ignored, otherwise our
+  // unsynced new child would be orphaned.  This is a hierarchy conflict.
+
+  ExpectGroupToChange(apply_updates_command_, GROUP_UI);
+  apply_updates_command_.ExecuteImpl(session());
+
+  sessions::StatusController* status = session()->mutable_status_controller();
+  sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI);
+
+  // This should count as a hierarchy conflict.
+  ASSERT_TRUE(status->conflict_progress());
+  EXPECT_EQ(1, status->conflict_progress()->HierarchyConflictingItemsSize());
+  EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize());
+}
+
+// Runs the ApplyUpdatesCommand on a server-created item that has a locally
+// unknown parent.  We expect the command to not apply the update because the
+// item is in a CONFLICT_HIERARCHY state.
+TEST_F(ApplyUpdatesCommandTest, HierarchyConflictUnknownParent) {
   // We shouldn't be able to do anything with either of these items.
   CreateUnappliedNewItemWithParent("some_item",
                                    DefaultBookmarkSpecifics(),
                                    "unknown_parent");
   CreateUnappliedNewItemWithParent("some_other_item",
                                    DefaultBookmarkSpecifics(),
                                    "some_item");
 
   ExpectGroupToChange(apply_updates_command_, GROUP_UI);
   apply_updates_command_.ExecuteImpl(session());
 
   sessions::StatusController* status = session()->mutable_status_controller();
   sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI);
   ASSERT_TRUE(status->update_progress());
   EXPECT_EQ(2, status->update_progress()->AppliedUpdatesSize())
       << "All updates should have been attempted";
   ASSERT_TRUE(status->conflict_progress());
-  EXPECT_EQ(2, status->conflict_progress()->ConflictingItemsSize())
+  EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize())
+      << "Updates with unknown parent should not be treated as 'simple'"
+      << " conflicts";
+  EXPECT_EQ(2, status->conflict_progress()->HierarchyConflictingItemsSize())
       << "All updates with an unknown ancestors should be in conflict";
   EXPECT_EQ(0, status->update_progress()->SuccessfullyAppliedUpdateCount())
       << "No item with an unknown ancestor should be applied";
 }
 
 TEST_F(ApplyUpdatesCommandTest, ItemsBothKnownAndUnknown) {
   // See what happens when there's a mixture of good and bad updates.
   string root_server_id = syncable::GetNullId().GetServerId();
   CreateUnappliedNewItemWithParent("first_unknown_item",
                                    DefaultBookmarkSpecifics(),
@@ skipping 16 matching lines @@
 
   ExpectGroupToChange(apply_updates_command_, GROUP_UI);
   apply_updates_command_.ExecuteImpl(session());
 
   sessions::StatusController* status = session()->mutable_status_controller();
   sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI);
   ASSERT_TRUE(status->update_progress());
   EXPECT_EQ(6, status->update_progress()->AppliedUpdatesSize())
       << "All updates should have been attempted";
   ASSERT_TRUE(status->conflict_progress());
-  EXPECT_EQ(2, status->conflict_progress()->ConflictingItemsSize())
+  EXPECT_EQ(2, status->conflict_progress()->HierarchyConflictingItemsSize())
       << "The updates with unknown ancestors should be in conflict";
   EXPECT_EQ(4, status->update_progress()->SuccessfullyAppliedUpdateCount())
       << "The updates with known ancestors should be successfully applied";
 }
 
 TEST_F(ApplyUpdatesCommandTest, DecryptablePassword) {
   // Decryptable password updates should be applied.
   Cryptographer* cryptographer;
   {
       // Storing the cryptographer separately is bad, but for this test we
@@ skipping 18 matching lines @@
 
   ExpectGroupToChange(apply_updates_command_, GROUP_PASSWORD);
   apply_updates_command_.ExecuteImpl(session());
 
   sessions::StatusController* status = session()->mutable_status_controller();
   sessions::ScopedModelSafeGroupRestriction r(status, GROUP_PASSWORD);
   ASSERT_TRUE(status->update_progress());
   EXPECT_EQ(1, status->update_progress()->AppliedUpdatesSize())
       << "All updates should have been attempted";
   ASSERT_TRUE(status->conflict_progress());
-  EXPECT_EQ(0, status->conflict_progress()->ConflictingItemsSize())
+  EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize())
       << "No update should be in conflict because they're all decryptable";
   EXPECT_EQ(1, status->update_progress()->SuccessfullyAppliedUpdateCount())
       << "The updates that can be decrypted should be applied";
 }
 
 TEST_F(ApplyUpdatesCommandTest, UndecryptableData) {
   // Undecryptable updates should not be applied.
   sync_pb::EntitySpecifics encrypted_bookmark;
   encrypted_bookmark.mutable_encrypted();
   AddDefaultExtensionValue(syncable::BOOKMARKS, &encrypted_bookmark);
@@ skipping 12 matching lines @@
   sessions::StatusController* status = session()->mutable_status_controller();
   EXPECT_TRUE(status->HasConflictingUpdates())
     << "Updates that can't be decrypted should trigger the syncer to have "
     << "conflicting updates.";
   {
     sessions::ScopedModelSafeGroupRestriction r(status, GROUP_UI);
     ASSERT_TRUE(status->update_progress());
     EXPECT_EQ(2, status->update_progress()->AppliedUpdatesSize())
         << "All updates should have been attempted";
     ASSERT_TRUE(status->conflict_progress());
-    EXPECT_EQ(0, status->conflict_progress()->ConflictingItemsSize())
+    EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize())
         << "The updates that can't be decrypted should not be in regular "
         << "conflict";
-    EXPECT_EQ(2, status->conflict_progress()->NonblockingConflictingItemsSize())
-        << "The updates that can't be decrypted should be in nonblocking "
+    EXPECT_EQ(2, status->conflict_progress()->EncryptionConflictingItemsSize())
+        << "The updates that can't be decrypted should be in encryption "
         << "conflict";
     EXPECT_EQ(0, status->update_progress()->SuccessfullyAppliedUpdateCount())
         << "No update that can't be decrypted should be applied";
   }
   {
     sessions::ScopedModelSafeGroupRestriction r(status, GROUP_PASSWORD);
     ASSERT_TRUE(status->update_progress());
     EXPECT_EQ(1, status->update_progress()->AppliedUpdatesSize())
         << "All updates should have been attempted";
   ASSERT_TRUE(status->conflict_progress());
-    EXPECT_EQ(0, status->conflict_progress()->ConflictingItemsSize())
+    EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize())
         << "The updates that can't be decrypted should not be in regular "
         << "conflict";
-    EXPECT_EQ(1, status->conflict_progress()->NonblockingConflictingItemsSize())
-        << "The updates that can't be decrypted should be in nonblocking "
+    EXPECT_EQ(1, status->conflict_progress()->EncryptionConflictingItemsSize())
+        << "The updates that can't be decrypted should be in encryption "
         << "conflict";
     EXPECT_EQ(0, status->update_progress()->SuccessfullyAppliedUpdateCount())
         << "No update that can't be decrypted should be applied";
   }
 }
 
 TEST_F(ApplyUpdatesCommandTest, SomeUndecryptablePassword) {
   // Only decryptable password updates should be applied.
   {
     sync_pb::EntitySpecifics specifics;
@@ skipping 35 matching lines @@
   sessions::StatusController* status = session()->mutable_status_controller();
   EXPECT_TRUE(status->HasConflictingUpdates())
     << "Updates that can't be decrypted should trigger the syncer to have "
     << "conflicting updates.";
   {
     sessions::ScopedModelSafeGroupRestriction r(status, GROUP_PASSWORD);
     ASSERT_TRUE(status->update_progress());
     EXPECT_EQ(2, status->update_progress()->AppliedUpdatesSize())
         << "All updates should have been attempted";
     ASSERT_TRUE(status->conflict_progress());
-    EXPECT_EQ(0, status->conflict_progress()->ConflictingItemsSize())
+    EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize())
         << "The updates that can't be decrypted should not be in regular "
         << "conflict";
-    EXPECT_EQ(1, status->conflict_progress()->NonblockingConflictingItemsSize())
-        << "The updates that can't be decrypted should be in nonblocking "
+    EXPECT_EQ(1, status->conflict_progress()->EncryptionConflictingItemsSize())
+        << "The updates that can't be decrypted should be in encryption "
         << "conflict";
     EXPECT_EQ(1, status->update_progress()->SuccessfullyAppliedUpdateCount())
         << "The undecryptable password update shouldn't be applied";
   }
 }
 
 TEST_F(ApplyUpdatesCommandTest, NigoriUpdate) {
   // Storing the cryptographer separately is bad, but for this test we
   // know it's safe.
   Cryptographer* cryptographer;
@@ skipping 26 matching lines @@
 
   ExpectGroupToChange(apply_updates_command_, GROUP_PASSIVE);
   apply_updates_command_.ExecuteImpl(session());
 
   sessions::StatusController* status = session()->mutable_status_controller();
   sessions::ScopedModelSafeGroupRestriction r(status, GROUP_PASSIVE);
   ASSERT_TRUE(status->update_progress());
   EXPECT_EQ(1, status->update_progress()->AppliedUpdatesSize())
       << "All updates should have been attempted";
   ASSERT_TRUE(status->conflict_progress());
-  EXPECT_EQ(0, status->conflict_progress()->ConflictingItemsSize())
+  EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize())
       << "The nigori update shouldn't be in conflict";
   EXPECT_EQ(1, status->update_progress()->SuccessfullyAppliedUpdateCount())
       << "The nigori update should be applied";
 
   EXPECT_FALSE(cryptographer->is_ready());
   EXPECT_TRUE(cryptographer->has_pending_keys());
   EXPECT_TRUE(
       cryptographer->GetEncryptedTypes()
           .Equals(syncable::ModelTypeSet::All()));
 }
@@ skipping 33 matching lines @@
 
   ExpectGroupToChange(apply_updates_command_, GROUP_PASSIVE);
   apply_updates_command_.ExecuteImpl(session());
 
   sessions::StatusController* status = session()->mutable_status_controller();
   sessions::ScopedModelSafeGroupRestriction r(status, GROUP_PASSIVE);
   ASSERT_TRUE(status->update_progress());
   EXPECT_EQ(1, status->update_progress()->AppliedUpdatesSize())
       << "All updates should have been attempted";
   ASSERT_TRUE(status->conflict_progress());
-  EXPECT_EQ(0, status->conflict_progress()->ConflictingItemsSize())
+  EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize())
       << "The nigori update shouldn't be in conflict";
   EXPECT_EQ(1, status->update_progress()->SuccessfullyAppliedUpdateCount())
       << "The nigori update should be applied";
 
   EXPECT_FALSE(cryptographer->is_ready());
   EXPECT_TRUE(cryptographer->has_pending_keys());
   EXPECT_TRUE(
       cryptographer->GetEncryptedTypes()
           .Equals(syncable::ModelTypeSet::All()));
 }
@@ skipping 69 matching lines @@
 
   ExpectGroupToChange(apply_updates_command_, GROUP_PASSIVE);
   apply_updates_command_.ExecuteImpl(session());
 
   sessions::StatusController* status = session()->mutable_status_controller();
   sessions::ScopedModelSafeGroupRestriction r(status, GROUP_PASSIVE);
   ASSERT_TRUE(status->update_progress());
   EXPECT_EQ(1, status->update_progress()->AppliedUpdatesSize())
       << "All updates should have been attempted";
   ASSERT_TRUE(status->conflict_progress());
-  EXPECT_EQ(0, status->conflict_progress()->ConflictingItemsSize())
+  EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize())
       << "No updates should be in conflict";
-  EXPECT_EQ(0, status->conflict_progress()->NonblockingConflictingItemsSize())
+  EXPECT_EQ(0, status->conflict_progress()->EncryptionConflictingItemsSize())
       << "No updates should be in conflict";
   EXPECT_EQ(1, status->update_progress()->SuccessfullyAppliedUpdateCount())
       << "The nigori update should be applied";
   EXPECT_FALSE(cryptographer->has_pending_keys());
   EXPECT_TRUE(cryptographer->is_ready());
   {
     ScopedDirLookup dir(syncdb()->manager(), syncdb()->name());
     ASSERT_TRUE(dir.good());
     ReadTransaction trans(FROM_HERE, dir);
 
@@ skipping 80 matching lines @@
 
   ExpectGroupToChange(apply_updates_command_, GROUP_PASSIVE);
   apply_updates_command_.ExecuteImpl(session());
 
   sessions::StatusController* status = session()->mutable_status_controller();
   sessions::ScopedModelSafeGroupRestriction r(status, GROUP_PASSIVE);
   ASSERT_TRUE(status->update_progress());
   EXPECT_EQ(1, status->update_progress()->AppliedUpdatesSize())
       << "All updates should have been attempted";
   ASSERT_TRUE(status->conflict_progress());
-  EXPECT_EQ(0, status->conflict_progress()->ConflictingItemsSize())
+  EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize())
       << "The unsynced changes don't trigger a blocking conflict with the "
       << "nigori update.";
-  EXPECT_EQ(0, status->conflict_progress()->NonblockingConflictingItemsSize())
-      << "The unsynced changes don't trigger a non-blocking conflict with the "
+  EXPECT_EQ(0, status->conflict_progress()->EncryptionConflictingItemsSize())
+      << "The unsynced changes don't trigger an encryption conflict with the "
       << "nigori update.";
   EXPECT_EQ(1, status->update_progress()->SuccessfullyAppliedUpdateCount())
       << "The nigori update should be applied";
   EXPECT_FALSE(cryptographer->is_ready());
   EXPECT_TRUE(cryptographer->has_pending_keys());
   {
     ScopedDirLookup dir(syncdb()->manager(), syncdb()->name());
     ASSERT_TRUE(dir.good());
     ReadTransaction trans(FROM_HERE, dir);
 
     // Since we have pending keys, we would have failed to encrypt, but the
     // cryptographer should be updated.
     EXPECT_FALSE(VerifyUnsyncedChangesAreEncrypted(&trans, encrypted_types));
     EXPECT_TRUE(cryptographer->GetEncryptedTypes().Equals(
         syncable::ModelTypeSet().All()));
     EXPECT_FALSE(cryptographer->is_ready());
     EXPECT_TRUE(cryptographer->has_pending_keys());
 
     Syncer::UnsyncedMetaHandles handles;
     SyncerUtil::GetUnsyncedEntries(&trans, &handles);
     EXPECT_EQ(2*batch_s+1, handles.size());
   }
 }
 
 }  // namespace browser_sync