Only set remote id during url node creation.

components/enhanced_bookmarks/enhanced_bookmark_model_unittest.cc

Index: components/enhanced_bookmarks/enhanced_bookmark_model_unittest.cc
diff --git a/components/enhanced_bookmarks/enhanced_bookmark_model_unittest.cc b/components/enhanced_bookmarks/enhanced_bookmark_model_unittest.cc
index 5263297bb6f32af4d292dabfd45308e759a91e5e..edf052e3903165641efe8497f40487260353b2d7 100644
--- a/components/enhanced_bookmarks/enhanced_bookmark_model_unittest.cc
+++ b/components/enhanced_bookmarks/enhanced_bookmark_model_unittest.cc
@@ -18,6 +18,8 @@
 
 namespace {
 
+using enhanced_bookmarks::EnhancedBookmarkModel;
+
 const std::string BOOKMARK_URL("http://example.com/index.html");
 
 class EnhancedBookmarkModelTest : public testing::Test {
@@ -28,11 +30,11 @@ class EnhancedBookmarkModelTest : public testing::Test {
   virtual void SetUp() OVERRIDE {
     bookmarks::TestBookmarkClient bookmark_client;
     bookmark_model_.reset(bookmark_client.CreateModel().release());
-    model_.reset(new enhanced_bookmarks::EnhancedBookmarkModel(
-        bookmark_model_.get(), "v1.0"));
+    model_.reset(new EnhancedBookmarkModel(bookmark_model_.get(), "v1.0"));
   }
 
   virtual void TearDown() OVERRIDE {
+    model_->ShutDown();
     model_.reset();
     bookmark_model_.reset();
   }
@@ -48,26 +50,40 @@ class EnhancedBookmarkModelTest : public testing::Test {
 
   const BookmarkNode* AddBookmark(const std::string& name,
                                   const BookmarkNode* parent) {
-    return bookmark_model_->AddURL(parent,
-                                   0,  // index.
-                                   base::ASCIIToUTF16(name),
-                                   GURL(BOOKMARK_URL));
+    return model_->AddURL(parent,
+                          0,  // index.
+                          base::ASCIIToUTF16(name),
+                          GURL(BOOKMARK_URL),
+                          base::Time::Now());
   }
 
   const BookmarkNode* AddFolder(const std::string& name,
                                 const BookmarkNode* parent) {
-    return bookmark_model_->AddFolder(parent, 0, base::ASCIIToUTF16(name));
+    return model_->AddFolder(parent, 0, base::ASCIIToUTF16(name));
+  }
+
+  std::string GetVersion(const BookmarkNode* node) {
+    return GetMetaInfoField(node, "stars.version");
+  }
+
+  std::string GetId(const BookmarkNode* node) {
+    return GetMetaInfoField(node, "stars.id");
   }
 
-  std::string GetVersionForNode(const BookmarkNode* node) {
-    std::string version;
-    if (!node->GetMetaInfo("stars.version", &version))
+  std::string GetOldId(const BookmarkNode* node) {
+    return GetMetaInfoField(node, "stars.oldId");
+  }
+
+  std::string GetMetaInfoField(const BookmarkNode* node,
+                               const std::string& name) {
+    std::string value;
+    if (!node->GetMetaInfo(name, &value))
       return std::string();
-    return version;
+    return value;
   }
 
   scoped_ptr<BookmarkModel> bookmark_model_;
-  scoped_ptr<enhanced_bookmarks::EnhancedBookmarkModel> model_;
+  scoped_ptr<EnhancedBookmarkModel> model_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(EnhancedBookmarkModelTest);
@@ -253,7 +269,7 @@ TEST_F(EnhancedBookmarkModelTest, TestEncodeDecode) {
   EXPECT_EQ(url, GURL("http://example.com/i.jpg"));
   EXPECT_EQ(width, 22);
   EXPECT_EQ(height, 33);
-  EXPECT_EQ("v1.0", GetVersionForNode(node));
+  EXPECT_EQ("v1.0", GetVersion(node));
 }
 
 TEST_F(EnhancedBookmarkModelTest, TestDoubleEncodeDecode) {
@@ -276,25 +292,26 @@ TEST_F(EnhancedBookmarkModelTest, TestDoubleEncodeDecode) {
   EXPECT_EQ(url, GURL("http://example.com/i.jpg"));
   EXPECT_EQ(width, 33);
   EXPECT_EQ(height, 44);
-  EXPECT_EQ("v1.0", GetVersionForNode(node));
+  EXPECT_EQ("v1.0", GetVersion(node));
 }
 
 TEST_F(EnhancedBookmarkModelTest, TestRemoteId) {
   const BookmarkNode* node = AddBookmark();
-  const BookmarkNode* folder_node = AddFolder();
-
-  std::string remote_id = model_->GetRemoteId(node);
-  // First call creates the UUID, second call should return the same.
-  EXPECT_EQ(remote_id, model_->GetRemoteId(node));
-
   // Verify that the remote id starts with the correct prefix.
-  EXPECT_TRUE(StartsWithASCII(remote_id, "ebc_", true));
-  std::string folder_remote_id = model_->GetRemoteId(folder_node);
-  EXPECT_TRUE(StartsWithASCII(folder_remote_id, "ebf_", true));
-
-  // Verifiy version field was set.
-  EXPECT_EQ("v1.0", GetVersionForNode(node));
-  EXPECT_EQ("v1.0", GetVersionForNode(folder_node));
+  EXPECT_TRUE(StartsWithASCII(model_->GetRemoteId(node), "ebc_", true));
+
+  // Getting the remote id for nodes that don't have them should return the
+  // empty string.
+  const BookmarkNode* existing_node =
+      bookmark_model_->AddURL(bookmark_model_->other_node(),
+                              0,
+                              base::ASCIIToUTF16("Title"),
+                              GURL(GURL(BOOKMARK_URL)));
+  EXPECT_TRUE(model_->GetRemoteId(existing_node).empty());
+
+  // Folder nodes should not have a remote id set on creation.
+  const BookmarkNode* folder_node = AddFolder();
+  EXPECT_TRUE(model_->GetRemoteId(folder_node).empty());
 }
 
 TEST_F(EnhancedBookmarkModelTest, TestEmptyDescription) {
@@ -313,7 +330,7 @@ TEST_F(EnhancedBookmarkModelTest, TestDescription) {
 
   // Check the description is the one that was set.
   EXPECT_EQ(model_->GetDescription(node), description);
-  EXPECT_EQ("v1.0", GetVersionForNode(node));
+  EXPECT_EQ("v1.0", GetVersion(node));
 }
 
 // If there is no notes field, the description should fall back on the snippet.
@@ -349,10 +366,10 @@ TEST_F(EnhancedBookmarkModelTest, TestDescriptionFallback) {
 // EnhancedBookmarkModel makes a change to a node.
 TEST_F(EnhancedBookmarkModelTest, TestVersionField) {
   const BookmarkNode* node = AddBookmark();
-  EXPECT_EQ("", GetVersionForNode(node));
+  EXPECT_EQ("", GetVersion(node));
 
   model_->SetDescription(node, "foo");
-  EXPECT_EQ("v1.0", GetVersionForNode(node));
+  EXPECT_EQ("v1.0", GetVersion(node));
 
   // Add a suffix to the version to set.
   model_->SetVersionSuffix("alpha");
@@ -360,21 +377,97 @@ TEST_F(EnhancedBookmarkModelTest, TestVersionField) {
   model_->SetDescription(node, "foo");
   // Since the description didn't actually change, the version field should
   // not either.
-  EXPECT_EQ("v1.0", GetVersionForNode(node));
+  EXPECT_EQ("v1.0", GetVersion(node));
 
   model_->SetDescription(node, "bar");
-  EXPECT_EQ("v1.0/alpha", GetVersionForNode(node));
+  EXPECT_EQ("v1.0/alpha", GetVersion(node));
 }
 
-// Verifies that the stars.userEdit field is set appropriately when editing a
-// node.
-TEST_F(EnhancedBookmarkModelTest, TestUserEdit) {
-  const BookmarkNode* node = AddBookmark();
+// Verifies that duplicate nodes are reset when the model is created.
+TEST_F(EnhancedBookmarkModelTest, ResetDuplicateNodesOnInitialization) {
+  model_->ShutDown();
+
+  const BookmarkNode* parent = bookmark_model_->other_node();
+  const BookmarkNode* node1 = bookmark_model_->AddURL(
+      parent, 0, base::ASCIIToUTF16("Some title"), GURL(BOOKMARK_URL));
+  const BookmarkNode* node2 = bookmark_model_->AddURL(
+      parent, 0, base::ASCIIToUTF16("Some title"), GURL(BOOKMARK_URL));
+  const BookmarkNode* node3 = bookmark_model_->AddURL(
+      parent, 0, base::ASCIIToUTF16("Some title"), GURL(BOOKMARK_URL));
+  const BookmarkNode* node4 = bookmark_model_->AddURL(
+      parent, 0, base::ASCIIToUTF16("Some title"), GURL(BOOKMARK_URL));
+
+  bookmark_model_->SetNodeMetaInfo(node1, "stars.id", "c_1");
+  bookmark_model_->SetNodeMetaInfo(node2, "stars.id", "c_2");
+  bookmark_model_->SetNodeMetaInfo(node3, "stars.id", "c_1");
+  bookmark_model_->SetNodeMetaInfo(node4, "stars.id", "c_1");
+  EXPECT_EQ("c_1", GetId(node1));
+  EXPECT_EQ("c_2", GetId(node2));
+  EXPECT_EQ("c_1", GetId(node3));
+  EXPECT_EQ("c_1", GetId(node4));
+
+  model_.reset(new EnhancedBookmarkModel(bookmark_model_.get(), "v2.0"));
+
+  EXPECT_EQ("c_2", GetId(node2));
+  EXPECT_EQ("", GetId(node1));
+  EXPECT_EQ("", GetId(node3));
+  EXPECT_EQ("", GetId(node4));
+  EXPECT_EQ("c_1", GetOldId(node1));
+  EXPECT_EQ("c_1", GetOldId(node3));
+  EXPECT_EQ("c_1", GetOldId(node4));
+  EXPECT_EQ("v2.0", GetVersion(node1));
+  EXPECT_EQ("v2.0", GetVersion(node3));
+  EXPECT_EQ("v2.0", GetVersion(node4));
+}
 
-  model_->SetDescription(node, "foo");
-  std::string user_edit;
-  ASSERT_TRUE(node->GetMetaInfo("stars.userEdit", &user_edit));
-  EXPECT_EQ("true", user_edit);
+// Verifies that duplicate nodes are reset if one is created.
+TEST_F(EnhancedBookmarkModelTest, ResetDuplicateAddedNodes) {
+  BookmarkNode::MetaInfoMap meta_info;
+  meta_info["stars.id"] = "c_1";
+  const BookmarkNode* parent = bookmark_model_->other_node();
+
+  const BookmarkNode* node1 =
+      bookmark_model_->AddURLWithCreationTimeAndMetaInfo(
+          parent,
+          0,
+          base::ASCIIToUTF16("Some title"),
+          GURL(BOOKMARK_URL),
+          base::Time::Now(),
+          &meta_info);
+  EXPECT_EQ("c_1", GetId(node1));
+
+  const BookmarkNode* node2 =
+      bookmark_model_->AddURLWithCreationTimeAndMetaInfo(
+          parent,
+          0,
+          base::ASCIIToUTF16("Some title"),
+          GURL(BOOKMARK_URL),
+          base::Time::Now(),
+          &meta_info);
+  EXPECT_EQ("", GetId(node1));
+  EXPECT_EQ("", GetId(node2));
+  EXPECT_EQ("c_1", GetOldId(node1));
+  EXPECT_EQ("c_1", GetOldId(node2));
+  EXPECT_EQ("v1.0", GetVersion(node1));
+  EXPECT_EQ("v1.0", GetVersion(node2));
+}
+
+// Verifies that duplicate nodes are reset if an id is changed to a duplicate
+// value.
+TEST_F(EnhancedBookmarkModelTest, ResetDuplicateChangedNodes) {
+  const BookmarkNode* node1 = AddBookmark();
+  const BookmarkNode* node2 = AddBookmark();
+
+  bookmark_model_->SetNodeMetaInfo(node1, "stars.id", "c_1");
+  EXPECT_EQ("c_1", GetId(node1));
+
+  bookmark_model_->SetNodeMetaInfo(node2, "stars.id", "c_1");
+  EXPECT_EQ("", GetId(node1));
+  EXPECT_EQ("", GetId(node2));
+  EXPECT_EQ("c_1", GetOldId(node1));
+  EXPECT_EQ("c_1", GetOldId(node2));
+  EXPECT_EQ("v1.0", GetVersion(node1));
+  EXPECT_EQ("v1.0", GetVersion(node2));
 }
 

[noyau (Ping after 24h), 2014/09/18 08:43:22]

Can you add tests to make sure the observer methods are all called properly?
Thanks.

[Rune Fevang, 2014/09/19 01:18:07]

Done.

 }  // namespace