Sync: MutableEntry shouldn't track changes that have no actual impact on the stored data

sync/syncable/model_neutral_mutable_entry.cc

 // Copyright 2013 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 "sync/syncable/model_neutral_mutable_entry.h"
 
 #include <string>
 
 #include "sync/internal_api/public/base/unique_position.h"
 #include "sync/syncable/directory.h"
@@ skipping 87 matching lines @@
     : Entry(trans, GET_BY_CLIENT_TAG, tag), base_write_transaction_(trans) {
 }
 
 ModelNeutralMutableEntry::ModelNeutralMutableEntry(
     BaseWriteTransaction* trans, GetTypeRoot, ModelType type)
     : Entry(trans, GET_TYPE_ROOT, type), base_write_transaction_(trans) {
 }
 
 void ModelNeutralMutableEntry::PutBaseVersion(int64 value) {
   DCHECK(kernel_);
-  base_write_transaction_->TrackChangesTo(kernel_);
   if (kernel_->ref(BASE_VERSION) != value) {
+    base_write_transaction_->TrackChangesTo(kernel_);
     kernel_->put(BASE_VERSION, value);
-    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
+    MarkDirty();
   }
 }
 
 void ModelNeutralMutableEntry::PutServerVersion(int64 value) {
   DCHECK(kernel_);
-  base_write_transaction_->TrackChangesTo(kernel_);
   if (kernel_->ref(SERVER_VERSION) != value) {
+    base_write_transaction_->TrackChangesTo(kernel_);
     ScopedKernelLock lock(dir());
     kernel_->put(SERVER_VERSION, value);
-    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
+    MarkDirty();
   }
 }
 
 void ModelNeutralMutableEntry::PutServerMtime(base::Time value) {
   DCHECK(kernel_);
-  base_write_transaction_->TrackChangesTo(kernel_);
   if (kernel_->ref(SERVER_MTIME) != value) {
+    base_write_transaction_->TrackChangesTo(kernel_);
     kernel_->put(SERVER_MTIME, value);
-    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
+    MarkDirty();
   }
 }
 
 void ModelNeutralMutableEntry::PutServerCtime(base::Time value) {
   DCHECK(kernel_);
-  base_write_transaction_->TrackChangesTo(kernel_);
   if (kernel_->ref(SERVER_CTIME) != value) {
+    base_write_transaction_->TrackChangesTo(kernel_);
     kernel_->put(SERVER_CTIME, value);
-    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
+    MarkDirty();
   }
 }
 
 bool ModelNeutralMutableEntry::PutId(const Id& value) {
   DCHECK(kernel_);
-  base_write_transaction_->TrackChangesTo(kernel_);
   if (kernel_->ref(ID) != value) {
+    base_write_transaction_->TrackChangesTo(kernel_);
     if (!dir()->ReindexId(base_write_transaction(), kernel_, value))
       return false;
-    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
+    MarkDirty();
   }
   return true;
 }
 
 void ModelNeutralMutableEntry::PutServerParentId(const Id& value) {
   DCHECK(kernel_);
-  base_write_transaction_->TrackChangesTo(kernel_);
-
   if (kernel_->ref(SERVER_PARENT_ID) != value) {
+    base_write_transaction_->TrackChangesTo(kernel_);
     kernel_->put(SERVER_PARENT_ID, value);
-    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
+    MarkDirty();
   }
 }
 
 bool ModelNeutralMutableEntry::PutIsUnsynced(bool value) {
   DCHECK(kernel_);
+  if (kernel_->ref(IS_UNSYNCED) != value) {
   base_write_transaction_->TrackChangesTo(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",
                       base_write_transaction())) {
         return false;
       }
     } else {
       if (!SyncAssert(1U == index->erase(kernel_->ref(META_HANDLE)),
                       FROM_HERE,
                       "Entry Not succesfully erased",
                       base_write_transaction())) {
         return false;
       }
     }
     kernel_->put(IS_UNSYNCED, value);
-    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
+    MarkDirty();
   }
   return true;
 }
 
 bool ModelNeutralMutableEntry::PutIsUnappliedUpdate(bool value) {
   DCHECK(kernel_);
-  base_write_transaction_->TrackChangesTo(kernel_);
   if (kernel_->ref(IS_UNAPPLIED_UPDATE) != value) {
+    base_write_transaction_->TrackChangesTo(kernel_);
     // 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",
                       base_write_transaction())) {
         return false;
       }
     } else {
       if (!SyncAssert(1U == index->erase(kernel_->ref(META_HANDLE)),
                       FROM_HERE,
                       "Entry Not succesfully erased",
                       base_write_transaction())) {
         return false;
       }
     }
     kernel_->put(IS_UNAPPLIED_UPDATE, value);
