Populate versions on individual nodes in sync model and native bookmark model.

chrome/browser/sync/glue/bookmark_change_processor.cc

 // Copyright (c) 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 "chrome/browser/sync/glue/bookmark_change_processor.h"
 
 #include <stack>
 #include <vector>
 
 #include "base/location.h"
@@ skipping 95 matching lines @@
   }
   // This node should have no children.
   DCHECK(!sync_node.HasChildren());
   // Remove association and delete the sync node.
   model_associator_->Disassociate(sync_node.GetId());
   sync_node.Remove();
 }
 
 void BookmarkChangeProcessor::RemoveSyncNodeHierarchy(
     const BookmarkNode* topmost) {
-  syncer::WriteTransaction trans(FROM_HERE, share_handle());
+  int64 new_version = -1;
+  {
+    syncer::WriteTransaction trans(FROM_HERE, share_handle(), &new_version);
 
-  // Later logic assumes that |topmost| has been unlinked.
-  DCHECK(topmost->is_root());
+    // Later logic assumes that |topmost| has been unlinked.
+    DCHECK(topmost->is_root());
 
-  // A BookmarkModel deletion event means that |node| and all its children were
-  // deleted. Sync backend expects children to be deleted individually, so we do
-  // a depth-first-search here.  At each step, we consider the |index|-th child
-  // of |node|.  |index_stack| stores index values for the parent levels.
-  std::stack<int> index_stack;
-  index_stack.push(0);  // For the final pop.  It's never used.
-  const BookmarkNode* node = topmost;
-  int index = 0;
-  while (node) {
-    // The top of |index_stack| should always be |node|'s index.
-    DCHECK(node->is_root() || (node->parent()->GetIndexOf(node) ==
-      index_stack.top()));
-    if (index == node->child_count()) {
-      // If we've processed all of |node|'s children, delete |node| and move
-      // on to its successor.
-      RemoveOneSyncNode(&trans, node);
-      node = node->parent();
-      index = index_stack.top() + 1;      // (top() + 0) was what we removed.
-      index_stack.pop();
-    } else {
-      // If |node| has an unprocessed child, process it next after pushing the
-      // current state onto the stack.
-      DCHECK_LT(index, node->child_count());
-      index_stack.push(index);
-      node = node->GetChild(index);
-      index = 0;
+    // A BookmarkModel deletion event means that |node| and all its children
+    // were deleted. Sync backend expects children to be deleted individually,
+    // so we do a depth-first-search here.  At each step, we consider the
+    // |index|-th child of |node|.  |index_stack| stores index values for the
+    // parent levels.
+    std::stack<int> index_stack;
+    index_stack.push(0);  // For the final pop.  It's never used.
+    const BookmarkNode* node = topmost;
+    int index = 0;
+    while (node) {
+      // The top of |index_stack| should always be |node|'s index.
+      DCHECK(node->is_root() || (node->parent()->GetIndexOf(node) ==
+        index_stack.top()));
+      if (index == node->child_count()) {
+        // If we've processed all of |node|'s children, delete |node| and move
+        // on to its successor.
+        RemoveOneSyncNode(&trans, node);
+        node = node->parent();
+        index = index_stack.top() + 1;      // (top() + 0) was what we removed.
+        index_stack.pop();
+      } else {
+        // If |node| has an unprocessed child, process it next after pushing the
+        // current state onto the stack.
+        DCHECK_LT(index, node->child_count());
+        index_stack.push(index);
+        node = node->GetChild(index);
+        index = 0;
+      }
     }
+    DCHECK(index_stack.empty());  // Nothing should be left on the stack.
   }
-  DCHECK(index_stack.empty());  // Nothing should be left on the stack.
+
+  // Don't need to update versions of deleted nodes.
+  UpdateTransactionVersion(new_version, bookmark_model_,
+                           std::vector<const BookmarkNode*>());
 }
 
 void BookmarkChangeProcessor::Loaded(BookmarkModel* model,
                                      bool ids_reassigned) {
   NOTREACHED();
 }
 
 void BookmarkChangeProcessor::BookmarkModelBeingDeleted(
     BookmarkModel* model) {
   NOTREACHED();
   bookmark_model_ = NULL;
 }
 
 void BookmarkChangeProcessor::BookmarkNodeAdded(BookmarkModel* model,
                                                 const BookmarkNode* parent,
                                                 int index) {
   DCHECK(share_handle());
 
-  // Acquire a scoped write lock via a transaction.
-  syncer::WriteTransaction trans(FROM_HERE, share_handle());
+  int64 new_version = -1;
+  int64 sync_id = syncer::kInvalidId;
+  {
+    // Acquire a scoped write lock via a transaction.
+    syncer::WriteTransaction trans(FROM_HERE, share_handle(), &new_version);
+    sync_id = CreateSyncNode(parent, model, index, &trans,
+                             model_associator_, error_handler());
+  }
 
-  CreateSyncNode(parent, model, index, &trans, model_associator_,
-                 error_handler());
+  if (syncer::kInvalidId != sync_id) {
+    // NOTE(haitaol): Siblings of added node in sync DB will also be updated to

[Nicolas Zea, 2012/11/02 21:02:25]

I don't think you need to mark this as a note for yourself. It's a useful
comment anyways, so should just be a normal comment.

[haitaol1, 2012/11/02 22:58:18]

Done.

+    //                reflect new PREV_ID/NEXT_ID and thus get a new version.
+    //                But we only update version of added node here. After
+    //                switching to ordinals for positioning, PREV_ID/NEXT_ID
+    //                will be deprecated and siblings will not be updated.
+    UpdateTransactionVersion(
+        new_version, model,
+        std::vector<const BookmarkNode*>(1, parent->GetChild(index)));
+  }
 }
 
 // static
 int64 BookmarkChangeProcessor::CreateSyncNode(const BookmarkNode* parent,
     BookmarkModel* model, int index, syncer::WriteTransaction* trans,
     BookmarkModelAssociator* associator,
     DataTypeErrorHandler* error_handler) {
   const BookmarkNode* child = parent->GetChild(index);
   DCHECK(child);
 
   // Create a WriteNode container to hold the new node.
   syncer::WriteNode sync_child(trans);
 
   // Actually create the node with the appropriate initial position.
   if (!PlaceSyncNode(CREATE, parent, index, trans, &sync_child, associator)) {
     error_handler->OnSingleDatatypeUnrecoverableError(FROM_HERE,
         "Sync node creation failed; recovery unlikely");
     return syncer::kInvalidId;
   }
 
   UpdateSyncNodeProperties(child, model, &sync_child);
 
   // Associate the ID from the sync domain with the bookmark node, so that we
   // can refer back to this item later.
   associator->Associate(child, sync_child.GetId());
 
   return sync_child.GetId();
 }
 
-
 void BookmarkChangeProcessor::BookmarkNodeRemoved(BookmarkModel* model,
-                                                    const BookmarkNode* parent,
-                                                    int index,
-                                                    const BookmarkNode* node) {
+                                                  const BookmarkNode* parent,

[Nicolas Zea, 2012/11/02 21:02:25]

fix indent

[haitaol1, 2012/11/02 22:58:18]

This looks fine on my desktop and I verified that they are all spaces.

On 2012/11/02 21:02:25, Nicolas Zea wrote:

+                                                  int index,
+                                                  const BookmarkNode* node) {
   RemoveSyncNodeHierarchy(node);
 }
 
 void BookmarkChangeProcessor::BookmarkNodeChanged(BookmarkModel* model,
                                                   const BookmarkNode* node) {
   // We shouldn't see changes to the top-level nodes.
   if (model->is_permanent_node(node)) {
     NOTREACHED() << "Saw update to permanent node!";
     return;
   }
 
-  // Acquire a scoped write lock via a transaction.
-  syncer::WriteTransaction trans(FROM_HERE, share_handle());
+  int64 new_version = -1;
+  {
+    // Acquire a scoped write lock via a transaction.
+    syncer::WriteTransaction trans(FROM_HERE, share_handle(), &new_version);
 
-  // Lookup the sync node that's associated with |node|.
-  syncer::WriteNode sync_node(&trans);
-  if (!model_associator_->InitSyncNodeFromChromeId(node->id(), &sync_node)) {
-    // TODO(tim): Investigating bug 121587.
-    if (model_associator_->GetSyncIdFromChromeId(node->id()) ==
-                                                 syncer::kInvalidId) {
-      error_handler()->OnSingleDatatypeUnrecoverableError(FROM_HERE,
-          "Bookmark id not found in model associator on BookmarkNodeChanged");
-      LOG(ERROR) << "Bad id.";
-    } else if (!sync_node.GetEntry()->good()) {
-      error_handler()->OnSingleDatatypeUnrecoverableError(FROM_HERE,
-          "Could not InitByIdLookup on BookmarkNodeChanged, good() failed");
-      LOG(ERROR) << "Bad entry.";
-    } else if (sync_node.GetEntry()->Get(syncer::syncable::IS_DEL)) {
-      error_handler()->OnSingleDatatypeUnrecoverableError(FROM_HERE,
-          "Could not InitByIdLookup on BookmarkNodeChanged, is_del true");
-      LOG(ERROR) << "Deleted entry.";
-    } else {
-      syncer::Cryptographer* crypto = trans.GetCryptographer();
-      syncer::ModelTypeSet encrypted_types(trans.GetEncryptedTypes());
-      const sync_pb::EntitySpecifics& specifics =
-          sync_node.GetEntry()->Get(syncer::syncable::SPECIFICS);
-      CHECK(specifics.has_encrypted());
-      const bool can_decrypt = crypto->CanDecrypt(specifics.encrypted());
-      const bool agreement = encrypted_types.Has(syncer::BOOKMARKS);
-      if (!agreement && !can_decrypt) {
+    // Lookup the sync node that's associated with |node|.
+    syncer::WriteNode sync_node(&trans);
+    if (!model_associator_->InitSyncNodeFromChromeId(node->id(), &sync_node)) {
+      // TODO(tim): Investigating bug 121587.
+      if (model_associator_->GetSyncIdFromChromeId(node->id()) ==
+                                                   syncer::kInvalidId) {
         error_handler()->OnSingleDatatypeUnrecoverableError(FROM_HERE,
-            "Could not InitByIdLookup on BookmarkNodeChanged, "
-            " Cryptographer thinks bookmarks not encrypted, and CanDecrypt"
-            " failed.");
-        LOG(ERROR) << "Case 1.";
-      } else if (agreement && can_decrypt) {
+            "Bookmark id not found in model associator on BookmarkNodeChanged");
+        LOG(ERROR) << "Bad id.";
+      } else if (!sync_node.GetEntry()->good()) {
         error_handler()->OnSingleDatatypeUnrecoverableError(FROM_HERE,
-            "Could not InitByIdLookup on BookmarkNodeChanged, "
-            " Cryptographer thinks bookmarks are encrypted, and CanDecrypt"
-            " succeeded (?!), but DecryptIfNecessary failed.");
-        LOG(ERROR) << "Case 2.";
-      } else if (agreement) {
+            "Could not InitByIdLookup on BookmarkNodeChanged, good() failed");
+        LOG(ERROR) << "Bad entry.";
+      } else if (sync_node.GetEntry()->Get(syncer::syncable::IS_DEL)) {
         error_handler()->OnSingleDatatypeUnrecoverableError(FROM_HERE,
-            "Could not InitByIdLookup on BookmarkNodeChanged, "
-            " Cryptographer thinks bookmarks are encrypted, but CanDecrypt"
-            " failed.");
-        LOG(ERROR) << "Case 3.";
+            "Could not InitByIdLookup on BookmarkNodeChanged, is_del true");
+        LOG(ERROR) << "Deleted entry.";
       } else {
-        error_handler()->OnSingleDatatypeUnrecoverableError(FROM_HERE,
-            "Could not InitByIdLookup on BookmarkNodeChanged, "
-            " Cryptographer thinks bookmarks not encrypted, but CanDecrypt"
-            " succeeded (super weird, btw)");
-        LOG(ERROR) << "Case 4.";
+        syncer::Cryptographer* crypto = trans.GetCryptographer();
+        syncer::ModelTypeSet encrypted_types(trans.GetEncryptedTypes());
+        const sync_pb::EntitySpecifics& specifics =
+            sync_node.GetEntry()->Get(syncer::syncable::SPECIFICS);
+        CHECK(specifics.has_encrypted());
+        const bool can_decrypt = crypto->CanDecrypt(specifics.encrypted());
+        const bool agreement = encrypted_types.Has(syncer::BOOKMARKS);
+        if (!agreement && !can_decrypt) {
+          error_handler()->OnSingleDatatypeUnrecoverableError(FROM_HERE,
+              "Could not InitByIdLookup on BookmarkNodeChanged, "
+              " Cryptographer thinks bookmarks not encrypted, and CanDecrypt"
+              " failed.");
+          LOG(ERROR) << "Case 1.";
+        } else if (agreement && can_decrypt) {
+          error_handler()->OnSingleDatatypeUnrecoverableError(FROM_HERE,
+              "Could not InitByIdLookup on BookmarkNodeChanged, "
+              " Cryptographer thinks bookmarks are encrypted, and CanDecrypt"
+              " succeeded (?!), but DecryptIfNecessary failed.");
+          LOG(ERROR) << "Case 2.";
+        } else if (agreement) {
+          error_handler()->OnSingleDatatypeUnrecoverableError(FROM_HERE,
+              "Could not InitByIdLookup on BookmarkNodeChanged, "
+              " Cryptographer thinks bookmarks are encrypted, but CanDecrypt"
+              " failed.");
+          LOG(ERROR) << "Case 3.";
+        } else {
+          error_handler()->OnSingleDatatypeUnrecoverableError(FROM_HERE,
+              "Could not InitByIdLookup on BookmarkNodeChanged, "
+              " Cryptographer thinks bookmarks not encrypted, but CanDecrypt"
+              " succeeded (super weird, btw)");
+          LOG(ERROR) << "Case 4.";
+        }
       }
+      return;
     }
-    return;
+
+    UpdateSyncNodeProperties(node, model, &sync_node);
+
+    DCHECK_EQ(sync_node.GetIsFolder(), node->is_folder());
+    DCHECK_EQ(model_associator_->GetChromeNodeFromSyncId(
+              sync_node.GetParentId()),
+              node->parent());
+    // This node's index should be one more than the predecessor's index.
+    DCHECK_EQ(node->parent()->GetIndexOf(node),
+              CalculateBookmarkModelInsertionIndex(node->parent(),
+                                                   &sync_node,
+                                                   model_associator_));
   }
 
-  UpdateSyncNodeProperties(node, model, &sync_node);
-
-  DCHECK_EQ(sync_node.GetIsFolder(), node->is_folder());
-  DCHECK_EQ(model_associator_->GetChromeNodeFromSyncId(
-            sync_node.GetParentId()),
-            node->parent());
-  // This node's index should be one more than the predecessor's index.
-  DCHECK_EQ(node->parent()->GetIndexOf(node),
-            CalculateBookmarkModelInsertionIndex(node->parent(),
-                                                 &sync_node,
-                                                 model_associator_));
+  UpdateTransactionVersion(new_version, model,
+                           std::vector<const BookmarkNode*>(1, node));
 }
 
-
 void BookmarkChangeProcessor::BookmarkNodeMoved(BookmarkModel* model,
       const BookmarkNode* old_parent, int old_index,
       const BookmarkNode* new_parent, int new_index) {
   const BookmarkNode* child = new_parent->GetChild(new_index);
   // We shouldn't see changes to the top-level nodes.
   if (model->is_permanent_node(child)) {
     NOTREACHED() << "Saw update to permanent node!";
     return;
   }
 
-  // Acquire a scoped write lock via a transaction.
-  syncer::WriteTransaction trans(FROM_HERE, share_handle());
+  int64 new_version = -1;
+  {
+    // Acquire a scoped write lock via a transaction.
+    syncer::WriteTransaction trans(FROM_HERE, share_handle(), &new_version);
 
-  // Lookup the sync node that's associated with |child|.
-  syncer::WriteNode sync_node(&trans);
-  if (!model_associator_->InitSyncNodeFromChromeId(child->id(), &sync_node)) {
-    error_handler()->OnSingleDatatypeUnrecoverableError(FROM_HERE,
-                                                        std::string());
-    return;
+    // Lookup the sync node that's associated with |child|.
+    syncer::WriteNode sync_node(&trans);
+    if (!model_associator_->InitSyncNodeFromChromeId(child->id(), &sync_node)) {
+      error_handler()->OnSingleDatatypeUnrecoverableError(FROM_HERE,
+                                                          std::string());
+      return;
+    }
+
+    if (!PlaceSyncNode(MOVE, new_parent, new_index, &trans, &sync_node,
+                       model_associator_)) {
+      error_handler()->OnSingleDatatypeUnrecoverableError(FROM_HERE,
+                                                          std::string());
+      return;
+    }
   }
 
-  if (!PlaceSyncNode(MOVE, new_parent, new_index, &trans, &sync_node,
-                     model_associator_)) {
-    error_handler()->OnSingleDatatypeUnrecoverableError(FROM_HERE,
-                                                        std::string());
-    return;
-  }
+  UpdateTransactionVersion(new_version, model,
+                           std::vector<const BookmarkNode*>(1, child));
 }
 
 void BookmarkChangeProcessor::BookmarkNodeFaviconChanged(
     BookmarkModel* model,
     const BookmarkNode* node) {
   BookmarkNodeChanged(model, node);
 }
 
 void BookmarkChangeProcessor::BookmarkNodeChildrenReordered(
     BookmarkModel* model, const BookmarkNode* node) {
+  int64 new_version = -1;
+  std::vector<const BookmarkNode*> children;
+  {
+    // Acquire a scoped write lock via a transaction.
+    syncer::WriteTransaction trans(FROM_HERE, share_handle(), &new_version);
 
-  // Acquire a scoped write lock via a transaction.
-  syncer::WriteTransaction trans(FROM_HERE, share_handle());
+    // The given node's children got reordered. We need to reorder all the
+    // children of the corresponding sync node.
+    for (int i = 0; i < node->child_count(); ++i) {
+      const BookmarkNode* child = node->GetChild(i);
+      children.push_back(child);
 
-  // The given node's children got reordered. We need to reorder all the
-  // children of the corresponding sync node.
-  for (int i = 0; i < node->child_count(); ++i) {
-    syncer::WriteNode sync_child(&trans);
-    if (!model_associator_->InitSyncNodeFromChromeId(node->GetChild(i)->id(),
-                                                     &sync_child)) {
-      error_handler()->OnSingleDatatypeUnrecoverableError(FROM_HERE,
-                                                          std::string());
-      return;
-    }
-    DCHECK_EQ(sync_child.GetParentId(),
-              model_associator_->GetSyncIdFromChromeId(node->id()));
+      syncer::WriteNode sync_child(&trans);
+      if (!model_associator_->InitSyncNodeFromChromeId(child->id(),
+                                                       &sync_child)) {
+        error_handler()->OnSingleDatatypeUnrecoverableError(FROM_HERE,
+                                                            std::string());
+        return;
+      }
+      DCHECK_EQ(sync_child.GetParentId(),
+                model_associator_->GetSyncIdFromChromeId(node->id()));
 
-    if (!PlaceSyncNode(MOVE, node, i, &trans, &sync_child,
-                       model_associator_)) {
-      error_handler()->OnSingleDatatypeUnrecoverableError(FROM_HERE,
-                                                          std::string());
-      return;
+      if (!PlaceSyncNode(MOVE, node, i, &trans, &sync_child,
+                         model_associator_)) {
+        error_handler()->OnSingleDatatypeUnrecoverableError(FROM_HERE,
+                                                            std::string());
+        return;
+      }
     }
   }
+
+  // TODO(haitaol): Filter out children that didn't actually change.
+  UpdateTransactionVersion(new_version, model, children);
 }
 
 // static
 bool BookmarkChangeProcessor::PlaceSyncNode(MoveOrCreate operation,
       const BookmarkNode* parent, int index, syncer::WriteTransaction* trans,
       syncer::WriteNode* dst, BookmarkModelAssociator* associator) {
   syncer::ReadNode sync_parent(trans);
   if (!associator->InitSyncNodeFromChromeId(parent->id(), &sync_parent)) {
     LOG(WARNING) << "Parent lookup failed";
     return false;
@@ skipping 138 matching lines @@
           << "ACTION_ADD should be seen if and only if the node is unknown.";
       passed_deletes = true;
 
       syncer::ReadNode src(trans);
       if (src.InitByIdLookup(it->id) != syncer::BaseNode::INIT_OK) {
         error_handler()->OnSingleDatatypeUnrecoverableError(FROM_HERE,
             "ApplyModelChanges was passed a bad ID");
         return;
       }
 
-      if (!CreateOrUpdateBookmarkNode(&src, model, model_associator_)) {
+      const BookmarkNode* node = CreateOrUpdateBookmarkNode(&src, model,
+                                                            model_associator_);
+      if (node) {
+        bookmark_model_->SetNodeMetaInfo(node, kBookmarkTransactionVersionKey,
+                                         base::Int64ToString(model_version));
+      } else {
         // Because the Synced Bookmarks node can be created server side, it's
         // possible it'll arrive at the client as an update. In that case it
         // won't have been associated at startup, the GetChromeNodeFromSyncId
         // call above will return NULL, and we won't detect it as a permanent
         // node, resulting in us trying to create it here (which will
         // fail). Therefore, we add special logic here just to detect the
         // Synced Bookmarks folder.
         syncer::ReadNode synced_bookmarks(trans);
         if (synced_bookmarks.InitByTagLookup(kMobileBookmarksTag) ==
                 syncer::BaseNode::INIT_OK &&
@@ skipping 72 matching lines @@
                           base::Time::FromInternalValue(
                               src->GetBookmarkSpecifics().creation_time_us()));
     }
 
     SetBookmarkFavicon(src, dst, model);
   }
 
   return dst;
 }
 
+void BookmarkChangeProcessor::UpdateTransactionVersion(
+    int64 new_version,
+    BookmarkModel* model,
+    const std::vector<const BookmarkNode*>& nodes) {
+  if (new_version > 0) {

[Nicolas Zea, 2012/11/02 21:02:25]

is it possible to verify that new_version is > existing version?

[haitaol1, 2012/11/02 22:58:18]

To do that, I'll need to query old version after transaction's created. For
verification purpose only, I'd like to verify in tests.

On 2012/11/02 21:02:25, Nicolas Zea wrote:

+    model->SetNodeMetaInfo(model->root_node(), kBookmarkTransactionVersionKey,
+                           base::Int64ToString(new_version));
+    for (uint32 i = 0; i < nodes.size(); ++i) {
+      model->SetNodeMetaInfo(nodes[i], kBookmarkTransactionVersionKey,
+                             base::Int64ToString(new_version));
+    }
+  }
+}
+
 // static
 // Creates a bookmark node under the given parent node from the given sync
 // node. Returns the newly created node.
 const BookmarkNode* BookmarkChangeProcessor::CreateBookmarkNode(
     syncer::BaseNode* sync_node,
     const BookmarkNode* parent,
     BookmarkModel* model,
     int index) {
   DCHECK(parent);
   DCHECK(index >= 0 && index <= parent->child_count());
@@ skipping 65 matching lines @@
     const BookmarkNode* bookmark_node,
     BookmarkModel* model,
     syncer::WriteNode* sync_node) {
   std::vector<unsigned char> favicon_bytes;
   EncodeFavicon(bookmark_node, model, &favicon_bytes);
   if (!favicon_bytes.empty())
     sync_node->SetFaviconBytes(favicon_bytes);
 }
 
 }  // namespace browser_sync