Only set remote id during url node creation.

components/enhanced_bookmarks/enhanced_bookmark_model.cc

 // Copyright 2014 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.
 
 #include "components/enhanced_bookmarks/enhanced_bookmark_model.h"
 
 #include <iomanip>
 #include <sstream>
 
 #include "base/base64.h"
 #include "base/logging.h"
 #include "base/rand_util.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/bookmarks/browser/bookmark_node.h"
+#include "components/enhanced_bookmarks/enhanced_bookmark_model_observer.h"
 #include "components/enhanced_bookmarks/proto/metadata.pb.h"
+#include "ui/base/models/tree_node_iterator.h"
 #include "url/gurl.h"
 
 namespace {
 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.
 std::string DataForMetaInfoField(const BookmarkNode* node,
                                  const std::string& field) {
   std::string value;
   if (!node->GetMetaInfo(field, &value))
     return std::string();
 
   std::string decoded;
@@ skipping 16 matching lines @@
     return false;
 
   *out_url = url;
   *width = info.width();
   *height = info.height();
   return true;
 }
 
 // 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);
   random_id << base::RandUint64() << base::RandUint64();
   return random_id.str();
 }
 }  // namespace
 
 namespace enhanced_bookmarks {
 
 EnhancedBookmarkModel::EnhancedBookmarkModel(BookmarkModel* bookmark_model,
                                              const std::string& version)
-    : bookmark_model_(bookmark_model), version_(version) {
+    : bookmark_model_(bookmark_model), loaded_(false), version_(version) {
+  bookmark_model_->AddObserver(this);
+  if (bookmark_model_->loaded()) {
+    InitializeIdMap();
+    loaded_ = true;
+  }
 }
 
 EnhancedBookmarkModel::~EnhancedBookmarkModel() {
 }
 
+void EnhancedBookmarkModel::ShutDown() {
+  FOR_EACH_OBSERVER(EnhancedBookmarkModelObserver,
+                    observers_,
+                    EnhancedBookmarkModelShuttingDown());
+  bookmark_model_->RemoveObserver(this);
+  bookmark_model_ = NULL;
+}
+
+void EnhancedBookmarkModel::AddObserver(
+    EnhancedBookmarkModelObserver* observer) {
+  observers_.AddObserver(observer);
+}
+
+void EnhancedBookmarkModel::RemoveObserver(
+    EnhancedBookmarkModelObserver* observer) {
+  observers_.RemoveObserver(observer);
+}
+
 // 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);
 }
 
 // Adds a new folder node at the specified position.
 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.
 const BookmarkNode* EnhancedBookmarkModel::AddURL(
     const BookmarkNode* parent,
     int index,
     const base::string16& title,
     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);
 }
 
 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) {
   // First, look for a custom note set by the user.
   std::string description;
   if (node->GetMetaInfo(kNoteKey, &description) && !description.empty())
     return description;
 
   // If none are present, return the snippet.
   return GetSnippet(node);
@@ skipping 24 matching lines @@
   info->set_height(height);
   data.set_allocated_original_info(info.release());
 
   std::string output;
   bool result = data.SerializePartialToString(&output);
   if (!result)
     return false;
 
   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;
 }
 
 bool EnhancedBookmarkModel::GetOriginalImage(const BookmarkNode* node,
                                              GURL* url,
                                              int* width,
                                              int* height) {
   std::string decoded(DataForMetaInfoField(node, kImageDataKey));
   if (decoded == "")
     return false;
@@ skipping 39 matching lines @@
     return std::string();
 
   return data.snippet();
 }
 
 void EnhancedBookmarkModel::SetVersionSuffix(
     const std::string& version_suffix) {
   version_suffix_ = version_suffix;
 }
 
