sync: Refactor MutablEntry internals

sync/syncable/mutable_entry.cc

Index: sync/syncable/mutable_entry.cc
diff --git a/sync/syncable/mutable_entry.cc b/sync/syncable/mutable_entry.cc
index d85a9c61fa25d6e0f14cffe3800ed5378cb6b622..c9cb58d74a438d22a98317506e29af018fb6da63 100644
--- a/sync/syncable/mutable_entry.cc
+++ b/sync/syncable/mutable_entry.cc
@@ -121,105 +121,192 @@ MutableEntry::MutableEntry(WriteTransaction* trans, GetByServerTag,
 }
 
 void MutableEntry::PutBaseVersion(int64 value) {
-  Put(BASE_VERSION, value);
+  DCHECK(kernel_);
+  write_transaction_->SaveOriginal(kernel_);
+  if (kernel_->ref(BASE_VERSION) != value) {
+    kernel_->put(BASE_VERSION, value);
+    kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
+  }
 }
 
 void MutableEntry::PutServerVersion(int64 value) {
-  Put(SERVER_VERSION, value);
+  DCHECK(kernel_);
+  write_transaction_->SaveOriginal(kernel_);
+  if (kernel_->ref(SERVER_VERSION) != value) {
+    ScopedKernelLock lock(dir());

[Nicolas Zea, 2013/09/16 22:55:07]

The scoped kernel lock is only necessary when modifying a field that changes the
indexing right? Why is it introduced in these methods?

[rlarocque, 2013/09/16 23:49:56]

It's not new.  The old definition of Put(Int64Field, ...) used a
ScopedKernelLock(), so I copied that behavior in this definition.

Put(TimeField, ...) did not use that lock, so the PutMtime() and related
functions below don't use it.  

I think the lock was intended to protect the dirty_metahandles index, but that
would be redundant because no one touches that field without owning the
transaction lock.

I decided not to change it because I want this new code to be exactly the same
as the old.

[Nicolas Zea, 2013/09/17 00:54:46]

Ah, I had assumed ServerVersion and BaseVersion had the same implementation
previously, but it appears not. Not making any change makes sense.

+    kernel_->put(SERVER_VERSION, value);
+    kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
+  }
 }
 
 void MutableEntry::PutLocalExternalId(int64 value) {
-  Put(LOCAL_EXTERNAL_ID, value);
+  DCHECK(kernel_);
+  write_transaction_->SaveOriginal(kernel_);
+  if (kernel_->ref(LOCAL_EXTERNAL_ID) != value) {
+    ScopedKernelLock lock(dir());
+    kernel_->put(LOCAL_EXTERNAL_ID, value);
+    kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
+  }
 }
 
 void MutableEntry::PutMtime(base::Time value) {

[rlarocque, 2013/09/16 23:49:56]

I think these time setters are the strongest argument against this CL.  More so
than other fields, these four setters are almost identical.  For that reason, I
had an earlier version of the patch that kept the shared setter code for
TimeField values.  

I went back on that decision and decided to split these up when I realized that
doing so would make this class more consistent, and that we'd need to either add
a flag to the common setter or define two different setters for the SERVER and
non-SERVER cases.  We should only call SaveOriginal() in the non-SERVER cases.

-  Put(MTIME, value);
+  DCHECK(kernel_);
+  write_transaction_->SaveOriginal(kernel_);
+  if (kernel_->ref(MTIME) != value) {
+    kernel_->put(MTIME, value);
+    kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
+  }
 }
 
 void MutableEntry::PutServerMtime(base::Time value) {
-  Put(SERVER_MTIME, value);
+  DCHECK(kernel_);
+  write_transaction_->SaveOriginal(kernel_);
+  if (kernel_->ref(SERVER_MTIME) != value) {
+    kernel_->put(SERVER_MTIME, value);
+    kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
+  }
 }
 
 void MutableEntry::PutCtime(base::Time value) {
-  Put(CTIME, value);
+  DCHECK(kernel_);
+  write_transaction_->SaveOriginal(kernel_);
+  if (kernel_->ref(CTIME) != value) {
+    kernel_->put(CTIME, value);
+    kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
+  }
 }
 
 void MutableEntry::PutServerCtime(base::Time value) {
-  Put(SERVER_CTIME, value);
+  DCHECK(kernel_);
+  write_transaction_->SaveOriginal(kernel_);
+  if (kernel_->ref(SERVER_CTIME) != value) {
+    kernel_->put(SERVER_CTIME, value);
+    kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
+  }
 }
 
 bool MutableEntry::PutId(const Id& value) {
-  return Put(ID, value);
+  DCHECK(kernel_);
+  write_transaction_->SaveOriginal(kernel_);
+  if (kernel_->ref(ID) != value) {
+    if (!dir()->ReindexId(write_transaction(), kernel_, value))
+      return false;
+    kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
+  }
+  return true;
 }
 
 void MutableEntry::PutParentId(const Id& value) {
-  Put(PARENT_ID, value);
+  DCHECK(kernel_);
+  write_transaction_->SaveOriginal(kernel_);
+  if (kernel_->ref(PARENT_ID) != value) {
+    PutParentIdPropertyOnly(value);
+    if (!GetIsDel()) {
+      if (!PutPredecessor(Id())) {
+        // TODO(lipalani) : Propagate the error to caller. crbug.com/100444.
+        NOTREACHED();
+      }
+    }
+  }
 }
 
 void MutableEntry::PutServerParentId(const Id& value) {
-  Put(SERVER_PARENT_ID, value);
+  DCHECK(kernel_);
+  write_transaction_->SaveOriginal(kernel_);
+
+  if (kernel_->ref(SERVER_PARENT_ID) != value) {
+    kernel_->put(SERVER_PARENT_ID, value);
+    kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
+  }
 }
 
 bool MutableEntry::PutIsUnsynced(bool value) {
-  return Put(IS_UNSYNCED, value);
+  DCHECK(kernel_);
+  write_transaction_->SaveOriginal(kernel_);
+  if (kernel_->ref(IS_UNSYNCED) != value) {
+    MetahandleSet* index = &dir()->kernel_->unsynced_metahandles;
+
+    ScopedKernelLock lock(dir());
+    if (value) {
+      if (!SyncAssert(index->insert(kernel_->ref(META_HANDLE)).second,
+                      FROM_HERE,
+                      "Could not insert",
+                      write_transaction())) {
+        return false;
+      }
+    } else {
+      if (!SyncAssert(1U == index->erase(kernel_->ref(META_HANDLE)),
+                      FROM_HERE,
+                      "Entry Not succesfully erased",
+                      write_transaction())) {
+        return false;
+      }
+    }
+    kernel_->put(IS_UNSYNCED, value);
+    kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
+  }
+  return true;
 }
 
 bool MutableEntry::PutIsUnappliedUpdate(bool value) {
-  return Put(IS_UNAPPLIED_UPDATE, value);
+  DCHECK(kernel_);
+  write_transaction_->SaveOriginal(kernel_);
+  if (kernel_->ref(IS_UNAPPLIED_UPDATE) != value) {
+    // Use kernel_->GetServerModelType() instead of
+    // GetServerModelType() as we may trigger some DCHECKs in the
+    // latter.
+    MetahandleSet* index = &dir()->kernel_->unapplied_update_metahandles[
+        kernel_->GetServerModelType()];
+
+    ScopedKernelLock lock(dir());
+    if (value) {
+      if (!SyncAssert(index->insert(kernel_->ref(META_HANDLE)).second,
+                      FROM_HERE,
+                      "Could not insert",
+                      write_transaction())) {
+        return false;
+      }
+    } else {
+      if (!SyncAssert(1U == index->erase(kernel_->ref(META_HANDLE)),
+                      FROM_HERE,
+                      "Entry Not succesfully erased",
+                      write_transaction())) {
+        return false;
+      }
+    }
+    kernel_->put(IS_UNAPPLIED_UPDATE, value);
+    kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
+  }
+  return true;
 }
 
 void MutableEntry::PutIsDir(bool value) {
-  Put(IS_DIR, value);
+  DCHECK(kernel_);
+  write_transaction_->SaveOriginal(kernel_);
+  bool old_value = kernel_->ref(IS_DIR);
+  if (old_value != value) {
+    kernel_->put(IS_DIR, value);
+    kernel_->mark_dirty(GetDirtyIndexHelper());
+  }
 }
 
 void MutableEntry::PutServerIsDir(bool value) {
-  Put(SERVER_IS_DIR, value);
-}
-
-void MutableEntry::PutServerIsDel(bool value) {
-  Put(SERVER_IS_DEL, value);
-}
-
-void MutableEntry::PutNonUniqueName(const std::string& value) {
-  Put(NON_UNIQUE_NAME, value);
-}
-
-void MutableEntry::PutServerNonUniqueName(const std::string& value) {
-  Put(SERVER_NON_UNIQUE_NAME, value);
-}
-
-void MutableEntry::PutSpecifics(const sync_pb::EntitySpecifics& value) {
-  Put(SPECIFICS, value);
-}
-
-void MutableEntry::PutServerSpecifics(const sync_pb::EntitySpecifics& value) {
-  Put(SERVER_SPECIFICS, value);
-}
-
-void MutableEntry::PutBaseServerSpecifics(
-    const sync_pb::EntitySpecifics& value) {
-  Put(BASE_SERVER_SPECIFICS, value);
-}
-
-void MutableEntry::PutUniquePosition(const UniquePosition& value) {
-  Put(UNIQUE_POSITION, value);
-}
-
-void MutableEntry::PutServerUniquePosition(const UniquePosition& value) {
-  Put(SERVER_UNIQUE_POSITION, value);
-}
-
-void MutableEntry::PutSyncing(bool value) {
-  Put(SYNCING, value);
+  DCHECK(kernel_);
+  write_transaction_->SaveOriginal(kernel_);
+  bool old_value = kernel_->ref(SERVER_IS_DIR);
+  if (old_value != value) {
+    kernel_->put(SERVER_IS_DIR, value);
+    kernel_->mark_dirty(GetDirtyIndexHelper());
+  }
 }
 
