Fix a bug where positions inside of a newly-committed folder would be

reversed if the children of the folder weren't committed in the same
batch as the folder.

Refactor ApplyUpdatesCommandTest, pulling out functionality that would
be useful for any SyncerCommand test, into syncer_command_test.h.

Add a test case for ProcessCommitResponseCommand using the new
SyncerCommandTest framework.  Add a test for the bug.

BUG=33081
TEST=sync_unit_tests. Also, manual testing, using the reduced repro
instructions described in comment #26 of the bug.

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

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

2 files changed, 19 insertions, 3 deletions

diff --git a/chrome/browser/sync/syncable/syncable.cc b/chrome/browser/sync/syncable/syncable.cc
index d5ce7a6..fdd379c 100755
--- a/chrome/browser/sync/syncable/syncable.cc
+++ b/chrome/browser/sync/syncable/syncable.cc
@@ -1142,8 +1142,8 @@ bool MutableEntry::Put(IdField field, const Id& value) {
       if (!dir()->ReindexId(kernel_, value))
         return false;
     } else if (PARENT_ID == field) {
-      dir()->ReindexParentId(kernel_, value);
-      PutPredecessor(Id());
+      PutParentIdPropertyOnly(value);  // Makes sibling order inconsistent.
+      PutPredecessor(Id());  // Fixes up the sibling order inconsistency.
     } else {
       kernel_->put(field, value);
     }
@@ -1152,6 +1152,10 @@ bool MutableEntry::Put(IdField field, const Id& value) {
   return true;
 }
 
+void MutableEntry::PutParentIdPropertyOnly(const Id& parent_id) {
+  dir()->ReindexParentId(kernel_, parent_id);
+}
+
 bool MutableEntry::Put(BaseVersion field, int64 value) {
   DCHECK(kernel_);
   if (kernel_->ref(field) != value) {
@@ -1269,6 +1273,7 @@ bool MutableEntry::PutPredecessor(const Id& predecessor_id) {
   return true;
 }
 
+
 ///////////////////////////////////////////////////////////////////////////
 // High-level functions
 
diff --git a/chrome/browser/sync/syncable/syncable.h b/chrome/browser/sync/syncable/syncable.h
index 0fe199e..be091d0 100755
--- a/chrome/browser/sync/syncable/syncable.h
+++ b/chrome/browser/sync/syncable/syncable.h
@@ -442,6 +442,18 @@ class MutableEntry : public Entry {
   // TODO(chron): Remove some of these unecessary return values.
   bool Put(Int64Field field, const int64& value);
   bool Put(IdField field, const Id& value);
+
+  // Do a simple property-only update if the PARENT_ID field.  Use with caution.
+  //
+  // The normal Put(IS_PARENT) call will move the item to the front of the
+  // sibling order to maintain the linked list invariants when the parent
+  // changes.  That's usually what you want to do, but it's inappropriate
+  // when the caller is trying to change the parent ID of a the whole set
+  // of children (e.g. because the ID changed during a commit).  For those
+  // cases, there's this function.  It will corrupt the sibling ordering
+  // if you're not careful.
+  void PutParentIdPropertyOnly(const Id& parent_id);
+
   bool Put(StringField field, const std::string& value);
   bool Put(BaseVersion field, int64 value);
 
@@ -474,7 +486,6 @@ class MutableEntry : public Entry {
   }
 
  protected:
-
   template <typename FieldType, typename ValueType>
   inline bool PutField(FieldType field, const ValueType& value) {
     DCHECK(kernel_);