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

chrome/browser/sync/glue/bookmark_model_associator.cc

Index: chrome/browser/sync/glue/bookmark_model_associator.cc
diff --git a/chrome/browser/sync/glue/bookmark_model_associator.cc b/chrome/browser/sync/glue/bookmark_model_associator.cc
index bac5dad899d2a1399628f5bb8660c9ff0401d432..a7d1109ba917b61d4063a126172ca074555bf658 100644
--- a/chrome/browser/sync/glue/bookmark_model_associator.cc
+++ b/chrome/browser/sync/glue/bookmark_model_associator.cc
@@ -87,7 +87,9 @@ class BookmarkNodeFinder {
   // Finds best matching node for the given sync node.
   // Returns the matching node if one exists; NULL otherwise. If a matching
   // node is found, it's removed for further matches.
-  const BookmarkNode* FindBookmarkNode(const syncer::BaseNode& sync_node);
+  const BookmarkNode* FindBookmarkNode(const GURL& url,
+                                       const std::string& title,
+                                       bool is_folder);
 
  private:
   typedef std::multiset<const BookmarkNode*, BookmarkComparer> BookmarkNodesSet;
@@ -123,11 +125,11 @@ BookmarkNodeFinder::BookmarkNodeFinder(const BookmarkNode* parent_node)
 }
 
 const BookmarkNode* BookmarkNodeFinder::FindBookmarkNode(
-    const syncer::BaseNode& sync_node) {
-  // Create a bookmark node from the given sync node.
-  BookmarkNode temp_node(GURL(sync_node.GetBookmarkSpecifics().url()));
-  temp_node.SetTitle(UTF8ToUTF16(sync_node.GetTitle()));
-  if (sync_node.GetIsFolder())
+    const GURL& url, const std::string& title, bool is_folder) {
+  // Create a bookmark node from the given bookmark attributes.
+  BookmarkNode temp_node(url);
+  temp_node.SetTitle(UTF8ToUTF16(title));
+  if (is_folder)
     temp_node.set_type(BookmarkNode::FOLDER);
   else
     temp_node.set_type(BookmarkNode::URL);
@@ -438,6 +440,9 @@ syncer::SyncError BookmarkModelAssociator::BuildAssociations(
     dfs_stack.push(mobile_bookmarks_sync_id);
 
   syncer::WriteTransaction trans(FROM_HERE, user_share_);
+
+  ApplyDeletesFromSyncJournal(&trans);
+
   syncer::ReadNode bm_root(&trans);
   if (bm_root.InitByTagLookup(syncer::ModelTypeToRootTag(syncer::BOOKMARKS)) ==
       syncer::BaseNode::INIT_OK) {
@@ -480,7 +485,10 @@ syncer::SyncError BookmarkModelAssociator::BuildAssociations(
       }
 
       const BookmarkNode* child_node = NULL;
-      child_node = node_finder.FindBookmarkNode(sync_child_node);
+      child_node = node_finder.FindBookmarkNode(
+          GURL(sync_child_node.GetBookmarkSpecifics().url()),
+          sync_child_node.GetTitle(),
+          sync_child_node.GetIsFolder());
       if (child_node)
         Associate(child_node, sync_child_id);
       // All bookmarks are currently modified at association time (even if
@@ -536,6 +544,84 @@ syncer::SyncError BookmarkModelAssociator::BuildAssociations(
   return syncer::SyncError();
 }
 
+struct FolderInfo {
+  FolderInfo(const BookmarkNode* f, const BookmarkNode* p, int64 id)
+      : folder(f), parent(p), sync_id(id) {}
+  const BookmarkNode* folder;
+  const BookmarkNode* parent;
+  int64 sync_id;
+};
+typedef std::vector<FolderInfo> FolderInfoList;
+
+int64 BookmarkModelAssociator::ApplyDeletesFromSyncJournal(
+    syncer::BaseTransaction* trans) {
+  int64 num_deleted = 0;
+
+  syncer::SyncDataList deleted_sync_bookmarks;
+  trans->GetDeletedSyncData(syncer::BOOKMARKS, &deleted_sync_bookmarks);

[tim (not reviewing), 2012/12/11 19:58:33]

How can we make it so that this data is *sent* to the glue layer
(ModelAssociators, SyncableService) by the sync engine only when needed? Versus
having to add an instruction to Building a Sync Data type of the form "Don't
forget to look up the delete journal"..  It would be better if this information
was part of / embedded in GetAllSyncData (for example), or even sent via a
separate callback (though I prefer the former). 

+  if (deleted_sync_bookmarks.empty())
+    return 0;
+
+  std::stack<const BookmarkNode*> dfs_stack;
+  dfs_stack.push(bookmark_model_->bookmark_bar_node());
+  dfs_stack.push(bookmark_model_->other_node());
+  if (expect_mobile_bookmarks_folder_)
+    dfs_stack.push(bookmark_model_->mobile_node());
+
+  FolderInfoList folders_deleted;
+  while (!dfs_stack.empty()) {
+    const BookmarkNode* parent = dfs_stack.top();
+    dfs_stack.pop();
+
+    BookmarkNodeFinder finder(parent);
+    syncer::SyncDataList::iterator it = deleted_sync_bookmarks.begin();
+    while (it != deleted_sync_bookmarks.end()) {
+      const BookmarkNode* child = finder.FindBookmarkNode(
+          GURL(it->GetSpecifics().bookmark().url()), it->GetTitle(),
+          it->IsFolder());
+      if (child) {
+        if (child->is_folder()) {
+          // Save folders first and delete only empty ones later.
+          folders_deleted.push_back(FolderInfo(child, parent,
+                                               it->GetRemoteId()));
+        } else {
+          bookmark_model_->Remove(parent, child->GetIndexOf(parent));
+          ++num_deleted;
+        }
+        deleted_sync_bookmarks.erase(it++);
+      } else {
+        ++it;
+      }
+    }
+
+    if (deleted_sync_bookmarks.empty())
+      break;
+  }
+
+  // Ids of sync nodes not found in bookmark model, i.e. deletion is persisted.
+  std::set<int64> confirmed_sync_deletes;
+
+  // Remove empty folders from bottom to top.
+  for (FolderInfoList::const_reverse_iterator it = folders_deleted.rbegin();
+      it != folders_deleted.rend(); ++it) {
+    if (it->folder->child_count() == 0) {
+      bookmark_model_->Remove(it->parent, it->folder->GetIndexOf(it->parent));
+      ++num_deleted;
+    } else {
+      // Keep non-empty folder and remove its journal so that it won't match
+      // again in the future.
+      confirmed_sync_deletes.insert(it->sync_id);
+    }
+  }
+
+  // Consider deletion of unmatched sync nodes to be confirmed.
+  for (size_t i = 0; i < deleted_sync_bookmarks.size(); ++i)
+    confirmed_sync_deletes.insert(deleted_sync_bookmarks[i].GetRemoteId());
+  trans->PurgeDeletedSyncData(confirmed_sync_deletes);
+
+  return num_deleted;
+}
+
 void BookmarkModelAssociator::PostPersistAssociationsTask() {
   // No need to post a task if a task is already pending.
   if (weak_factory_.HasWeakPtrs())