sync: Remove the remaining conflict sets code

chrome/browser/sync/engine/syncer_util.cc

Index: chrome/browser/sync/engine/syncer_util.cc
diff --git a/chrome/browser/sync/engine/syncer_util.cc b/chrome/browser/sync/engine/syncer_util.cc
index bf1456e662da5ebf12deedafce8c683dbf9486d9..cf670338048f8349e1b6bc2035b0db2e05bb9532 100644
--- a/chrome/browser/sync/engine/syncer_util.cc
+++ b/chrome/browser/sync/engine/syncer_util.cc
@@ -333,11 +333,6 @@ UpdateAttemptResponse SyncerUtil::AttemptToUpdateEntry(
     }
   }
 
-  if (entry->Get(IS_UNSYNCED)) {
-    DVLOG(1) << "Skipping update, returning conflict for: " << id
-             << " ; it's unsynced.";
-    return CONFLICT;
-  }
   if (!entry->Get(SERVER_IS_DEL)) {
     syncable::Id new_parent = entry->Get(SERVER_PARENT_ID);
     Entry parent(trans, GET_BY_ID,  new_parent);
@@ -348,13 +343,13 @@ UpdateAttemptResponse SyncerUtil::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)) {
-      return CONFLICT;
+      return HIERARCHY_CONFLICT;

[rlarocque, 2012/01/31 19:12:17]

There is a subtle side-effect to this change, which affects mostly unit tests.  

The old logic would have treated !parent.good() as though it were a simple
conflict.  This commonly happens in unit tests, so I had to update a bunch of
assertions accordingly.  

In the real world, this could happen if we received updates out of order.  Under
the old system, we would have classified this as a simple conflict, but it would
not have been put into a conflict set.  We would have tried to resolve this as
though it were a simple conflict, wasting CPU cycles and possibly throwing away
a valid server changes if we decided for some reason that this was a "client
wins" case.  

Under the new system, we will treat it as a HIERARCHY conflict, which means will
ignore the update and hope the server sends us a better update soon.

[Nicolas Zea, 2012/02/01 19:16:19]

So I'm clear, the only side effect here is that we no longer invoke the conflict
resolver on this item, right? We'll still attempt to apply this update next time
around (and if the parent has arrived by then the update will apply fine).

Effectively, hierarchy conflict is treated the same as encryption conflict.

[rlarocque, 2012/02/02 00:32:56]

Yes, that is correct.

     }
     if (entry->Get(PARENT_ID) != new_parent) {
       if (!entry->Get(IS_DEL) && !IsLegalNewParent(trans, id, new_parent)) {
         DVLOG(1) << "Not updating item " << id
                  << ", illegal new parent (would cause loop).";
-        return CONFLICT;
+        return HIERARCHY_CONFLICT;
       }
     }
   } else if (entry->Get(IS_DIR)) {
@@ -364,10 +359,16 @@ UpdateAttemptResponse SyncerUtil::AttemptToUpdateEntry(
       // If we have still-existing children, then we need to deal with
       // them before we can process this change.
       DVLOG(1) << "Not deleting directory; it's not empty " << *entry;
-      return CONFLICT;
+      return HIERARCHY_CONFLICT;
     }
   }
 
+  if (entry->Get(IS_UNSYNCED)) {

[rlarocque, 2012/01/31 19:12:17]

This was moved down here because it's important that HIERARCHY_CONFLICT take
precedence over CONFLICT.

+    DVLOG(1) << "Skipping update, returning conflict for: " << id
+             << " ; it's unsynced.";
+    return CONFLICT;
+  }
+
   if (specifics.has_encrypted()) {
     DVLOG(2) << "Received a decryptable "
              << syncable::ModelTypeToString(entry->GetServerModelType())