+void EnhancedBookmarkModel::BookmarkModelChanged() {
+}
+
+void EnhancedBookmarkModel::BookmarkModelLoaded(BookmarkModel* model,
+                                                bool ids_reassigned) {
+  InitializeIdMap();
+  FOR_EACH_OBSERVER(
+      EnhancedBookmarkModelObserver, observers_, EnhancedBookmarkModelLoaded());
+}
+
+void EnhancedBookmarkModel::BookmarkNodeAdded(BookmarkModel* model,
+                                              const BookmarkNode* parent,
+                                              int index) {
+  const BookmarkNode* node = parent->GetChild(index);
+  AddToIdMap(node);
+  ResetDuplicateRemoteIds();
+  FOR_EACH_OBSERVER(
+      EnhancedBookmarkModelObserver, observers_, EnhancedBookmarkAdded(node));
+}
+
+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);
+  FOR_EACH_OBSERVER(
+      EnhancedBookmarkModelObserver, observers_, EnhancedBookmarkRemoved(node));
+}
+
+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();
+    }
+
+    // The remote id could have been changed again by the de-duping process, so
+    // get the current state of the id before notifying observers.
+    std::string new_remote_id = GetRemoteId(node);
+    if (prev_remote_id_ != new_remote_id) {
+      FOR_EACH_OBSERVER(EnhancedBookmarkModelObserver,
+                        observers_,
+                        EnhancedBookmarkRemoteIdChanged(
+                            node, prev_remote_id_, new_remote_id));
+    }
+  }
+}
+
+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();
+  FOR_EACH_OBSERVER(EnhancedBookmarkModelObserver,
+                    observers_,
+                    EnhancedBookmarkAllUserNodesRemoved());
+}
+
+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;
+  }
+}
+
+void EnhancedBookmarkModel::ResetDuplicateRemoteIds() {
+  for (NodeToIdMap::iterator it = nodes_to_reset_.begin();

[Mark, 2014/09/18 18:18:27]

Just checking, are you able to get some sort of lock on Chrome Sync from the
binary code?  That would be great to ensure these deduping actions are not
getting split across request boundaries.

[Rune Fevang, 2014/09/19 01:18:07]

There's no sync API support for doing that, the bookmark change processor simply
listens to the bookmark model and updates/creates sync nodes as they come in.

+       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;
   const BookmarkNode::MetaInfoMap* old_meta_info = node->GetMetaInfoMap();
   if (old_meta_info)
     meta_info.insert(old_meta_info->begin(), old_meta_info->end());
 
   // Don't update anything if the value to set is already there.
   BookmarkNode::MetaInfoMap::iterator it = meta_info.find(field);
   if (it != meta_info.end() && it->second == value)
     return;
 
   meta_info[field] = value;
   meta_info[kVersionKey] = GetVersionString();
-  meta_info[kUserEditKey] = user_edit ? "true" : "false";
   bookmark_model_->SetNodeMetaInfoMap(node, meta_info);
 }
 
 std::string EnhancedBookmarkModel::GetVersionString() {
   if (version_suffix_.empty())
     return version_;
   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) {

[Mark, 2014/09/18 18:18:27]

So is all this effort really necessary?  What happens if you touch every item
again?  Why not directly replace the meta info values?

[Rune Fevang, 2014/09/19 01:18:07]

Necessary? Not really, it's more of a safeguard against setting the version
field if we didn't actually change anything, I believe we ran into that problem
on the JS client right?

Thanks for drawing my eye to this function, noticed a bug (and added a test for
it) ;)

+        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,
                                          int image_height,
                                          const GURL& thumbnail_url,
                                          int thumbnail_width,
                                          int thumbnail_height) {
   DCHECK(node->is_url());
   DCHECK(image_url.is_valid() || image_url.is_empty());
   DCHECK(thumbnail_url.is_valid() || thumbnail_url.is_empty());
@@ skipping 40 matching lines @@
   if (!result)
     return false;
 
   std::string encoded;
   base::Base64Encode(output, &encoded);
   bookmark_model_->SetNodeMetaInfo(node, kImageDataKey, encoded);
   return true;
 }
 
 }  // namespace enhanced_bookmarks