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

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 109 matching lines @@
     return Add(title, url, false, parent_id, predecessor_id);
   }
 
   // Pretend that the server told the syncer to delete an object.
   void Delete(int64 id) {
     {
       // Delete the sync node.
       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();
     }
     {
       // Verify the deletion.
       syncer::ReadNode node(trans_);
       EXPECT_EQ(BaseNode::INIT_FAILED_ENTRY_IS_DEL, node.InitByIdLookup(id));
     }
 
     syncer::ChangeRecord record;
     record.action = syncer::ChangeRecord::ACTION_DELETE;
@@ skipping 275 matching lines @@
 
     // Set up change processor.
     change_processor_.reset(
         new BookmarkChangeProcessor(model_associator_.get(),
                                     &mock_error_handler_));
     change_processor_->Start(&profile_, test_user_share_.user_share());
   }
 
   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();
 
     // TODO(akalin): Actually close the database and flush it to disk
     // (and make StartSync reload from disk).  This would require
     // refactoring TestUserShare.
   }
 
   void UnloadBookmarkModel() {
@@ skipping 565 matching lines @@
   EXPECT_EQ(2, model_->other_node()->child_count());
 
   // Restart the sync service to trigger model association.
   StopSync();
   StartSync();
 
   EXPECT_EQ(2, model_->other_node()->child_count());
   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;
 };
 
 // Map from bookmark node ID to its version.
 typedef std::map<int64, int64> BookmarkNodeVersionMap;
 
 // TODO(ncarter): Integrate the existing TestNode/PopulateNodeFromString code
 // in the bookmark model unittest, to make it simpler to set up test data
@@ skipping 724 matching lines @@
             new_versions[changed_bookmark->id()]);
   initial_versions.erase(changed_bookmark->id());
   ExpectTransactionVersionMatch(model_->bookmark_bar_node(), initial_versions);
   ExpectTransactionVersionMatch(model_->other_node(), initial_versions);
   ExpectTransactionVersionMatch(model_->mobile_node(), initial_versions);
 }
 
 }  // namespace
 
 }  // namespace browser_sync