Only set remote id during url node creation.

components/enhanced_bookmarks/enhanced_bookmark_model.cc

Index: components/enhanced_bookmarks/enhanced_bookmark_model.cc
diff --git a/components/enhanced_bookmarks/enhanced_bookmark_model.cc b/components/enhanced_bookmarks/enhanced_bookmark_model.cc
index a20e38bac31b494cd8b63e2ff329fd38d645477e..486a2aae09fb4b14e129ae2dd326739bee35e6ea 100644
--- a/components/enhanced_bookmarks/enhanced_bookmark_model.cc
+++ b/components/enhanced_bookmarks/enhanced_bookmark_model.cc
@@ -13,6 +13,7 @@
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/bookmarks/browser/bookmark_node.h"
 #include "components/enhanced_bookmarks/proto/metadata.pb.h"
+#include "ui/base/models/tree_node_iterator.h"
 #include "url/gurl.h"
 
 namespace {
@@ -21,11 +22,10 @@ const char* kBookmarkBarId = "f_bookmarks_bar";
 const char* kIdKey = "stars.id";
 const char* kImageDataKey = "stars.imageData";
 const char* kNoteKey = "stars.note";
+const char* kOldIdKey = "stars.oldId";
 const char* kPageDataKey = "stars.pageData";
-const char* kUserEditKey = "stars.userEdit";
 const char* kVersionKey = "stars.version";
 
-const char* kFolderPrefix = "ebf_";
 const char* kBookmarkPrefix = "ebc_";
 
 // Helper method for working with bookmark metainfo.
@@ -62,13 +62,9 @@ bool PopulateImageData(const image::collections::ImageData_ImageInfo& info,
 
 // Generate a random remote id, with a prefix that depends on whether the node
 // is a folder or a bookmark.
-std::string GenerateRemoteId(bool is_folder) {
+std::string GenerateRemoteId() {
   std::stringstream random_id;
-  // Add prefix depending on whether the node is a folder or not.
-  if (is_folder)
-    random_id << kFolderPrefix;
-  else
-    random_id << kBookmarkPrefix;
+  random_id << kBookmarkPrefix;
 
   // Generate 32 digit hex string random suffix.
   random_id << std::hex << std::setfill('0') << std::setw(16);
@@ -82,16 +78,23 @@ namespace enhanced_bookmarks {
 EnhancedBookmarkModel::EnhancedBookmarkModel(BookmarkModel* bookmark_model,
                                              const std::string& version)
     : bookmark_model_(bookmark_model), version_(version) {
+  bookmark_model_->AddObserver(this);
+  if (bookmark_model_->loaded())
+    InitializeIdMap();
 }
 
 EnhancedBookmarkModel::~EnhancedBookmarkModel() {
 }
 
+void EnhancedBookmarkModel::ShutDown() {
+  bookmark_model_->RemoveObserver(this);
+  bookmark_model_ = NULL;
+}
+
 // Moves |node| to |new_parent| and inserts it at the given |index|.
 void EnhancedBookmarkModel::Move(const BookmarkNode* node,
                                  const BookmarkNode* new_parent,
                                  int index) {
-  // TODO(rfevang): Update meta info placement fields.
   bookmark_model_->Move(node, new_parent, index);
 }
 
@@ -100,12 +103,7 @@ const BookmarkNode* EnhancedBookmarkModel::AddFolder(
     const BookmarkNode* parent,
     int index,
     const base::string16& title) {
-  BookmarkNode::MetaInfoMap meta_info;
-  meta_info[kIdKey] = GenerateRemoteId(true);
-
-  // TODO(rfevang): Set meta info placement fields.
-  return bookmark_model_->AddFolderWithMetaInfo(
-      parent, index, title, &meta_info);
+  return bookmark_model_->AddFolder(parent, index, title);
 }
 
 // Adds a url at the specified position.
@@ -116,9 +114,7 @@ const BookmarkNode* EnhancedBookmarkModel::AddURL(
     const GURL& url,
     const base::Time& creation_time) {
   BookmarkNode::MetaInfoMap meta_info;
-  meta_info[kIdKey] = GenerateRemoteId(false);
-
-  // TODO(rfevang): Set meta info placement fields.
+  meta_info[kIdKey] = GenerateRemoteId();
   return bookmark_model_->AddURLWithCreationTimeAndMetaInfo(
       parent, index, title, url, creation_time, &meta_info);
 }
@@ -127,24 +123,23 @@ std::string EnhancedBookmarkModel::GetRemoteId(const BookmarkNode* node) {
   if (node == bookmark_model_->bookmark_bar_node())
     return kBookmarkBarId;
 
-  // Permanent nodes other than the bookmarks bar don't have ids.
-  DCHECK(!bookmark_model_->is_permanent_node(node));
-
   std::string id;
-  if (!node->GetMetaInfo(kIdKey, &id) || id.empty())
-    return SetRemoteId(node);
+  if (!node->GetMetaInfo(kIdKey, &id))
+    return std::string();
   return id;
 }
 
-std::string EnhancedBookmarkModel::SetRemoteId(const BookmarkNode* node) {
-  std::string remote_id = GenerateRemoteId(node->is_folder());
-  SetMetaInfo(node, kIdKey, remote_id, false);
-  return remote_id;
+const BookmarkNode* EnhancedBookmarkModel::BookmarkForRemoteId(
+    const std::string& remote_id) {
+  IdToNodeMap::iterator it = id_map_.find(remote_id);
+  if (it != id_map_.end())
+    return it->second;
+  return NULL;
 }
 
 void EnhancedBookmarkModel::SetDescription(const BookmarkNode* node,
                                            const std::string& description) {
-  SetMetaInfo(node, kNoteKey, description, true);
+  SetMetaInfo(node, kNoteKey, description);
 }
 
 std::string EnhancedBookmarkModel::GetDescription(const BookmarkNode* node) {
@@ -189,9 +184,7 @@ bool EnhancedBookmarkModel::SetOriginalImage(const BookmarkNode* node,
 
   std::string encoded;
   base::Base64Encode(output, &encoded);
-  SetMetaInfo(node, kImageDataKey, encoded, true);
-  // Ensure that the bookmark has a stars.id, to trigger the server processing.
-  GetRemoteId(node);
+  SetMetaInfo(node, kImageDataKey, encoded);
   return true;
 }
 
@@ -251,10 +244,97 @@ void EnhancedBookmarkModel::SetVersionSuffix(
   version_suffix_ = version_suffix;
 }
 
+void EnhancedBookmarkModel::BookmarkModelChanged() {
+}
+
+void EnhancedBookmarkModel::BookmarkModelLoaded(BookmarkModel* model,
+                                                bool ids_reassigned) {
+  InitializeIdMap();
+}
+
+void EnhancedBookmarkModel::BookmarkNodeAdded(BookmarkModel* model,
+                                              const BookmarkNode* parent,
+                                              int index) {
+  AddToIdMap(parent->GetChild(index));
+  ResetDuplicateRemoteIds();
+}
+
+void EnhancedBookmarkModel::BookmarkNodeRemoved(
+    BookmarkModel* model,
+    const BookmarkNode* parent,
+    int old_index,
+    const BookmarkNode* node,
+    const std::set<GURL>& removed_urls) {
+  std::string remote_id = GetRemoteId(node);
+  id_map_.erase(remote_id);
+}
+
+void EnhancedBookmarkModel::OnWillChangeBookmarkMetaInfo(
+    BookmarkModel* model,
+    const BookmarkNode* node) {
+  prev_remote_id_ = GetRemoteId(node);
+}
+
+void EnhancedBookmarkModel::BookmarkMetaInfoChanged(BookmarkModel* model,
+                                                    const BookmarkNode* node) {
+  std::string remote_id = GetRemoteId(node);
+  if (remote_id != prev_remote_id_) {
+    id_map_.erase(prev_remote_id_);
+    if (!remote_id.empty()) {
+      AddToIdMap(node);
+      ResetDuplicateRemoteIds();
+    }
+  }
+}
+
+void EnhancedBookmarkModel::BookmarkAllUserNodesRemoved(
+    BookmarkModel* model,
+    const std::set<GURL>& removed_urls) {
+  id_map_.clear();
+  // Re-initialize so non-user nodes with remote ids are present in the map.
+  InitializeIdMap();
+}
+
+void EnhancedBookmarkModel::InitializeIdMap() {
+  ui::TreeNodeIterator<const BookmarkNode> iterator(
+      bookmark_model_->root_node());
+  while (iterator.has_next()) {
+    AddToIdMap(iterator.Next());
+  }
+  ResetDuplicateRemoteIds();
+}
+
+void EnhancedBookmarkModel::AddToIdMap(const BookmarkNode* node) {
+  std::string remote_id = GetRemoteId(node);
+  if (remote_id.empty())
+    return;
+
+  // Try to insert the node.
+  std::pair<IdToNodeMap::iterator, bool> result =
+      id_map_.insert(make_pair(remote_id, node));
+  if (!result.second) {
+    // Some node already had the same remote id, so add both nodes to the
+    // to-be-reset set.
+    nodes_to_reset_[result.first->second] = remote_id;
+    nodes_to_reset_[node] = remote_id;

[noyau (Ping after 24h), 2014/09/17 08:24:35]

Don't you need to remove the node you found in the map from the map as well?

[Rune Fevang, 2014/09/17 22:51:04]

It needs to stay there in case there are more duplicates (say 3), so all three
will be found. That's also why the resetting of duplicate ids is deferred until
the loading is completed instead of being done on detection. The nodes will be
removed from the id map when OnMetaInfoChanged is received.

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

Acknowledged.

+  }
+}
+
+void EnhancedBookmarkModel::ResetDuplicateRemoteIds() {
+  for (NodeToIdMap::iterator it = nodes_to_reset_.begin();
+       it != nodes_to_reset_.end();
+       ++it) {
+    BookmarkNode::MetaInfoMap meta_info;
+    meta_info[kIdKey] = "";
+    meta_info[kOldIdKey] = it->second;
+    SetMultipleMetaInfo(it->first, meta_info);
+  }
+  nodes_to_reset_.clear();
+}
+
 void EnhancedBookmarkModel::SetMetaInfo(const BookmarkNode* node,
                                         const std::string& field,
-                                        const std::string& value,
-                                        bool user_edit) {
+                                        const std::string& value) {
   DCHECK(!bookmark_model_->is_permanent_node(node));
 
   BookmarkNode::MetaInfoMap meta_info;
@@ -269,7 +349,6 @@ void EnhancedBookmarkModel::SetMetaInfo(const BookmarkNode* node,
 
   meta_info[field] = value;
   meta_info[kVersionKey] = GetVersionString();
-  meta_info[kUserEditKey] = user_edit ? "true" : "false";
   bookmark_model_->SetNodeMetaInfoMap(node, meta_info);
 }
 
@@ -279,6 +358,37 @@ std::string EnhancedBookmarkModel::GetVersionString() {
   return version_ + '/' + version_suffix_;
 }
 
+void EnhancedBookmarkModel::SetMultipleMetaInfo(
+    const BookmarkNode* node,
+    BookmarkNode::MetaInfoMap meta_info) {
+  DCHECK(!bookmark_model_->is_permanent_node(node));
+
+  // Don't update anything if every value is already set correctly.
+  if (node->GetMetaInfoMap()) {
+    bool changed = false;
+    const BookmarkNode::MetaInfoMap* old_meta_info = node->GetMetaInfoMap();
+    for (BookmarkNode::MetaInfoMap::iterator it = meta_info.begin();
+         it != meta_info.end();
+         ++it) {
+      BookmarkNode::MetaInfoMap::const_iterator old_field =
+          old_meta_info->find(it->first);
+      if (old_field == old_meta_info->end() ||
+          old_field->second == it->second) {
+        changed = true;
+        break;
+      }
+    }
+    if (!changed)
+      return;
+
+    // Fill in the values that aren't changing
+    meta_info.insert(old_meta_info->begin(), old_meta_info->end());
+  }
+
+  meta_info[kVersionKey] = GetVersionString();
+  bookmark_model_->SetNodeMetaInfoMap(node, meta_info);
+}
+
 bool EnhancedBookmarkModel::SetAllImages(const BookmarkNode* node,
                                          const GURL& image_url,
                                          int image_width,