Prevent Meta Info from being overwritten due to reordering of entries.

chrome/browser/sync/profile_sync_service_bookmark_unittest.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.
 
 // TODO(akalin): This file is basically just a unit test for
 // BookmarkChangeProcessor.  Write unit tests for
 // BookmarkModelAssociator separately.
 
 #include <map>
 #include <queue>
@@ skipping 81 matching lines @@
       EXPECT_EQ(BaseNode::INIT_OK, predecessor.InitByIdLookup(predecessor_id));
       EXPECT_EQ(predecessor.GetParentId(), parent.GetId());
       EXPECT_TRUE(node.InitBookmarkByCreation(parent, &predecessor));
     }
     EXPECT_EQ(node.GetPredecessorId(), predecessor_id);
     EXPECT_EQ(node.GetParentId(), parent_id);
     node.SetIsFolder(is_folder);
     node.SetTitle(title);
 
     sync_pb::BookmarkSpecifics specifics(node.GetBookmarkSpecifics());
+    const base::Time creation_time(base::Time::Now());
+    specifics.set_creation_time_us(creation_time.ToInternalValue());
     if (!is_folder)
       specifics.set_url(url);
     if (meta_info_map)
       SetNodeMetaInfo(*meta_info_map, &specifics);
     node.SetBookmarkSpecifics(specifics);
 
     syncer::ChangeRecord record;
     record.action = syncer::ChangeRecord::ACTION_ADD;
     record.id = node.GetId();
     changes_.push_back(record);
@@ skipping 138 matching lines @@
       return;
     syncer::ChangeRecord record;
     record.action = syncer::ChangeRecord::ACTION_UPDATE;
     record.id = id;
     changes_.push_back(record);
   }
 
   void SetNodeMetaInfo(const BookmarkNode::MetaInfoMap& meta_info_map,
                        sync_pb::BookmarkSpecifics* specifics) {
     specifics->clear_meta_info();
-    for (BookmarkNode::MetaInfoMap::const_iterator it =
-        meta_info_map.begin(); it != meta_info_map.end(); ++it) {
+    // Deliberatly set MetaInfoMap entries in opposite order (compared
+    // to the implementation in BookmarkChangeProcessor) to ensure that
+    // (a) the implementation isn't sensitive to the order and
+    // (b) the original meta info isn't blindly overwritten by
+    //     BookmarkChangeProcessor unless there is a real change.
+    BookmarkNode::MetaInfoMap::const_iterator it = meta_info_map.end();

[Nicolas Zea, 2015/02/04 22:46:21]

nit: leave as for loop?

[stanisc, 2015/02/05 01:40:15]

I have considered the for loop, but couldn't find an elegant way to code it. For
loop doesn't work well in this case because the end isn't included (which means
it needs to be decremented first) but the start has to be included (which means
the body has to be executed one more time after detecting that it ==
meta_info_map.begin().

+    while (it != meta_info_map.begin()) {
+      --it;
       sync_pb::MetaInfo* meta_info = specifics->add_meta_info();
       meta_info->set_key(it->first);
       meta_info->set_value(it->second);
     }
   }
 
-
   // The transaction on which everything happens.
   syncer::WriteTransaction *trans_;
 
   // The change list we construct.
   syncer::ChangeRecordList changes_;
 };
 
 class ExtensiveChangesBookmarkModelObserver
     : public bookmarks::BaseBookmarkModelObserver {
  public:
@@ skipping 1736 matching lines @@
   ExpectModelMatch();
 
   // Change/delete existing meta info and verify.
   model_->DeleteNodeMetaInfo(folder_node, "folder");
   model_->SetNodeMetaInfo(node, "node", "changednodevalue");
   model_->DeleteNodeMetaInfo(node, "other");
   model_->SetNodeMetaInfo(node, "newkey", "newkeyvalue");
   ExpectModelMatch();
 }
 
+// Tests that

[Nicolas Zea, 2015/02/04 22:46:21]

Finish comment?

[stanisc, 2015/02/05 01:40:15]

Done.

+TEST_F(ProfileSyncServiceBookmarkTestWithData, MetaInfoPreservedOnNonChange) {
+  LoadBookmarkModel(DELETE_EXISTING_STORAGE, DONT_SAVE_TO_STORAGE);
+  WriteTestDataToBookmarkModel();
+  StartSync();
+
+  std::string orig_specifics;
+  int64 sync_id;
+  const BookmarkNode* bookmark;
+
+  // Create bookmark folder node containing meta info.
+  {
+    syncer::WriteTransaction trans(FROM_HERE, test_user_share_.user_share());
+    FakeServerChange adds(&trans);
+
+    int64 folder_id = adds.AddFolder("folder title", bookmark_bar_id(), 0);
+
+    BookmarkNode::MetaInfoMap node_meta_info;
+    node_meta_info["one"] = "1";
+    node_meta_info["two"] = "2";
+    node_meta_info["three"] = "3";
+
+    sync_id = adds.AddURLWithMetaInfo("node title", "http://www.foo.com/",
+                                      &node_meta_info, folder_id, 0);
+
+    // Verify that the node propagates to the bookmark model
+    adds.ApplyPendingChanges(change_processor_.get());
+
+    bookmark = model_->bookmark_bar_node()->GetChild(0)->GetChild(0);
+    EXPECT_EQ(node_meta_info, *bookmark->GetMetaInfoMap());
+
+    syncer::ReadNode sync_node(&trans);
+    EXPECT_EQ(BaseNode::INIT_OK, sync_node.InitByIdLookup(sync_id));
+    orig_specifics = sync_node.GetBookmarkSpecifics().SerializeAsString();
+  }
+
+  // Force change processor to update the sync node.
+  change_processor_->BookmarkMetaInfoChanged(model_, bookmark);
+
+  // Read bookmark specifics again and verify that there is no change.
+  {
+    syncer::ReadTransaction trans(FROM_HERE, test_user_share_.user_share());
+    syncer::ReadNode sync_node(&trans);
+    EXPECT_EQ(BaseNode::INIT_OK, sync_node.InitByIdLookup(sync_id));
+    std::string new_specifics =
+        sync_node.GetBookmarkSpecifics().SerializeAsString();
+    ASSERT_EQ(orig_specifics, new_specifics);
+  }
+}
+
 void ProfileSyncServiceBookmarkTestWithData::GetTransactionVersions(
     const BookmarkNode* root,
     BookmarkNodeVersionMap* node_versions) {
   node_versions->clear();
   std::queue<const BookmarkNode*> nodes;
   nodes.push(root);
   while (!nodes.empty()) {
     const BookmarkNode* n = nodes.front();
     nodes.pop();
 
@@ skipping 151 matching lines @@
   ExpectModelMatch();
 
   // Then simulate the add call arriving late.
   change_processor_->BookmarkNodeAdded(model_, model_->bookmark_bar_node(), 0);
   ExpectModelMatch();
 }
 
 }  // namespace
 
 }  // namespace browser_sync