-    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
+    MarkDirty();
   }
   return true;
 }
 
 void ModelNeutralMutableEntry::PutServerIsDir(bool value) {
   DCHECK(kernel_);
-  base_write_transaction_->TrackChangesTo(kernel_);
-  bool old_value = kernel_->ref(SERVER_IS_DIR);
-  if (old_value != value) {
+  if (kernel_->ref(SERVER_IS_DIR) != value) {
+    base_write_transaction_->TrackChangesTo(kernel_);
     kernel_->put(SERVER_IS_DIR, value);
-    kernel_->mark_dirty(GetDirtyIndexHelper());
+    MarkDirty();
   }
 }
 
 void ModelNeutralMutableEntry::PutServerIsDel(bool value) {
   DCHECK(kernel_);
-  base_write_transaction_->TrackChangesTo(kernel_);
   bool old_value = kernel_->ref(SERVER_IS_DEL);
   if (old_value != value) {
+    base_write_transaction_->TrackChangesTo(kernel_);
     kernel_->put(SERVER_IS_DEL, value);
-    kernel_->mark_dirty(GetDirtyIndexHelper());
+    MarkDirty();
   }
 
   if (!value || kernel_->ref(IS_UNAPPLIED_UPDATE)) {
     // 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.
 
     // Please note that the delete journal applies only to the deletions
     // originating on the server side (hence the IS_UNAPPLIED_UPDATE check),
     // but it still makes sense to remove the entry from the delete journal
     // when it gets undeleted locally.
     dir()->delete_journal()->UpdateDeleteJournalForServerDelete(
         base_write_transaction(), old_value, *kernel_);
   }
 }
 
 void ModelNeutralMutableEntry::PutServerNonUniqueName(
     const std::string& value) {
   DCHECK(kernel_);
-  base_write_transaction_->TrackChangesTo(kernel_);
-
   if (kernel_->ref(SERVER_NON_UNIQUE_NAME) != value) {
+    base_write_transaction_->TrackChangesTo(kernel_);
     kernel_->put(SERVER_NON_UNIQUE_NAME, value);
-    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
+    MarkDirty();
   }
 }
 
 bool ModelNeutralMutableEntry::PutUniqueServerTag(const string& new_tag) {
   if (new_tag == kernel_->ref(UNIQUE_SERVER_TAG)) {
     return true;
   }
 
   base_write_transaction_->TrackChangesTo(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);
+  MarkDirty();
   if (!new_tag.empty()) {
     dir()->kernel()->server_tags_map[new_tag] = kernel_;
   }
 
   return true;
 }
 
 bool ModelNeutralMutableEntry::PutUniqueClientTag(const string& new_tag) {
   if (new_tag == kernel_->ref(UNIQUE_CLIENT_TAG)) {
     return true;
   }
 
   base_write_transaction_->TrackChangesTo(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);
+  MarkDirty();
   if (!new_tag.empty()) {
     dir()->kernel()->client_tags_map[new_tag] = kernel_;
   }
 
   return true;
 }
 
 void ModelNeutralMutableEntry::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;
   }
 
+  //TODO(stanisc): Does this need a call to TrackChangesTo?
+
   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));
   }
 
   kernel_->put(UNIQUE_BOOKMARK_TAG, tag);