-bool MutableEntry::PutIsDel(bool is_del) {
+void MutableEntry::PutIsDel(bool value) {
   DCHECK(kernel_);
   write_transaction_->SaveOriginal(kernel_);
-  if (is_del == kernel_->ref(IS_DEL)) {
-    return true;
+  if (value == kernel_->ref(IS_DEL)) {
+    return;
   }
-  if (is_del) {
+  if (value) {
     // If the server never knew about this item and it's deleted then we don't
     // need to keep it around.  Unsetting IS_UNSYNCED will:
     // - Ensure that the item is never committed to the server.
@@ -229,7 +316,7 @@ bool MutableEntry::PutIsDel(bool is_del) {
     //   DirectoryBackingStore::DropDeletedEntries() when we next restart sync.
     //   This will save memory and avoid crbug.com/125381.
     if (!GetId().ServerKnows()) {
-      Put(IS_UNSYNCED, false);
+      PutIsUnsynced(false);
     }
   }
 
@@ -240,128 +327,142 @@ bool MutableEntry::PutIsDel(bool is_del) {
     ScopedParentChildIndexUpdater updater(lock, kernel_,
         &dir()->kernel_->parent_child_index);
 
-    kernel_->put(IS_DEL, is_del);
+    kernel_->put(IS_DEL, value);
     kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
   }
-
-  return true;
 }
 
-bool MutableEntry::Put(Int64Field field, const int64& value) {
+void MutableEntry::PutServerIsDel(bool value) {
   DCHECK(kernel_);
-
-  // We shouldn't set TRANSACTION_VERSION here.  See UpdateTransactionVersion.
-  DCHECK_NE(TRANSACTION_VERSION, field);
-
   write_transaction_->SaveOriginal(kernel_);
-  if (kernel_->ref(field) != value) {
-    ScopedKernelLock lock(dir());
-    kernel_->put(field, value);
-    kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
+  bool old_value = kernel_->ref(SERVER_IS_DEL);
+  if (old_value != value) {
+    kernel_->put(SERVER_IS_DEL, value);
+    kernel_->mark_dirty(GetDirtyIndexHelper());
   }
-  return true;
+
+  // Update delete journal for existence status change on server side here
+  // instead of in PutIsDel() because IS_DEL may not be updated due to
+  // early returns when processing updates. And because
+  // UpdateDeleteJournalForServerDelete() checks for SERVER_IS_DEL, it has
+  // to be called on sync thread.
+  dir()->delete_journal()->UpdateDeleteJournalForServerDelete(
+      write_transaction(), old_value, *kernel_);
 }
 
-bool MutableEntry::Put(TimeField field, const base::Time& value) {
+void MutableEntry::PutNonUniqueName(const std::string& value) {
   DCHECK(kernel_);
   write_transaction_->SaveOriginal(kernel_);
-  if (kernel_->ref(field) != value) {
-    kernel_->put(field, value);
+
+  if (kernel_->ref(NON_UNIQUE_NAME) != value) {
+    kernel_->put(NON_UNIQUE_NAME, value);
     kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
   }
-  return true;
 }
 
-bool MutableEntry::Put(IdField field, const Id& value) {
+void MutableEntry::PutServerNonUniqueName(const std::string& value) {
   DCHECK(kernel_);
   write_transaction_->SaveOriginal(kernel_);
-  if (kernel_->ref(field) != value) {
-    if (ID == field) {
-      if (!dir()->ReindexId(write_transaction(), kernel_, value))
-        return false;
-    } else if (PARENT_ID == field) {
-      PutParentIdPropertyOnly(value);
-      if (!GetIsDel()) {
-        if (!PutPredecessor(Id())) {
-          // TODO(lipalani) : Propagate the error to caller. crbug.com/100444.
-          NOTREACHED();
-        }
-      }
-    } else {
-      kernel_->put(field, value);
-    }
+
+  if (kernel_->ref(SERVER_NON_UNIQUE_NAME) != value) {
+    kernel_->put(SERVER_NON_UNIQUE_NAME, value);
     kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
   }
-  return true;
 }
 
-bool MutableEntry::Put(UniquePositionField field, const UniquePosition& value) {
-  DCHECK(kernel_);
-  write_transaction_->SaveOriginal(kernel_);
-  if(!kernel_->ref(field).Equals(value)) {
-    // We should never overwrite a valid position with an invalid one.
-    DCHECK(value.IsValid());
-    ScopedKernelLock lock(dir());
-    if (UNIQUE_POSITION == field) {
-      ScopedParentChildIndexUpdater updater(
-          lock, kernel_, &dir()->kernel_->parent_child_index);
-      kernel_->put(field, value);
-    } else {
-      kernel_->put(field, value);
-    }
-    kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
+bool MutableEntry::PutUniqueServerTag(const string& new_tag) {
+  if (new_tag == kernel_->ref(UNIQUE_SERVER_TAG)) {
+    return true;
   }
-  return true;
-}
 
-void MutableEntry::PutParentIdPropertyOnly(const Id& parent_id) {
   write_transaction_->SaveOriginal(kernel_);
-  dir()->ReindexParentId(write_transaction(), kernel_, parent_id);
+  ScopedKernelLock lock(dir());
+  // Make sure your new value is not in there already.
+  if (dir()->kernel_->server_tags_map.find(new_tag) !=
+      dir()->kernel_->server_tags_map.end()) {
+    DVLOG(1) << "Detected duplicate server tag";
+    return false;
+  }
+  dir()->kernel_->server_tags_map.erase(
+      kernel_->ref(UNIQUE_SERVER_TAG));
+  kernel_->put(UNIQUE_SERVER_TAG, new_tag);
   kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
+  if (!new_tag.empty()) {
+    dir()->kernel_->server_tags_map[new_tag] = kernel_;
+  }
+
+  return true;
 }
 
-bool MutableEntry::Put(BaseVersion field, int64 value) {
-  DCHECK(kernel_);
+bool MutableEntry::PutUniqueClientTag(const string& new_tag) {
+  if (new_tag == kernel_->ref(UNIQUE_CLIENT_TAG)) {
+    return true;
+  }
+
   write_transaction_->SaveOriginal(kernel_);
-  if (kernel_->ref(field) != value) {
-    kernel_->put(field, value);
-    kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
+  ScopedKernelLock lock(dir());
+  // Make sure your new value is not in there already.
+  if (dir()->kernel_->client_tags_map.find(new_tag) !=
+      dir()->kernel_->client_tags_map.end()) {
+    DVLOG(1) << "Detected duplicate client tag";
+    return false;
   }
+  dir()->kernel_->client_tags_map.erase(
+      kernel_->ref(UNIQUE_CLIENT_TAG));
+  kernel_->put(UNIQUE_CLIENT_TAG, new_tag);
+  kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
+  if (!new_tag.empty()) {
+    dir()->kernel_->client_tags_map[new_tag] = kernel_;
+  }
+
   return true;
 }
 
-bool MutableEntry::Put(StringField field, const string& value) {
-  DCHECK(kernel_);
-  write_transaction_->SaveOriginal(kernel_);
-  if (field == UNIQUE_CLIENT_TAG) {
-    return PutUniqueClientTag(value);
+void MutableEntry::PutUniqueBookmarkTag(const std::string& tag) {
+  // This unique tag will eventually be used as the unique suffix when adjusting
+  // this bookmark's position.  Let's make sure it's a valid suffix.
+  if (!UniquePosition::IsValidSuffix(tag)) {
+    NOTREACHED();
+    return;
   }
 
-  if (field == UNIQUE_SERVER_TAG) {
-    return PutUniqueServerTag(value);
+  if (!kernel_->ref(UNIQUE_BOOKMARK_TAG).empty()
+      && tag != kernel_->ref(UNIQUE_BOOKMARK_TAG)) {

[Nicolas Zea, 2013/09/17 00:54:46]

nit: move && to end of previous line

[rlarocque, 2013/09/17 01:13:47]

Done.

+    // There is only one scenario where our tag is expected to change.  That
+    // scenario occurs when our current tag is a non-correct tag assigned during
+    // the UniquePosition migration.
+    std::string migration_generated_tag =
+        GenerateSyncableBookmarkHash(std::string(),
+                                     kernel_->ref(ID).GetServerId());
+    DCHECK_EQ(migration_generated_tag, kernel_->ref(UNIQUE_BOOKMARK_TAG));
   }
 
-  DCHECK_NE(UNIQUE_BOOKMARK_TAG, field)
-      << "Should use PutUniqueBookmarkTag instead of Put(UNIQUE_BOOKMARK_TAG)";
+  kernel_->put(UNIQUE_BOOKMARK_TAG, tag);
+  kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
+}
 
-  if (kernel_->ref(field) != value) {
-    kernel_->put(field, value);
+void MutableEntry::PutSpecifics(const sync_pb::EntitySpecifics& value) {
+  DCHECK(kernel_);
+  CHECK(!value.password().has_client_only_encrypted_data());
+  write_transaction_->SaveOriginal(kernel_);
+  // TODO(ncarter): This is unfortunately heavyweight.  Can we do
+  // better?
+  if (kernel_->ref(SPECIFICS).SerializeAsString()
+      != value.SerializeAsString()) {

[Nicolas Zea, 2013/09/17 00:54:46]

nit: move != to end of previous line

[rlarocque, 2013/09/17 01:13:47]

Done.

+    kernel_->put(SPECIFICS, value);
     kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
   }
-  return true;
 }
 
-bool MutableEntry::Put(ProtoField field,
-                       const sync_pb::EntitySpecifics& value) {
+void MutableEntry::PutServerSpecifics(const sync_pb::EntitySpecifics& value) {
   DCHECK(kernel_);
   CHECK(!value.password().has_client_only_encrypted_data());
   write_transaction_->SaveOriginal(kernel_);
   // TODO(ncarter): This is unfortunately heavyweight.  Can we do
   // better?
-  if (kernel_->ref(field).SerializeAsString() != value.SerializeAsString()) {
-    const bool update_unapplied_updates_index =
-        (field == SERVER_SPECIFICS) && kernel_->ref(IS_UNAPPLIED_UPDATE);
-    if (update_unapplied_updates_index) {
+  if (kernel_->ref(SERVER_SPECIFICS).SerializeAsString()
+      != value.SerializeAsString()) {

[Nicolas Zea, 2013/09/17 00:54:46]

here too

[rlarocque, 2013/09/17 01:13:47]

Done.

+    if (kernel_->ref(IS_UNAPPLIED_UPDATE)) {
       // Remove ourselves from unapplied_update_metahandles with our
       // old server type.
       const ModelType old_server_type = kernel_->GetServerModelType();
@@ -372,10 +473,10 @@ bool MutableEntry::Put(ProtoField field,
       DCHECK_EQ(erase_count, 1u);
     }
 
-    kernel_->put(field, value);
+    kernel_->put(SERVER_SPECIFICS, value);
     kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
 
-    if (update_unapplied_updates_index) {
+    if (kernel_->ref(IS_UNAPPLIED_UPDATE)) {
       // Add ourselves back into unapplied_update_metahandles with our
       // new server type.
       const ModelType new_server_type = kernel_->GetServerModelType();
@@ -384,144 +485,62 @@ bool MutableEntry::Put(ProtoField field,
           .insert(metahandle);
     }
   }
-  return true;
 }
 
-bool MutableEntry::Put(BitField field, bool value) {
+void MutableEntry::PutBaseServerSpecifics(
+    const sync_pb::EntitySpecifics& value) {
   DCHECK(kernel_);
+  CHECK(!value.password().has_client_only_encrypted_data());
   write_transaction_->SaveOriginal(kernel_);
-  bool old_value = kernel_->ref(field);
-  if (old_value != value) {
-    kernel_->put(field, value);
-    kernel_->mark_dirty(GetDirtyIndexHelper());
-  }
-
-  // Update delete journal for existence status change on server side here
-  // instead of in PutIsDel() because IS_DEL may not be updated due to
-  // early returns when processing updates. And because
-  // UpdateDeleteJournalForServerDelete() checks for SERVER_IS_DEL, it has
-  // to be called on sync thread.
-  if (field == SERVER_IS_DEL) {
-    dir()->delete_journal()->UpdateDeleteJournalForServerDelete(
-        write_transaction(), old_value, *kernel_);
-  }
-
-  return true;
-}
-
-MetahandleSet* MutableEntry::GetDirtyIndexHelper() {
-  return &dir()->kernel_->dirty_metahandles;
-}
-
-bool MutableEntry::PutUniqueClientTag(const string& new_tag) {
-  if (new_tag == kernel_->ref(UNIQUE_CLIENT_TAG)) {
-    return true;
-  }
-
-  write_transaction_->SaveOriginal(kernel_);
-  ScopedKernelLock lock(dir());
-  // Make sure your new value is not in there already.
-  if (dir()->kernel_->client_tags_map.find(new_tag) !=
-      dir()->kernel_->client_tags_map.end()) {
-    DVLOG(1) << "Detected duplicate client tag";
-    return false;
-  }
-  dir()->kernel_->client_tags_map.erase(
-      kernel_->ref(UNIQUE_CLIENT_TAG));
-  kernel_->put(UNIQUE_CLIENT_TAG, new_tag);
-  kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
-  if (!new_tag.empty()) {
-    dir()->kernel_->client_tags_map[new_tag] = kernel_;
+  // TODO(ncarter): This is unfortunately heavyweight.  Can we do
+  // better?
+  if (kernel_->ref(BASE_SERVER_SPECIFICS).SerializeAsString()
+      != value.SerializeAsString()) {
+    kernel_->put(BASE_SERVER_SPECIFICS, value);
+    kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
   }
-
-  return true;
 }
 
-bool MutableEntry::PutUniqueServerTag(const string& new_tag) {
-  if (new_tag == kernel_->ref(UNIQUE_SERVER_TAG)) {
-    return true;
-  }
-
+void MutableEntry::PutUniquePosition(const UniquePosition& value) {
+  DCHECK(kernel_);
   write_transaction_->SaveOriginal(kernel_);
-  ScopedKernelLock lock(dir());
-  // Make sure your new value is not in there already.
-  if (dir()->kernel_->server_tags_map.find(new_tag) !=
-      dir()->kernel_->server_tags_map.end()) {
-    DVLOG(1) << "Detected duplicate server tag";
-    return false;
-  }
-  dir()->kernel_->server_tags_map.erase(
-      kernel_->ref(UNIQUE_SERVER_TAG));
-  kernel_->put(UNIQUE_SERVER_TAG, new_tag);
-  kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
-  if (!new_tag.empty()) {
-    dir()->kernel_->server_tags_map[new_tag] = kernel_;
+  if(!kernel_->ref(UNIQUE_POSITION).Equals(value)) {
+    // We should never overwrite a valid position with an invalid one.
+    DCHECK(value.IsValid());
+    ScopedKernelLock lock(dir());
+    ScopedParentChildIndexUpdater updater(
+        lock, kernel_, &dir()->kernel_->parent_child_index);
+    kernel_->put(UNIQUE_POSITION, value);
+    kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
   }
-
-  return true;
 }
 
-bool MutableEntry::Put(IndexedBitField field, bool value) {
+void MutableEntry::PutServerUniquePosition(const UniquePosition& value) {
   DCHECK(kernel_);
   write_transaction_->SaveOriginal(kernel_);
-  if (kernel_->ref(field) != value) {
-    MetahandleSet* index;
-    if (IS_UNSYNCED == field) {

[rlarocque, 2013/09/16 23:49:56]

Here's an example of branching code that doesn't actually save us much trouble
in the end.  This function does different things based on which IndexedBitField
was passed in.  I think it makes more sense to define separate functions for
each bit field, especially since we now define PutIsUnsynced(), PutIsUnapplied()
as part of the interface anyway.

If you look around, you'll find that almost all of the setters have some
field-specific code.  After refactoring out each instance of field-specific
behavior, I ended up with patch set #3.  After staring at it for a bit, I
decided to be more consistent and remove the common setters for TimeFields and
Int64 fields, though the argument for removing them is not as strong.

-      index = &dir()->kernel_->unsynced_metahandles;
-    } else {
-      // Use kernel_->GetServerModelType() instead of
-      // GetServerModelType() as we may trigger some DCHECKs in the
-      // latter.
-      index =
-          &dir()->kernel_->unapplied_update_metahandles[
-              kernel_->GetServerModelType()];
-    }
-
+  if(!kernel_->ref(SERVER_UNIQUE_POSITION).Equals(value)) {
+    // We should never overwrite a valid position with an invalid one.
+    DCHECK(value.IsValid());
     ScopedKernelLock lock(dir());
-    if (value) {
-      if (!SyncAssert(index->insert(kernel_->ref(META_HANDLE)).second,
-                      FROM_HERE,
-                      "Could not insert",
-                      write_transaction())) {
-        return false;
-      }
-    } else {
-      if (!SyncAssert(1U == index->erase(kernel_->ref(META_HANDLE)),
-                      FROM_HERE,
-                      "Entry Not succesfully erased",
-                      write_transaction())) {
-        return false;
-      }
-    }
-    kernel_->put(field, value);
+    kernel_->put(SERVER_UNIQUE_POSITION, value);
     kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
   }
-  return true;
 }
 
-void MutableEntry::PutUniqueBookmarkTag(const std::string& tag) {
-  // This unique tag will eventually be used as the unique suffix when adjusting
-  // this bookmark's position.  Let's make sure it's a valid suffix.
-  if (!UniquePosition::IsValidSuffix(tag)) {
-    NOTREACHED();
-    return;
-  }
-
-  if (!kernel_->ref(UNIQUE_BOOKMARK_TAG).empty()
-      && tag != kernel_->ref(UNIQUE_BOOKMARK_TAG)) {
-    // There is only one scenario where our tag is expected to change.  That
-    // scenario occurs when our current tag is a non-correct tag assigned during
-    // the UniquePosition migration.
-    std::string migration_generated_tag =
-        GenerateSyncableBookmarkHash(std::string(),
-                                     kernel_->ref(ID).GetServerId());
-    DCHECK_EQ(migration_generated_tag, kernel_->ref(UNIQUE_BOOKMARK_TAG));
-  }
+void MutableEntry::PutSyncing(bool value) {
+  kernel_->put(SYNCING, value);
+}
 
-  kernel_->put(UNIQUE_BOOKMARK_TAG, tag);
+void MutableEntry::PutParentIdPropertyOnly(const Id& parent_id) {
+  write_transaction_->SaveOriginal(kernel_);
+  dir()->ReindexParentId(write_transaction(), kernel_, parent_id);
   kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
 }
 
+MetahandleSet* MutableEntry::GetDirtyIndexHelper() {
+  return &dir()->kernel_->dirty_metahandles;
+}
+
 bool MutableEntry::PutPredecessor(const Id& predecessor_id) {
   MutableEntry predecessor(write_transaction_, GET_BY_ID, predecessor_id);
   if (!predecessor.good())
@@ -530,12 +549,6 @@ bool MutableEntry::PutPredecessor(const Id& predecessor_id) {
   return true;
 }
 
-bool MutableEntry::Put(BitTemp field, bool value) {
-  DCHECK(kernel_);
-  kernel_->put(field, value);
-  return true;
-}
-
 void MutableEntry::UpdateTransactionVersion(int64 value) {
   ScopedKernelLock lock(dir());
   kernel_->put(TRANSACTION_VERSION, value);