Set version field when changes are made to enhanced bookmarks.

components/enhanced_bookmarks/enhanced_bookmark_model.cc

Index: components/enhanced_bookmarks/enhanced_bookmark_model.cc
diff --git a/components/enhanced_bookmarks/metadata_accessor.cc b/components/enhanced_bookmarks/enhanced_bookmark_model.cc
similarity index 45%
rename from components/enhanced_bookmarks/metadata_accessor.cc
rename to components/enhanced_bookmarks/enhanced_bookmark_model.cc
index cfcc9be5efb2bd2c59f596e159e7e63225e7194b..8981bb6ecdc46874f6850bd9533ae445924ca45b 100644
--- a/components/enhanced_bookmarks/metadata_accessor.cc
+++ b/components/enhanced_bookmarks/enhanced_bookmark_model.cc
@@ -2,54 +2,48 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "components/enhanced_bookmarks/metadata_accessor.h"
+#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/proto/metadata.pb.h"
-#include "ui/base/models/tree_node_iterator.h"
-
-using namespace image::collections;
+#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* 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) {
-  const BookmarkNode::MetaInfoMap* map = node->GetMetaInfoMap();
-  if (!map)
-    return "";
-
-  BookmarkNode::MetaInfoMap::const_iterator it = map->find(field);
-  if (it == map->end())
-    return "";
+  std::string value;
+  if (!node->GetMetaInfo(field, &value))
+    return std::string();
 
   std::string decoded;
-  bool result = base::Base64Decode((*it).second, &decoded);
-  if (!result)
-    return "";
+  if (!base::Base64Decode(value, &decoded))
+    return std::string();
 
   return decoded;
 }
 
-// Sets a new remote id on a bookmark.
-std::string SetRemoteIdOnBookmark(BookmarkModel* bookmark_model,
-                                  const BookmarkNode* node) {
-  // Generate 16 digit hex string random id.
-  std::stringstream random_id;
-  random_id << std::hex << std::setfill('0') << std::setw(16);
-  random_id << base::RandUint64() << base::RandUint64();
-  std::string random_id_str = random_id.str();
-  bookmark_model->SetNodeMetaInfo(
-      node, enhanced_bookmarks::kIdDataKey, random_id_str);
-  return random_id_str;
-}
-
 // Helper method for working with ImageData_ImageInfo.
