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

sync/syncable/mutable_entry.cc

 // Copyright 2012 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/mutable_entry.h"
 
 #include "base/memory/scoped_ptr.h"
 #include "sync/internal_api/public/base/unique_position.h"
 #include "sync/syncable/directory.h"
 #include "sync/syncable/scoped_kernel_lock.h"
@@ skipping 104 matching lines @@
       write_transaction_(trans) {
 }
 
 MutableEntry::MutableEntry(WriteTransaction* trans, GetTypeRoot, ModelType type)
     : ModelNeutralMutableEntry(trans, GET_TYPE_ROOT, type),
       write_transaction_(trans) {
 }
 
 void MutableEntry::PutLocalExternalId(int64 value) {
   DCHECK(kernel_);
-  write_transaction()->TrackChangesTo(kernel_);
   if (kernel_->ref(LOCAL_EXTERNAL_ID) != value) {
+    write_transaction()->TrackChangesTo(kernel_);
     ScopedKernelLock lock(dir());
     kernel_->put(LOCAL_EXTERNAL_ID, value);
-    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
+    MarkDirty();
   }
 }
 
 void MutableEntry::PutMtime(base::Time value) {
   DCHECK(kernel_);
-  write_transaction()->TrackChangesTo(kernel_);
   if (kernel_->ref(MTIME) != value) {
+    write_transaction()->TrackChangesTo(kernel_);
     kernel_->put(MTIME, value);
-    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
+    MarkDirty();
   }
 }
 
 void MutableEntry::PutCtime(base::Time value) {
   DCHECK(kernel_);
-  write_transaction()->TrackChangesTo(kernel_);
   if (kernel_->ref(CTIME) != value) {
+    write_transaction()->TrackChangesTo(kernel_);
     kernel_->put(CTIME, value);
-    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
+    MarkDirty();
   }
 }
 
 void MutableEntry::PutParentId(const Id& value) {
   DCHECK(kernel_);
-  write_transaction()->TrackChangesTo(kernel_);
   if (kernel_->ref(PARENT_ID) != value) {
+    write_transaction()->TrackChangesTo(kernel_);
     PutParentIdPropertyOnly(value);
     if (!GetIsDel()) {
       if (!PutPredecessor(Id())) {
         // TODO(lipalani) : Propagate the error to caller. crbug.com/100444.
         NOTREACHED();
       }
     }
   }
 }
 
 void MutableEntry::PutIsDir(bool value) {
   DCHECK(kernel_);
-  write_transaction()->TrackChangesTo(kernel_);
-  bool old_value = kernel_->ref(IS_DIR);
-  if (old_value != value) {
+  if (kernel_->ref(IS_DIR) != value) {
+    write_transaction()->TrackChangesTo(kernel_);
     kernel_->put(IS_DIR, value);
-    kernel_->mark_dirty(GetDirtyIndexHelper());
+    MarkDirty();
   }
 }
 
 void MutableEntry::PutIsDel(bool value) {
   DCHECK(kernel_);
-  write_transaction()->TrackChangesTo(kernel_);
   if (value == kernel_->ref(IS_DEL)) {
     return;
   }
+
+  write_transaction()->TrackChangesTo(kernel_);
   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.
     // - Allow any items with the same UNIQUE_CLIENT_TAG created on other
     //   clients to override this entry.
     // - Let us delete this entry permanently when we next restart sync - see
     //   DirectoryBackingStore::SafeToPurgeOnLoading.
     //   This will avoid crbug.com/125381.
     // Note: do not unset IsUnsynced if the syncer has started but not yet
     // finished committing this entity.
     if (!GetId().ServerKnows() && !GetSyncing()) {
       PutIsUnsynced(false);
     }
   }
 
   {
     ScopedKernelLock lock(dir());
     // Some indices don't include deleted items and must be updated
     // upon a value change.
     ScopedParentChildIndexUpdater updater(lock, kernel_,
         &dir()->kernel()->parent_child_index);
 
     kernel_->put(IS_DEL, value);
-    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
+    MarkDirty();
   }
 }
 
 void MutableEntry::PutNonUniqueName(const std::string& value) {
   DCHECK(kernel_);
-  write_transaction()->TrackChangesTo(kernel_);
-
   if (kernel_->ref(NON_UNIQUE_NAME) != value) {
+    write_transaction()->TrackChangesTo(kernel_);
     kernel_->put(NON_UNIQUE_NAME, value);
-    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
+    MarkDirty();
   }
 }
 
 void MutableEntry::PutSpecifics(const sync_pb::EntitySpecifics& value) {
   DCHECK(kernel_);
   CHECK(!value.password().has_client_only_encrypted_data());
-  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(SPECIFICS).SerializeAsString()) {
+    write_transaction()->TrackChangesTo(kernel_);
     // Check for potential sharing - SPECIFICS is often
     // copied from SERVER_SPECIFICS.
     if (serialized_value ==
         kernel_->ref(SERVER_SPECIFICS).SerializeAsString()) {
       kernel_->copy(SERVER_SPECIFICS, SPECIFICS);
     } else {
       kernel_->put(SPECIFICS, value);
     }
-    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
+    MarkDirty();
   }
 }
 
 void MutableEntry::PutUniquePosition(const UniquePosition& value) {
   DCHECK(kernel_);
-  write_transaction()->TrackChangesTo(kernel_);
   if(!kernel_->ref(UNIQUE_POSITION).Equals(value)) {
+    write_transaction()->TrackChangesTo(kernel_);
     // 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);
+    MarkDirty();
   }
 }
 
 bool MutableEntry::PutPredecessor(const Id& predecessor_id) {
   if (predecessor_id.IsNull()) {
     dir()->PutPredecessor(kernel_, NULL);
   } else {
     MutableEntry predecessor(write_transaction(), GET_BY_ID, predecessor_id);
     if (!predecessor.good())
       return false;
     dir()->PutPredecessor(kernel_, predecessor.kernel_);
+    DCHECK(kernel_->is_dirty());
   }
   return true;
 }
 
 void MutableEntry::PutAttachmentMetadata(
     const sync_pb::AttachmentMetadata& value) {
   DCHECK(kernel_);
-  write_transaction()->TrackChangesTo(kernel_);
   const std::string& serialized_value = value.SerializeAsString();
   if (serialized_value !=
       kernel_->ref(ATTACHMENT_METADATA).SerializeAsString()) {
+    write_transaction()->TrackChangesTo(kernel_);
     dir()->UpdateAttachmentIndex(GetMetahandle(),
                                  kernel_->ref(ATTACHMENT_METADATA), value);
     // Check for potential sharing - ATTACHMENT_METADATA is often
     // copied from SERVER_ATTACHMENT_METADATA.
     if (serialized_value ==
         kernel_->ref(SERVER_ATTACHMENT_METADATA).SerializeAsString()) {
       kernel_->copy(SERVER_ATTACHMENT_METADATA, ATTACHMENT_METADATA);
     } else {
       kernel_->put(ATTACHMENT_METADATA, value);
     }
-    kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
+    MarkDirty();
   }
 }
 
 void MutableEntry::MarkAttachmentAsOnServer(
     const sync_pb::AttachmentIdProto& attachment_id) {
   DCHECK(kernel_);
   DCHECK(!attachment_id.unique_id().empty());
   write_transaction()->TrackChangesTo(kernel_);
   sync_pb::AttachmentMetadata attachment_metadata =
       kernel_->ref(ATTACHMENT_METADATA);
   for (int i = 0; i < attachment_metadata.record_size(); ++i) {
     sync_pb::AttachmentMetadataRecord* record =
         attachment_metadata.mutable_record(i);
     if (record->id().unique_id() != attachment_id.unique_id())
       continue;
     record->set_is_on_server(true);
   }
   kernel_->put(ATTACHMENT_METADATA, attachment_metadata);
-  kernel_->mark_dirty(&dir()->kernel()->dirty_metahandles);
+  MarkDirty();
   MarkForSyncing(this);
 }
 
 // This function sets only the flags needed to get this entry to sync.
 bool MarkForSyncing(MutableEntry* e) {
   DCHECK_NE(static_cast<MutableEntry*>(NULL), e);
   DCHECK(!e->IsRoot()) << "We shouldn't mark a permanent object for syncing.";
   if (!(e->PutIsUnsynced(true)))
     return false;
   if (e->GetSyncing())
     e->PutDirtySync(true);
   return true;
 }
 
 }  // namespace syncable
 }  // namespace syncer