-  kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
+  MarkDirty();
 }
 
 void ModelNeutralMutableEntry::PutServerSpecifics(
     const sync_pb::EntitySpecifics& value) {
   DCHECK(kernel_);
   CHECK(!value.password().has_client_only_encrypted_data());
-  base_write_transaction_->TrackChangesTo(kernel_);
   // TODO(ncarter): This is unfortunately heavyweight.  Can we do
   // better?
   const std::string& serialized_value = value.SerializeAsString();
   if (serialized_value != kernel_->ref(SERVER_SPECIFICS).SerializeAsString()) {
+    base_write_transaction_->TrackChangesTo(kernel_);
     if (kernel_->ref(IS_UNAPPLIED_UPDATE)) {
       // Remove ourselves from unapplied_update_metahandles with our
       // old server type.
       const ModelType old_server_type = kernel_->GetServerModelType();
       const int64 metahandle = kernel_->ref(META_HANDLE);
       size_t erase_count =
           dir()->kernel()->unapplied_update_metahandles[old_server_type]
           .erase(metahandle);
       DCHECK_EQ(erase_count, 1u);
     }
 
     // Check for potential sharing - SERVER_SPECIFICS is often
     // copied from SPECIFICS.
     if (serialized_value == kernel_->ref(SPECIFICS).SerializeAsString()) {
       kernel_->copy(SPECIFICS, SERVER_SPECIFICS);
     } else {
       kernel_->put(SERVER_SPECIFICS, value);
     }
-    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
+    MarkDirty();
 
     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();
       const int64 metahandle = kernel_->ref(META_HANDLE);
       dir()->kernel()->unapplied_update_metahandles[new_server_type]
           .insert(metahandle);
     }
   }
 }
 
 void ModelNeutralMutableEntry::PutBaseServerSpecifics(
     const sync_pb::EntitySpecifics& value) {
   DCHECK(kernel_);
   CHECK(!value.password().has_client_only_encrypted_data());
-  base_write_transaction_->TrackChangesTo(kernel_);
   // TODO(ncarter): This is unfortunately heavyweight.  Can we do
   // better?
   const std::string& serialized_value = value.SerializeAsString();
   if (serialized_value !=
       kernel_->ref(BASE_SERVER_SPECIFICS).SerializeAsString()) {
+    base_write_transaction_->TrackChangesTo(kernel_);
     // Check for potential sharing - BASE_SERVER_SPECIFICS is often
     // copied from SERVER_SPECIFICS.
     if (serialized_value ==
         kernel_->ref(SERVER_SPECIFICS).SerializeAsString()) {
       kernel_->copy(SERVER_SPECIFICS, BASE_SERVER_SPECIFICS);
     } else {
       kernel_->put(BASE_SERVER_SPECIFICS, value);
     }
-    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
+    MarkDirty();
   }
 }
 
 void ModelNeutralMutableEntry::PutServerUniquePosition(
     const UniquePosition& value) {
   DCHECK(kernel_);
-  base_write_transaction_->TrackChangesTo(kernel_);
   if(!kernel_->ref(SERVER_UNIQUE_POSITION).Equals(value)) {
+    base_write_transaction_->TrackChangesTo(kernel_);
     // We should never overwrite a valid position with an invalid one.
     DCHECK(value.IsValid());
     ScopedKernelLock lock(dir());
     kernel_->put(SERVER_UNIQUE_POSITION, value);
-    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
+    MarkDirty();
   }
 }
 
 void ModelNeutralMutableEntry::PutServerAttachmentMetadata(
     const sync_pb::AttachmentMetadata& value) {
   DCHECK(kernel_);
-  base_write_transaction_->TrackChangesTo(kernel_);
   const std::string& serialized_value = value.SerializeAsString();
   if (serialized_value !=
       kernel_->ref(SERVER_ATTACHMENT_METADATA).SerializeAsString()) {
+    base_write_transaction_->TrackChangesTo(kernel_);
     // Check for potential sharing - SERVER_ATTACHMENT_METADATA is often
     // copied from ATTACHMENT_METADATA.
     if (serialized_value ==
         kernel_->ref(ATTACHMENT_METADATA).SerializeAsString()) {
       kernel_->copy(ATTACHMENT_METADATA, SERVER_ATTACHMENT_METADATA);
     } else {
       kernel_->put(SERVER_ATTACHMENT_METADATA, value);
     }
-    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
+    MarkDirty();
   }
 }
 
 void ModelNeutralMutableEntry::PutSyncing(bool value) {
   kernel_->put(SYNCING, value);
 }
 
 void ModelNeutralMutableEntry::PutDirtySync(bool value) {
   DCHECK(!value || GetSyncing());
   kernel_->put(DIRTY_SYNC, value);
 }
 
 void ModelNeutralMutableEntry::PutParentIdPropertyOnly(const Id& parent_id) {
   base_write_transaction_->TrackChangesTo(kernel_);
   dir()->ReindexParentId(base_write_transaction(), kernel_, parent_id);
-  kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
+  MarkDirty();
 }
 
 void ModelNeutralMutableEntry::UpdateTransactionVersion(int64 value) {
-  ScopedKernelLock lock(dir());
   kernel_->put(TRANSACTION_VERSION, value);
-  kernel_->mark_dirty(&(dir()->kernel()->dirty_metahandles));
+  MarkDirty();
 }
 
 ModelNeutralMutableEntry::ModelNeutralMutableEntry(BaseWriteTransaction* trans)
   : Entry(trans), base_write_transaction_(trans) {}
 
-MetahandleSet* ModelNeutralMutableEntry::GetDirtyIndexHelper() {
-  return &dir()->kernel()->dirty_metahandles;
+void ModelNeutralMutableEntry::MarkDirty() {
+  kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
 }
 
 }  // namespace syncable
 
 }  // namespace syncer