Fix 26141

- Change sync id to bookmark id map to sync id to bookmark node map
- Make other appropriate changes.

BUG=26141
TEST=Exist.

Review URL: http://codereview.chromium.org/383002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@31597 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 30 insertions, 47 deletions

diff --git a/chrome/browser/sync/glue/change_processor.cc b/chrome/browser/sync/glue/change_processor.cc
index 89ec4306..bd3847a 100644
--- a/chrome/browser/sync/glue/change_processor.cc
+++ b/chrome/browser/sync/glue/change_processor.cc
@@ -74,11 +74,10 @@ void ChangeProcessor::RemoveOneSyncNode(sync_api::WriteTransaction* trans,
   // This node should have no children.
   DCHECK(sync_node.GetFirstChildId() == sync_api::kInvalidId);
   // Remove association and delete the sync node.
-  model_associator_->DisassociateIds(sync_node.GetId());
+  model_associator_->Disassociate(sync_node.GetId());
   sync_node.Remove();
 }
 
-
 void ChangeProcessor::RemoveSyncNodeHierarchy(const BookmarkNode* topmost) {
   sync_api::WriteTransaction trans(share_handle_);
 
@@ -156,7 +155,7 @@ int64 ChangeProcessor::CreateSyncNode(const BookmarkNode* parent,
 
   // Associate the ID from the sync domain with the bookmark node, so that we
   // can refer back to this item later.
-  associator->AssociateIds(child->id(), sync_child.GetId());
+  associator->Associate(child, sync_child.GetId());
 
   return sync_child.GetId();
 }
@@ -396,7 +395,7 @@ void ChangeProcessor::ApplyChangesFromSyncModel(
       DCHECK_EQ(dst->GetChildCount(), 0) << "Node being deleted has children";
       model->Remove(parent, parent->IndexOfChild(dst));
       dst = NULL;
-      model_associator_->DisassociateIds(changes[i].id);
+      model_associator_->Disassociate(changes[i].id);
     } else {
       DCHECK_EQ((changes[i].action ==
           sync_api::SyncManager::ChangeRecord::ACTION_ADD), (dst == NULL))
@@ -443,7 +442,7 @@ const BookmarkNode* ChangeProcessor::CreateOrUpdateBookmarkNode(
       src->GetId());
   if (!dst) {
     dst = CreateBookmarkNode(src, parent, model, index);
-    model_associator_->AssociateIds(dst->id(), src->GetId());
+    model_associator_->Associate(dst, src->GetId());
   } else {
     // URL and is_folder are not expected to change.
     // TODO(ncarter): Determine if such changes should be legal or not.
@@ -462,8 +461,8 @@ const BookmarkNode* ChangeProcessor::CreateOrUpdateBookmarkNode(
         src->GetIsFolder() ? GURL() : src->GetURL(),
         NULL);  // NULL because we don't need a BookmarkEditor::Handler.
     if (dst != old_dst) {  // dst was replaced with a new node with new URL.
-      model_associator_->DisassociateIds(src->GetId());
-      model_associator_->AssociateIds(dst->id(), src->GetId());
+      model_associator_->Disassociate(src->GetId());
+      model_associator_->Associate(dst, src->GetId());
     }
     SetBookmarkFavicon(src, dst, model->profile());
   }
diff --git a/chrome/browser/sync/glue/model_associator.cc b/chrome/browser/sync/glue/model_associator.cc
index a936d552..1754504 100644
--- a/chrome/browser/sync/glue/model_associator.cc
+++ b/chrome/browser/sync/glue/model_associator.cc
@@ -167,13 +167,9 @@ int64 ModelAssociator::GetSyncIdFromBookmarkId(int64 node_id) const {
   return iter == id_map_.end() ? sync_api::kInvalidId : iter->second;
 }
 
-bool ModelAssociator::GetBookmarkIdFromSyncId(int64 sync_id,
-                                              int64* node_id) const {
-  SyncIdToBookmarkIdMap::const_iterator iter = id_map_inverse_.find(sync_id);
-  if (iter == id_map_inverse_.end())
-    return false;
-  *node_id = iter->second;
-  return true;
+const BookmarkNode* ModelAssociator::GetBookmarkNodeFromSyncId(int64 sync_id) {
+  SyncIdToBookmarkNodeMap::const_iterator iter = id_map_inverse_.find(sync_id);
+  return iter == id_map_inverse_.end() ? NULL : iter->second;
 }
 
 bool ModelAssociator::InitSyncNodeFromBookmarkId(
@@ -189,30 +185,22 @@ bool ModelAssociator::InitSyncNodeFromBookmarkId(
   return true;
 }
 
-const BookmarkNode* ModelAssociator::GetBookmarkNodeFromSyncId(int64 sync_id) {
-  int64 node_id;
-  if (!GetBookmarkIdFromSyncId(sync_id, &node_id))
-    return false;
-  BookmarkModel* model = sync_service_->profile()->GetBookmarkModel();
-  // TODO(munjal): Fix issue 26141.
-  return model->GetNodeByID(node_id);
-}
-
-void ModelAssociator::AssociateIds(int64 node_id, int64 sync_id) {
+void ModelAssociator::Associate(const BookmarkNode* node, int64 sync_id) {
+  int64 node_id = node->id();
   DCHECK_NE(sync_id, sync_api::kInvalidId);
   DCHECK(id_map_.find(node_id) == id_map_.end());
   DCHECK(id_map_inverse_.find(sync_id) == id_map_inverse_.end());
   id_map_[node_id] = sync_id;
-  id_map_inverse_[sync_id] = node_id;
+  id_map_inverse_[sync_id] = node;
   dirty_associations_sync_ids_.insert(sync_id);
   PostPersistAssociationsTask();
 }
 
-void ModelAssociator::DisassociateIds(int64 sync_id) {
-  SyncIdToBookmarkIdMap::iterator iter = id_map_inverse_.find(sync_id);
+void ModelAssociator::Disassociate(int64 sync_id) {
+  SyncIdToBookmarkNodeMap::iterator iter = id_map_inverse_.find(sync_id);
   if (iter == id_map_inverse_.end())
     return;
-  id_map_.erase(iter->second);
+  id_map_.erase(iter->second->id());
   id_map_inverse_.erase(iter);
   dirty_associations_sync_ids_.erase(sync_id);
 }
@@ -282,7 +270,7 @@ bool ModelAssociator::AssociateTaggedPermanentNode(
   if (!GetSyncIdForTaggedNode(tag, &sync_id))
     return false;
 
-  AssociateIds(permanent_node->id(), sync_id);
+  Associate(permanent_node, sync_id);
   return true;
 }
 
@@ -401,7 +389,7 @@ bool ModelAssociator::BuildAssociations() {
         child_node = ChangeProcessor::CreateBookmarkNode(&sync_child_node,
             parent_node, model, index);
       }
-      AssociateIds(child_node->id(), sync_child_id);
+      Associate(child_node, sync_child_id);
       if (sync_child_node.GetIsFolder())
         dfs_stack.push(sync_child_id);
 
@@ -459,9 +447,9 @@ void ModelAssociator::PersistAssociations() {
       sync_service_->OnUnrecoverableError();
       return;
     }
-    int64 node_id;
-    if (GetBookmarkIdFromSyncId(sync_id, &node_id))
-      sync_node.SetExternalId(node_id);
+    const BookmarkNode* node = GetBookmarkNodeFromSyncId(sync_id);
+    if (node)
+      sync_node.SetExternalId(node->id());
     else
       NOTREACHED();
   }
@@ -534,7 +522,7 @@ bool ModelAssociator::LoadAssociations() {
         !NodesMatch(node, &sync_parent))
       return false;
 
-    AssociateIds(external_id, sync_parent.GetId());
+    Associate(node, sync_parent.GetId());
 
     // Add all children of the current node to the stack.
     int64 child_id = sync_parent.GetFirstChildId();
diff --git a/chrome/browser/sync/glue/model_associator.h b/chrome/browser/sync/glue/model_associator.h
index 214db23..e7d5c32 100644
--- a/chrome/browser/sync/glue/model_associator.h
+++ b/chrome/browser/sync/glue/model_associator.h
@@ -44,23 +44,19 @@ class ModelAssociator
   // bookmark node id.
   int64 GetSyncIdFromBookmarkId(int64 node_id) const;
 
-  // Stores bookmark node id for the given sync id in bookmark_id. Returns true
-  // if the bookmark id was successfully found; false otherwise.
-  bool GetBookmarkIdFromSyncId(int64 sync_id, int64* bookmark_id) const;
+  // Returns the bookmark node for the given sync id.
+  // Returns NULL if no bookmark node is found for the given sync id.
+  const BookmarkNode* GetBookmarkNodeFromSyncId(int64 sync_id);
 
   // Initializes the given sync node from the given bookmark node id.
   // Returns false if no sync node was found for the given bookmark node id or
   // if the initialization of sync node fails.
   bool InitSyncNodeFromBookmarkId(int64 node_id, sync_api::BaseNode* sync_node);
 
-  // Returns the bookmark node for the given sync id.
-  // Returns NULL if no bookmark node is found for the given sync id.
-  const BookmarkNode* GetBookmarkNodeFromSyncId(int64 sync_id);
-
-  // Associates the given bookmark node id with the given sync id.
-  void AssociateIds(int64 node_id, int64 sync_id);
-  // Disassociate the ids that correspond to the given sync id.
-  void DisassociateIds(int64 sync_id);
+  // Associates the given bookmark node with the given sync id.
+  void Associate(const BookmarkNode* node, int64 sync_id);
+  // Remove the association that corresponds to the given sync id.
+  void Disassociate(int64 sync_id);
 
   // Returns whether the bookmark model has user created nodes or not. That is,
   // whether there are nodes in the bookmark model except the bookmark bar and
@@ -94,7 +90,7 @@ class ModelAssociator
 
  private:
   typedef std::map<int64, int64> BookmarkIdToSyncIdMap;
-  typedef std::map<int64, int64> SyncIdToBookmarkIdMap;
+  typedef std::map<int64, const BookmarkNode*> SyncIdToBookmarkNodeMap;
   typedef std::set<int64> DirtyAssociationsSyncIds;
 
   // Posts a task to persist dirty associations.
@@ -125,7 +121,7 @@ class ModelAssociator
 
   ProfileSyncService* sync_service_;
   BookmarkIdToSyncIdMap id_map_;
-  SyncIdToBookmarkIdMap id_map_inverse_;
+  SyncIdToBookmarkNodeMap id_map_inverse_;
   // Stores sync ids for dirty associations.
   DirtyAssociationsSyncIds dirty_associations_sync_ids_;