Use delete journal to remove bookmarks that are already deleted in sync model

chrome/browser/sync/profile_sync_service_bookmark_unittest.cc

Index: chrome/browser/sync/profile_sync_service_bookmark_unittest.cc
diff --git a/chrome/browser/sync/profile_sync_service_bookmark_unittest.cc b/chrome/browser/sync/profile_sync_service_bookmark_unittest.cc
index 618b4ee5afcfb222883ee1405737b015eb64b9e3..f98ab160399d39b88c1c9b4e81551fb085fabe45 100644
--- a/chrome/browser/sync/profile_sync_service_bookmark_unittest.cc
+++ b/chrome/browser/sync/profile_sync_service_bookmark_unittest.cc
@@ -127,6 +127,8 @@ class FakeServerChange {
       syncer::WriteNode node(trans_);
       EXPECT_EQ(BaseNode::INIT_OK, node.InitByIdLookup(id));
       EXPECT_FALSE(node.GetFirstChildId());
+      node.GetMutableEntryForTest()->Put(syncer::syncable::SERVER_IS_DEL,
+                                         true);
       node.Remove();
     }
     {
@@ -422,8 +424,10 @@ class ProfileSyncServiceBookmarkTest : public testing::Test {
 
   void StopSync() {
     change_processor_.reset();
-    syncer::SyncError error = model_associator_->DisassociateModels();
-    EXPECT_FALSE(error.IsSet());
+    if (model_associator_.get()) {
+      syncer::SyncError error = model_associator_->DisassociateModels();
+      EXPECT_FALSE(error.IsSet());
+    }
     model_associator_.reset();
 
     message_loop_.RunUntilIdle();
@@ -1009,6 +1013,83 @@ TEST_F(ProfileSyncServiceBookmarkTest, MergeDuplicates) {
   ExpectModelMatch();
 }
 
+TEST_F(ProfileSyncServiceBookmarkTest, ApplySyncDeletesFromJournal) {
+  // Initialize sync model and bookmark model as:
+  // Folder 1
+  //   |-- URL 1
+  //   +-- Folder 2
+  //         +-- URL 2
+  LoadBookmarkModel(DELETE_EXISTING_STORAGE, SAVE_TO_STORAGE);
+  int64 f1 = 0;
+  int64 u1 = 0;
+  int64 f2 = 0;
+  int64 u2 = 0;
+  StartSync();
+  int initial_sync_bk_count = GetSyncBookmarkCount();
+  {
+    syncer::WriteTransaction trans(FROM_HERE, test_user_share_.user_share());
+    FakeServerChange adds(&trans);
+    f1 = adds.AddFolder(L"Folder 1", bookmark_bar_id(), 0);
+    u1 = adds.AddURL(L"URL 1", "http://www.google.com/", f1, 0);
+    f2 = adds.AddFolder(L"Folder 2", f1, u1);
+    u2 = adds.AddURL(L"URL 2", "http://mail.google.com/", f2, 0);
+    adds.ApplyPendingChanges(change_processor_.get());
+  }
+  StopSync();
+
+  // Reload bookmark model and disable model saving to make sync changes not
+  // persisted.
+  LoadBookmarkModel(LOAD_FROM_STORAGE, DONT_SAVE_TO_STORAGE);

[tim (not reviewing), 2013/01/14 23:32:48]

cool! that part was easy :)

[haitaol1, 2013/01/15 19:44:31]

Done.

+  EXPECT_EQ(5, model_->bookmark_bar_node()->GetTotalNodeCount());
+  ASSERT_TRUE(test_user_share_.Reload());
+  EXPECT_EQ(initial_sync_bk_count + 4, GetSyncBookmarkCount());
+  StartSync();
+  {
+    // Remove all folders/bookmarks added above.

[tim (not reviewing), 2013/01/14 23:32:48]

Maybe leave one behind to make it a bit more interesting?

[haitaol1, 2013/01/15 19:44:31]

Added another bookmark under bookmark bar that's not deleted. 

On 2013/01/14 23:32:48, timsteele wrote:

+    syncer::WriteTransaction trans(FROM_HERE, test_user_share_.user_share());
+    FakeServerChange dels(&trans);
+    dels.Delete(u2);
+    dels.Delete(f2);
+    dels.Delete(u1);
+    dels.Delete(f1);
+    dels.ApplyPendingChanges(change_processor_.get());
+  }
+  MessageLoop::current()->RunUntilIdle();

[tim (not reviewing), 2013/01/14 23:32:48]

Comment this line.

[haitaol1, 2013/01/15 19:44:31]

Turns out not needed.

+  StopSync();
+  // Bookmarks in in-memory bookmark model are removed.
+  EXPECT_EQ(1, model_->bookmark_bar_node()->GetTotalNodeCount());
+
+  // Reload dead bookmarks from storage.
+  LoadBookmarkModel(LOAD_FROM_STORAGE, DONT_SAVE_TO_STORAGE);
+  EXPECT_EQ(5, model_->bookmark_bar_node()->GetTotalNodeCount());
+  // Add a local bookmark unaware to sync.
+  model_->AddURL(model_->bookmark_bar_node()->GetChild(0),
+                 0, UTF8ToUTF16("local"), GURL("http://www.youtube.com"));
+  ASSERT_TRUE(test_user_share_.Reload());

[tim (not reviewing), 2013/01/14 23:32:48]

I'm not sure what this line and the one above are trying to achieve?

[haitaol1, 2013/01/15 19:44:31]

To test that non-empty folder that matches delete journal is not deleted.

On 2013/01/14 23:32:48, timsteele wrote:

+  EXPECT_EQ(initial_sync_bk_count, GetSyncBookmarkCount());
+  StartSync();
+  // u2, f2, u1 are removed by delete journal. f1 remains because it's not

[tim (not reviewing), 2013/01/14 23:32:48]

Why is it non-empty?  Because AddURL chooses it behind-the-scenes to add your
youtube.com URL above?  (Might add comment if that's the case).

[haitaol1, 2013/01/15 19:44:31]

Done.

+  // empty.
+  EXPECT_EQ(3, model_->bookmark_bar_node()->GetTotalNodeCount());
+  EXPECT_EQ(UTF8ToUTF16("Folder 1"),
+            model_->bookmark_bar_node()->GetChild(0)->GetTitle());
+  EXPECT_EQ(UTF8ToUTF16("local"),
+            model_->bookmark_bar_node()->GetChild(0)->GetChild(0)->GetTitle());
+  StopSync();
+
+  // Verify purging of delete journals.
+  ASSERT_TRUE(test_user_share_.Reload());
+  // Delete journals for u2, f2, u1 remains because they are used in last
+  // association.
+  EXPECT_EQ(3u, test_user_share_.GetDeleteJournalSize());
+  StartSync();
+  StopSync();
+  // Reload again and all delete journals should be gone because none is used
+  // in last association.
+  ASSERT_TRUE(test_user_share_.Reload());
+  EXPECT_EQ(0u, test_user_share_.GetDeleteJournalSize());
+}
+
 struct TestData {
   const wchar_t* title;
   const char* url;