-bool PopulateImageData(const ImageData_ImageInfo& info,
+bool PopulateImageData(const image::collections::ImageData_ImageInfo& info,
                        GURL* out_url,
                        int* width,
                        int* height) {
@@ -65,59 +59,109 @@ bool PopulateImageData(const ImageData_ImageInfo& info,
   *height = info.height();
   return true;
 }
-
 }  // namespace
 
 namespace enhanced_bookmarks {
 
-const char* kPageDataKey = "stars.pageData";
-const char* kImageDataKey = "stars.imageData";
-const char* kIdDataKey = "stars.id";
-const char* kNoteKey = "stars.note";
+EnhancedBookmarkModel::EnhancedBookmarkModel(BookmarkModel* bookmark_model,
+                                             const std::string& version)
+    : bookmark_model_(bookmark_model), version_(version) {
+}
+
+EnhancedBookmarkModel::~EnhancedBookmarkModel() {
+}
 
-std::string RemoteIdFromBookmark(BookmarkModel* bookmark_model,
-                                 const BookmarkNode* node) {
-  const BookmarkNode::MetaInfoMap* map = node->GetMetaInfoMap();
-  if (!map)
-    return SetRemoteIdOnBookmark(bookmark_model, node);
+// 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,
+    const BookmarkNode::MetaInfoMap* meta_info) {
+  // TODO(rfevang): Set meta info placement fields.
+  return bookmark_model_->AddFolderWithMetaInfo(
+      parent, index, title, meta_info);
+}
+
+// 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,
+    const BookmarkNode::MetaInfoMap* meta_info) {
+  // TODO(rfevang): Set meta info placement fields.
+  return bookmark_model_->AddURLWithCreationTimeAndMetaInfo(
+      parent, index, title, url, creation_time, meta_info);
+}
+
+std::string EnhancedBookmarkModel::GetRemoteIdForNode(

[noyau (Ping after 24h), 2014/09/05 12:04:43]

Why did you rename everything to "Node"? I prefer the specific "Bookmark" to the
generic "Node". It expresses better what the argument is.

[Rune Fevang, 2014/09/06 00:01:56]

Both bookmarks and folders have remote ids. I renamed as these methods generally
refer to both of them. I can change it back for the ones specific to Bookmarks
(images) if you like, I just liked the consistency.

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

I see, you consider folder are not bookmarks. I don't as the node themselves are
all BookmarkNodes, nodes pointing out that thy are part of the tree type, and
bookmark expessing what they contain. hence for me "Bookmark" is a better
description in the function name here as the nodes are all bookmark node. (I'm
rereading my self and I'm not sure what I am writing is understandable :)

But I see where you are coming from. I would actually prefer everything be
xxxForBookmark, but I would settle to leave the ID ones being xxxForNode, if you
feel strongly about it.

While you are at it can you add a DCHECK to the ones that are not supposed to
take a folder? No need to set an image or description on those.

[Rune Fevang, 2014/09/08 19:29:19]

How about omitting the ForBookmark/ForNode altogether? That's consistent with
how the BookmarkModel names its methods.

Added DCHECK to the image setting methods, folders are allowed to have
descriptions.

[noyau (Ping after 24h), 2014/09/09 08:54:28]

Duh, this solution is so obviously right!

+    const BookmarkNode* node) {
+  if (node == bookmark_model_->bookmark_bar_node())
+    return kBookmarkBarId;
 
-  BookmarkNode::MetaInfoMap::const_iterator it = map->find(kIdDataKey);
-  if (it == map->end())
-    return SetRemoteIdOnBookmark(bookmark_model, node);
+  // Permanent nodes other than the bookmarks bar don't have ids.
+  DCHECK(!bookmark_model_->is_permanent_node(node));
 
-  DCHECK(it->second.length());
-  return it->second;
+  std::string id;
+  if (!node->GetMetaInfo(kIdKey, &id) || id.empty())
+    return SetRemoteIdForNode(node);
+  return id;
 }
 
-void SetDescriptionForBookmark(BookmarkModel* bookmark_model,
-                               const BookmarkNode* node,
-                               const std::string& description) {
-  bookmark_model->SetNodeMetaInfo(node, kNoteKey, description);
+std::string EnhancedBookmarkModel::SetRemoteIdForNode(
+    const BookmarkNode* node) {
+  std::stringstream random_id;
+  // Add prefix depending on whether the node is a folder or not.
+  if (node->is_url())
+    random_id << kBookmarkPrefix;
+  else
+    random_id << kFolderPrefix;
+
+  // Generate 32 digit hex string random suffix.
+  random_id << std::hex << std::setfill('0') << std::setw(16);
+  random_id << base::RandUint64() << base::RandUint64();
+  std::string random_id_str = random_id.str();
+
+  SetNodeMetaInfo(node, kIdKey, random_id_str, false);
+  return random_id_str;
 }
 
-std::string DescriptionFromBookmark(const BookmarkNode* node) {
-  const BookmarkNode::MetaInfoMap* map = node->GetMetaInfoMap();
-  if (!map)
-    return "";
+void EnhancedBookmarkModel::SetDescriptionForNode(
+    const BookmarkNode* node,
+    const std::string& description,
+    bool user_edit) {
+  SetNodeMetaInfo(node, kNoteKey, description, user_edit);
+}
 
+std::string EnhancedBookmarkModel::GetDescriptionForNode(
+    const BookmarkNode* node) {
   // First, look for a custom note set by the user.
-  BookmarkNode::MetaInfoMap::const_iterator it = map->find(kNoteKey);
-  if (it != map->end() && it->second != "")
-    return it->second;
+  std::string description;
+  if (node->GetMetaInfo(kNoteKey, &description) && !description.empty())
+    return description;
 
   // If none are present, return the snippet.
-  return SnippetFromBookmark(node);
+  return GetSnippetForNode(node);
 }
 
-bool SetOriginalImageForBookmark(BookmarkModel* bookmark_model,
-                                 const BookmarkNode* node,
-                                 const GURL& url,
-                                 int width,
-                                 int height) {
+bool EnhancedBookmarkModel::SetOriginalImageForNode(const BookmarkNode* node,
+                                                    const GURL& url,
+                                                    int width,
+                                                    int height,
+                                                    bool user_edit) {
   DCHECK(url.is_valid());
 
   std::string decoded(DataForMetaInfoField(node, kImageDataKey));
-  ImageData data;
+  image::collections::ImageData data;
 
   // Try to populate the imageData with the existing data.
   if (decoded != "") {
@@ -127,7 +171,8 @@ bool SetOriginalImageForBookmark(BookmarkModel* bookmark_model,
       return false;
   }
 
-  scoped_ptr<ImageData_ImageInfo> info(new ImageData_ImageInfo);
+  scoped_ptr<image::collections::ImageData_ImageInfo> info(
+      new image::collections::ImageData_ImageInfo);
   info->set_url(url.spec());
   info->set_width(width);
   info->set_height(height);
@@ -140,21 +185,21 @@ bool SetOriginalImageForBookmark(BookmarkModel* bookmark_model,
 
   std::string encoded;
   base::Base64Encode(output, &encoded);
-  bookmark_model->SetNodeMetaInfo(node, kImageDataKey, encoded);
+  SetNodeMetaInfo(node, kImageDataKey, encoded, user_edit);
   // Ensure that the bookmark has a stars.id, to trigger the server processing.
-  RemoteIdFromBookmark(bookmark_model, node);
+  GetRemoteIdForNode(node);
   return true;
 }
 
-bool OriginalImageFromBookmark(const BookmarkNode* node,
-                               GURL* url,
-                               int* width,
-                               int* height) {
+bool EnhancedBookmarkModel::GetOriginalImageForNode(const BookmarkNode* node,
+                                                    GURL* url,
+                                                    int* width,
+                                                    int* height) {
   std::string decoded(DataForMetaInfoField(node, kImageDataKey));
   if (decoded == "")
     return false;
 
-  ImageData data;
+  image::collections::ImageData data;
   bool result = data.ParseFromString(decoded);
   if (!result)
     return false;
@@ -165,15 +210,15 @@ bool OriginalImageFromBookmark(const BookmarkNode* node,
   return PopulateImageData(data.original_info(), url, width, height);
 }
 
-bool ThumbnailImageFromBookmark(const BookmarkNode* node,
-                                GURL* url,
-                                int* width,
-                                int* height) {
+bool EnhancedBookmarkModel::GetThumbnailImageForNode(const BookmarkNode* node,
+                                                     GURL* url,
+                                                     int* width,
+                                                     int* height) {
   std::string decoded(DataForMetaInfoField(node, kImageDataKey));
   if (decoded == "")
     return false;
 
-  ImageData data;
+  image::collections::ImageData data;
   bool result = data.ParseFromString(decoded);
   if (!result)
     return false;
@@ -184,19 +229,50 @@ bool ThumbnailImageFromBookmark(const BookmarkNode* node,
   return PopulateImageData(data.thumbnail_info(), url, width, height);
 }
 
-std::string SnippetFromBookmark(const BookmarkNode* node) {
+std::string EnhancedBookmarkModel::GetSnippetForNode(const BookmarkNode* node) {
   std::string decoded(DataForMetaInfoField(node, kPageDataKey));
-  if (decoded == "")
+  if (decoded.empty())
     return decoded;
 
-  PageData data;
+  image::collections::PageData data;
   bool result = data.ParseFromString(decoded);
   if (!result)
-    return "";
+    return std::string();
 
   return data.snippet();
 }
 
+void EnhancedBookmarkModel::SetVersionSuffix(
+    const std::string& version_suffix) {
+  version_suffix_ = version_suffix;
+}
+
+void EnhancedBookmarkModel::SetNodeMetaInfo(const BookmarkNode* node,
+                                            const std::string& field,
+                                            const std::string& value,
+                                            bool user_edit) {
+  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());

[Yaron, 2014/09/05 03:16:53]

Some of these fields are quite long. Isn't this a speculative copy of the entire
map for every write to meta info?

[Rune Fevang, 2014/09/05 03:40:26]

It is, however if the fields are set one at a time, sync might send partial
writes to the server. If the fields are long enough that this is an issue that
seems wrong.

[Yaron, 2014/09/05 04:07:37]

Hmm. That's a little concerning re: sync. It's not clear from the API that this
could be an issue but I see that creation uses a MetaInfoMap so at least what's
likely the most expensive case is a little better.

[Rune Fevang, 2014/09/05 05:04:58]

The idea is that any write operations that "belong together" should happen in
one update. This is just a convenience function for the one-field case (since
we're now setting version and userEdit on those also).

[Mark, 2014/09/05 16:27:34]

Sync operates on SyncEntity fidelity.  There is no such thing as a partial write
in terms of Chrome Sync.  Setting all of this information is fine provided the
ordering does not change.

[Rune Fevang, 2014/09/06 00:01:56]

How do you distinguish between a partial write with an old value for the
userEdit field and a complete write that sets it to the value it already have
then? By partial write I mean that sync decides to send updates to the server
before all the fields have changed here (it runs on a different thread).

I really don't think doing three writes is cheaper than copying the map here, if
I remember correctly sync will do a full copy of all fields every time it
receives onMetaInfoChanged anyway. Also the extension API will do a copy when it
receives onMetaInfoWillChange, then compare that to the one present after
getting onMetaInfoChanged to get the diff. So at least in the case when the
extension is running, three updates will be more expensive than doing it this
way.

+
+  // 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_;
+}
+
 bool SetAllImagesForBookmark(BookmarkModel* bookmark_model,
                              const BookmarkNode* node,
                              const GURL& image_url,
@@ -208,7 +284,7 @@ bool SetAllImagesForBookmark(BookmarkModel* bookmark_model,
   DCHECK(image_url.is_valid() || image_url.is_empty());
   DCHECK(thumbnail_url.is_valid() || thumbnail_url.is_empty());
   std::string decoded(DataForMetaInfoField(node, kImageDataKey));
-  ImageData data;
+  image::collections::ImageData data;
 
   // Try to populate the imageData with the existing data.
   if (decoded != "") {
@@ -223,7 +299,8 @@ bool SetAllImagesForBookmark(BookmarkModel* bookmark_model,
   } else {
     // Regardless of whether an image info exists, we make a new one.
     // Intentially make a raw pointer.
-    ImageData_ImageInfo* info = new ImageData_ImageInfo;
+    image::collections::ImageData_ImageInfo* info =
+        new image::collections::ImageData_ImageInfo;
     info->set_url(image_url.spec());
     info->set_width(image_width);
     info->set_height(image_height);
@@ -236,7 +313,8 @@ bool SetAllImagesForBookmark(BookmarkModel* bookmark_model,
   } else {
     // Regardless of whether an image info exists, we make a new one.
     // Intentially make a raw pointer.
-    ImageData_ImageInfo* info = new ImageData_ImageInfo;
+    image::collections::ImageData_ImageInfo* info =
+        new image::collections::ImageData_ImageInfo;
     info->set_url(thumbnail_url.spec());
     info->set_width(thumbnail_width);
     info->set_height(thumbnail_height);