[Sync::USS] Support metadata import/export in BookmarkModelTypeProcessor

This CL adds support for importing and exporting sync metadata to
the BookmarkModelTypeProcessor.

Sync metadata are compiled into BookmarkModelMetadata proto. This
proto is used to communicate the sync metadata out and into the
processor.

This CL prepares for another CL that would actually persist/load
the sync metadata to/from disk.

Bug: 516866
Change-Id: I0a4af54383ac336a759c0504e4feee1ff1bda7c1
Reviewed-on: https://chromium-review.googlesource.com/1098918
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567982}

components/browser_sync/profile_sync_service.cc

@@ -269,8 +269,9 @@ void ProfileSyncService::Initialize() {
   if (base::FeatureList::IsEnabled(switches::kSyncUSSBookmarks)) {
     bookmark_model_type_processor_ =
         std::make_unique<sync_bookmarks::BookmarkModelTypeProcessor>(
-            sync_client_->GetBookmarkModel(),
             sync_client_->GetBookmarkUndoServiceIfExists());
+    bookmark_model_type_processor_->DecodeSyncMetadata(
+        std::string(), base::DoNothing(), sync_client_->GetBookmarkModel());
   }
 
   device_info_sync_bridge_ = std::make_unique<DeviceInfoSyncBridge>(

components/sync/protocol/bookmark_model_metadata.proto

+// Copyright 2018 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.
+
+syntax = "proto2";
+
+option optimize_for = LITE_RUNTIME;
+
+package sync_pb;
+
+import "model_type_state.proto";
+import "entity_metadata.proto";
+
+// Corresponds to a single bookmark id/metadata pair.
+message BookmarkMetadata {
+  // Bookmark local id.
+  optional int64 id = 1;
+
+  // Bookmarks sync metadata.
+  optional EntityMetadata metadata = 2;
+}
+
+// Sync proto to carry the sync metadata for the bookmarks model. It is used for
+// persisting and loading sync metadata from disk.
+message BookmarkModelMetadata {
+  // Bookmark global metadata.
+  optional ModelTypeState model_type_state = 1;
+
+  // A set of all bookmarks metadata.
+  repeated BookmarkMetadata bookmarks_metadata = 2;
+}

components/sync/protocol/protocol_sources.gni

@@ -10,6 +10,7 @@ sync_protocol_sources = [
   "//components/sync/protocol/arc_package_specifics.proto",
   "//components/sync/protocol/article_specifics.proto",
   "//components/sync/protocol/autofill_specifics.proto",
+  "//components/sync/protocol/bookmark_model_metadata.proto",
   "//components/sync/protocol/bookmark_specifics.proto",
   "//components/sync/protocol/client_commands.proto",
   "//components/sync/protocol/client_debug_info.proto",

components/sync_bookmarks/BUILD.gn

@@ -41,6 +41,7 @@ source_set("unit_tests") {
   deps = [
     ":sync_bookmarks",
     "//base",
+    "//base/test:test_support",
     "//components/bookmarks/browser",
     "//components/bookmarks/test",
     "//components/history/core/browser",

components/sync_bookmarks/bookmark_model_type_processor.h

@@ -6,8 +6,10 @@
 #define COMPONENTS_SYNC_BOOKMARKS_BOOKMARK_MODEL_TYPE_PROCESSOR_H_
 
 #include <memory>
+#include <string>
 #include <vector>
 
+#include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/sequence_checker.h"
@@ -19,6 +21,7 @@ class BookmarkUndoService;
 
 namespace bookmarks {
 class BookmarkModel;
+class BookmarkNode;
 }
 
 namespace sync_bookmarks {
@@ -26,10 +29,9 @@ namespace sync_bookmarks {
 class BookmarkModelTypeProcessor : public syncer::ModelTypeProcessor,
                                    public syncer::ModelTypeControllerDelegate {
  public:
-  // |bookmark_undo_service| and |bookmark_undo_service| must not be nullptr and
-  // must outlive this object.
-  BookmarkModelTypeProcessor(bookmarks::BookmarkModel* bookmark_model,
-                             BookmarkUndoService* bookmark_undo_service);
+  // |bookmark_undo_service| must not be nullptr and must outlive this object.
+  explicit BookmarkModelTypeProcessor(
+      BookmarkUndoService* bookmark_undo_service);
   ~BookmarkModelTypeProcessor() override;
 
   // ModelTypeProcessor implementation.
@@ -51,6 +53,21 @@ class BookmarkModelTypeProcessor : public syncer::ModelTypeProcessor,
   void GetStatusCountersForDebugging(StatusCountersCallback callback) override;
   void RecordMemoryUsageHistogram() override;
 
+  // Encodes all sync metadata into a string, representing a state that can be
+  // restored via DecodeSyncMetadata() below.
+  std::string EncodeSyncMetadata() const;
+
+  // It mainly decodes a BookmarkModelMetadata proto seralized in
+  // |metadata_str|, and uses it to fill in the tracker and the model type state
+  // objects. |model| must not be null and must outlive this object. It is used
+  // to the retrieve the local node ids, and is stored in the processor to be
+  // used for further model operations. |schedule_save_closure| is a repeating
+  // closure used to schedule a save of the bookmark model together with the
+  // metadata.
+  void DecodeSyncMetadata(const std::string& metadata_str,
+                          const base::RepeatingClosure& schedule_save_closure,
+                          bookmarks::BookmarkModel* model);
+
   // Public for testing.
   static std::vector<const syncer::UpdateResponseData*> ReorderUpdatesForTest(
       const syncer::UpdateResponseDataList& updates);
@@ -72,7 +89,7 @@ class BookmarkModelTypeProcessor : public syncer::ModelTypeProcessor,
   // Given a remote update entity, it returns the parent bookmark node of the
   // corresponding node. It returns null if the parent node cannot be found.
   const bookmarks::BookmarkNode* GetParentNode(
-      const syncer::EntityData& update_data) const;
+      const syncer::EntityData& update_entity) const;
 
   // Processes a remote creation of a bookmark node.
   // 1. For permanent folders, they are only registered in |bookmark_tracker_|.
@@ -80,40 +97,55 @@ class BookmarkModelTypeProcessor : public syncer::ModelTypeProcessor,
   //    ignored.
   // 3. Otherwise, a new node is created in the local bookmark model and
   //    registered in |bookmark_tracker_|.
-  void ProcessRemoteCreate(const syncer::EntityData& update_data);
+  void ProcessRemoteCreate(const syncer::UpdateResponseData& update);
 
-  // Processes a remote update of a bookmark node. |update_data| must not be a
+  // Processes a remote update of a bookmark node. |update| must not be a
   // deletion, and the server_id must be already tracked, otherwise, it is a
   // creation that gets handeled in ProcessRemoteCreate(). |tracked_entity| is
   // the tracked entity for that server_id. It is passed as a dependency instead
   // of performing a lookup inside ProcessRemoteUpdate() to avoid wasting CPU
   // cycles for doing another lookup (this code runs on the UI thread).
-  void ProcessRemoteUpdate(const syncer::EntityData& update_data,
+  void ProcessRemoteUpdate(const syncer::UpdateResponseData& update,
                            const SyncedBookmarkTracker::Entity* tracked_entity);
 
-  // Process a remote delete of a bookmark node. |update_data| must not be a
+  // Process a remote delete of a bookmark node. |update_entity| must not be a
   // deletion. |tracked_entity| is the tracked entity for that server_id. It is
   // passed as a dependency instead of performing a lookup inside
   // ProcessRemoteDelete() to avoid wasting CPU cycles for doing another lookup
   // (this code runs on the UI thread).
-  void ProcessRemoteDelete(const syncer::EntityData& update_data,
+  void ProcessRemoteDelete(const syncer::EntityData& update_entity,
                            const SyncedBookmarkTracker::Entity* tracked_entity);
 
   // Associates the permanent bookmark folders with the corresponding server
   // side ids and registers the association in |bookmark_tracker_|.
-  // |update_data| must contain server_defined_unique_tag that is used to
+  // |update_entity| must contain server_defined_unique_tag that is used to
   // determine the corresponding permanent node. All permanent nodes are assumed
   // to be directly children nodes of |kBookmarksRootId|. This method is used in
   // the initial sync cycle only.
-  void AssociatePermanentFolder(const syncer::EntityData& update_data);
+  void AssociatePermanentFolder(const syncer::UpdateResponseData& update);
+
+  // If preconditions are met, inform sync that we are ready to connect.
+  void ConnectIfReady();
+
+  // Stores the start callback in between OnSyncStarting() and
+  // DecodeSyncMetadata().
+  StartCallback start_callback_;
 
   // The bookmark model we are processing local changes from and forwarding
-  // remote changes to.
-  bookmarks::BookmarkModel* const bookmark_model_;
+  // remote changes to. It is set during DecodeSyncMetadata(), which is called
+  // during startup, as part of the bookmark-loading process.
+  bookmarks::BookmarkModel* bookmark_model_ = nullptr;
 
   // Used to suspend bookmark undo when processing remote changes.
   BookmarkUndoService* const bookmark_undo_service_;
 
+  // The callback used to schedule the persistence of bookmark model as well as
+  // the metadata to a file during which latest metadata should also be pulled
+  // via EncodeSyncMetadata. Processor should invoke it upon changes in the
+  // metadata that don't imply changes in the model itself. Persisting updates
+  // that imply model changes is the model's responsibility.
+  base::RepeatingClosure schedule_save_closure_;
+
   // Reference to the CommitQueue.
   //
   // The interface hides the posting of tasks across threads as well as the
@@ -123,7 +155,9 @@ class BookmarkModelTypeProcessor : public syncer::ModelTypeProcessor,
 
   // Keeps the mapping between server ids and bookmarks nodes. It also caches
   // the metadata upon a local change until the commit configration is received.
-  SyncedBookmarkTracker bookmark_tracker_;
+  // It is constructed and set during DecodeSyncMetadata(), which is called
+  // during startup, as part of the bookmark-loading process.
+  std::unique_ptr<SyncedBookmarkTracker> bookmark_tracker_;
 
   base::WeakPtrFactory<BookmarkModelTypeProcessor> weak_ptr_factory_;
 

components/sync_bookmarks/synced_bookmark_tracker.cc

@@ -6,30 +6,70 @@
 
 #include <utility>
 
+#include "base/base64.h"
+#include "base/sha1.h"
+#include "components/bookmarks/browser/bookmark_node.h"
+#include "components/sync/base/time.h"
 #include "components/sync/model/entity_data.h"
 
 namespace sync_bookmarks {
 
+namespace {
+
+void HashSpecifics(const sync_pb::EntitySpecifics& specifics,
+                   std::string* hash) {
+  DCHECK_GT(specifics.ByteSize(), 0);
+  base::Base64Encode(base::SHA1HashString(specifics.SerializeAsString()), hash);
+}
+
+}  // namespace
+
 SyncedBookmarkTracker::Entity::Entity(
-    const bookmarks::BookmarkNode* bookmark_node)
-    : bookmark_node_(bookmark_node) {
-  DCHECK(bookmark_node);
+    const bookmarks::BookmarkNode* bookmark_node,
+    std::unique_ptr<sync_pb::EntityMetadata> metadata)
+    : bookmark_node_(bookmark_node), metadata_(std::move(metadata)) {
+  if (bookmark_node) {
+    DCHECK(!metadata_->is_deleted());
+  } else {
+    DCHECK(metadata_->is_deleted());
+  }
 }
 
 SyncedBookmarkTracker::Entity::~Entity() = default;
 
 bool SyncedBookmarkTracker::Entity::MatchesData(
     const syncer::EntityData& data) const {
-  // TODO(crbug.com/516866): Implement properly.
-  return false;
+  if (metadata_->is_deleted() || data.is_deleted()) {
+    // In case of deletion, no need to check the specifics.
+    return metadata_->is_deleted() == data.is_deleted();
+  }
+  return MatchesSpecificsHash(data.specifics);
+}
+
+bool SyncedBookmarkTracker::Entity::MatchesSpecificsHash(
+    const sync_pb::EntitySpecifics& specifics) const {
+  DCHECK(!metadata_->is_deleted());
+  DCHECK_GT(specifics.ByteSize(), 0);
+  std::string hash;
+  HashSpecifics(specifics, &hash);
+  return hash == metadata_->specifics_hash();
 }
 
 bool SyncedBookmarkTracker::Entity::IsUnsynced() const {
-  // TODO(crbug.com/516866): Implement properly.
-  return false;
+  return metadata_->sequence_number() > metadata_->acked_sequence_number();
+}
+
+SyncedBookmarkTracker::SyncedBookmarkTracker(
+    std::vector<NodeMetadataPair> nodes_metadata,
+    std::unique_ptr<sync_pb::ModelTypeState> model_type_state)
+    : model_type_state_(std::move(model_type_state)) {
+  for (NodeMetadataPair& node_metadata : nodes_metadata) {
+    const std::string& sync_id = node_metadata.second->server_id();
+    sync_id_to_entities_map_[sync_id] = std::make_unique<Entity>(
+        node_metadata.first, std::move(node_metadata.second));
+  }
 }
 
-SyncedBookmarkTracker::SyncedBookmarkTracker() = default;
 SyncedBookmarkTracker::~SyncedBookmarkTracker() = default;
 
 const SyncedBookmarkTracker::Entity* SyncedBookmarkTracker::GetEntityForSyncId(
@@ -39,14 +79,56 @@ const SyncedBookmarkTracker::Entity* SyncedBookmarkTracker::GetEntityForSyncId(
 }
 
 void SyncedBookmarkTracker::Add(const std::string& sync_id,
-                                const bookmarks::BookmarkNode* bookmark_node) {
-  sync_id_to_entities_map_[sync_id] = std::make_unique<Entity>(bookmark_node);
+                                const bookmarks::BookmarkNode* bookmark_node,
+                                int64_t server_version,
+                                base::Time creation_time,
+                                const sync_pb::EntitySpecifics& specifics) {
+  DCHECK_GT(specifics.ByteSize(), 0);
+  auto metadata = std::make_unique<sync_pb::EntityMetadata>();
+  metadata->set_is_deleted(false);
+  metadata->set_server_id(sync_id);
+  metadata->set_server_version(server_version);
+  metadata->set_creation_time(syncer::TimeToProtoTime(creation_time));
+  HashSpecifics(specifics, metadata->mutable_specifics_hash());
+  sync_id_to_entities_map_[sync_id] =
+      std::make_unique<Entity>(bookmark_node, std::move(metadata));
+}
+
+void SyncedBookmarkTracker::Update(const std::string& sync_id,
+                                   int64_t server_version,
+                                   base::Time modification_time,
+                                   const sync_pb::EntitySpecifics& specifics) {
+  DCHECK_GT(specifics.ByteSize(), 0);
+  auto it = sync_id_to_entities_map_.find(sync_id);
+  Entity* entity = it->second.get();
+  DCHECK(entity);
+  entity->metadata()->set_server_id(sync_id);
+  entity->metadata()->set_server_version(server_version);
+  entity->metadata()->set_modification_time(
+      syncer::TimeToProtoTime(modification_time));
+  HashSpecifics(specifics, entity->metadata()->mutable_specifics_hash());
 }
 
 void SyncedBookmarkTracker::Remove(const std::string& sync_id) {
   sync_id_to_entities_map_.erase(sync_id);
 }
 
+sync_pb::BookmarkModelMetadata
+SyncedBookmarkTracker::BuildBookmarkModelMetadata() const {
+  sync_pb::BookmarkModelMetadata model_metadata;
+  for (const std::pair<const std::string, std::unique_ptr<Entity>>& pair :
+       sync_id_to_entities_map_) {
+    sync_pb::BookmarkMetadata* bookmark_metadata =
+        model_metadata.add_bookmarks_metadata();
+    if (pair.second->bookmark_node() != nullptr) {
+      bookmark_metadata->set_id(pair.second->bookmark_node()->id());
+    }
+    *bookmark_metadata->mutable_metadata() = *pair.second->metadata();
+  }
+  *model_metadata.mutable_model_type_state() = *model_type_state_;
+  return model_metadata;
+}
+
 std::size_t SyncedBookmarkTracker::TrackedEntitiesCountForTest() const {
   return sync_id_to_entities_map_.size();
 }

components/sync_bookmarks/synced_bookmark_tracker.h

@@ -8,8 +8,12 @@
 #include <map>
 #include <memory>
 #include <string>
+#include <utility>
+#include <vector>
 
 #include "base/macros.h"
+#include "base/time/time.h"
+#include "components/sync/protocol/bookmark_model_metadata.pb.h"
 #include "components/sync/protocol/entity_metadata.pb.h"
 
 namespace bookmarks {
@@ -22,6 +26,9 @@ struct EntityData;
 
 namespace sync_bookmarks {
 
+using NodeMetadataPair = std::pair<const bookmarks::BookmarkNode*,
+                                   std::unique_ptr<sync_pb::EntityMetadata>>;
+
 // This class is responsible for keeping the mapping between bookmarks node in
 // the local model and the server-side corresponding sync entities. It manages
 // the metadata for its entity and caches entity data upon a local change until
@@ -30,8 +37,9 @@ class SyncedBookmarkTracker {
  public:
   class Entity {
    public:
-    // |bookmark_node| must not be null and must outlive this object.
-    explicit Entity(const bookmarks::BookmarkNode* bookmark_node);
+    // |bookmark_node| can be null for tombstones. |metadata| must not be null.
+    Entity(const bookmarks::BookmarkNode* bookmark_node,
+           std::unique_ptr<sync_pb::EntityMetadata> metadata);
     ~Entity();
 
     // Returns true if this data is out of sync with the server.
@@ -41,32 +49,55 @@ class SyncedBookmarkTracker {
     // Check whether |data| matches the stored specifics hash.
     bool MatchesData(const syncer::EntityData& data) const;
 
-    // It never returns null.
+    // Returns null for tomstones.
     const bookmarks::BookmarkNode* bookmark_node() const {
       return bookmark_node_;
     }
 
+    const sync_pb::EntityMetadata* metadata() const { return metadata_.get(); }
+    sync_pb::EntityMetadata* metadata() { return metadata_.get(); }
+
    private:
+    // Check whether |specifics| matches the stored specifics_hash.
+    bool MatchesSpecificsHash(const sync_pb::EntitySpecifics& specifics) const;
+
     const bookmarks::BookmarkNode* const bookmark_node_;
 
+    // Serializable Sync metadata.
+    std::unique_ptr<sync_pb::EntityMetadata> metadata_;
+
     DISALLOW_COPY_AND_ASSIGN(Entity);
   };
 
-  SyncedBookmarkTracker();
+  SyncedBookmarkTracker(
+      std::vector<NodeMetadataPair> nodes_metadata,
+      std::unique_ptr<sync_pb::ModelTypeState> model_type_state);
   ~SyncedBookmarkTracker();
 
   // Returns null if not entity is found.
   const Entity* GetEntityForSyncId(const std::string& sync_id) const;
 
   // Adds an entry for the |sync_id| and the corresponding local bookmark node
-  // in |sync_id_to_entities_map_|.
+  // and metadata in |sync_id_to_entities_map_|.
   void Add(const std::string& sync_id,
-           const bookmarks::BookmarkNode* bookmark_node);
+           const bookmarks::BookmarkNode* bookmark_node,
+           int64_t server_version,
+           base::Time modification_time,
+           const sync_pb::EntitySpecifics& specifics);
+
+  // Adds an existing entry for the |sync_id| and the corresponding metadata in
+  // |sync_id_to_entities_map_|.
+  void Update(const std::string& sync_id,
+              int64_t server_version,
+              base::Time modification_time,
+              const sync_pb::EntitySpecifics& specifics);
 
   // Removes the entry coressponding to the |sync_id| from
   // |sync_id_to_entities_map_|.
   void Remove(const std::string& sync_id);
 
+  sync_pb::BookmarkModelMetadata BuildBookmarkModelMetadata() const;
+
   // Returns number of tracked entities. Used only in test.
   std::size_t TrackedEntitiesCountForTest() const;
 
@@ -77,6 +108,9 @@ class SyncedBookmarkTracker {
   // contain model type data/specifics.
   std::map<std::string, std::unique_ptr<Entity>> sync_id_to_entities_map_;
 
+  // The model metadata (progress marker, initial sync done, etc).
+  std::unique_ptr<sync_pb::ModelTypeState> model_type_state_;
+
   DISALLOW_COPY_AND_ASSIGN(SyncedBookmarkTracker);
 };
 

components/sync_bookmarks/synced_bookmark_tracker_unittest.cc

@@ -4,7 +4,11 @@
 
 #include "components/sync_bookmarks/synced_bookmark_tracker.h"
 
+#include "base/base64.h"
+#include "base/sha1.h"
 #include "components/bookmarks/browser/bookmark_node.h"
+#include "components/sync/base/time.h"
+#include "components/sync/model/entity_data.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -16,25 +20,55 @@ namespace sync_bookmarks {
 
 namespace {
 
+sync_pb::EntitySpecifics GenerateSpecifics(const std::string& title,
+                                           const std::string& url) {
+  sync_pb::EntitySpecifics specifics;
+  specifics.mutable_bookmark()->set_title(title);
+  specifics.mutable_bookmark()->set_url(url);
+  return specifics;
+}
+
 TEST(SyncedBookmarkTrackerTest, ShouldGetAssociatedNodes) {
-  SyncedBookmarkTracker tracker;
+  SyncedBookmarkTracker tracker(std::vector<NodeMetadataPair>(),
+                                std::make_unique<sync_pb::ModelTypeState>());
   const std::string kSyncId = "SYNC_ID";
+  const std::string kTitle = "Title";
+  const GURL kUrl("http://www.foo.com");
   const int64_t kId = 1;
-  bookmarks::BookmarkNode node(kId, GURL());
-  tracker.Add(kSyncId, &node);
+  const int64_t kServerVersion = 1000;
+  const base::Time kCreationTime(base::Time::Now() -
+                                 base::TimeDelta::FromSeconds(1));
+  const sync_pb::EntitySpecifics specifics =
+      GenerateSpecifics(std::string(), std::string());
+
+  bookmarks::BookmarkNode node(kId, kUrl);
+  tracker.Add(kSyncId, &node, kServerVersion, kCreationTime, specifics);
   const SyncedBookmarkTracker::Entity* entity =
       tracker.GetEntityForSyncId(kSyncId);
   ASSERT_THAT(entity, NotNull());
   EXPECT_THAT(entity->bookmark_node(), Eq(&node));
+  EXPECT_THAT(entity->metadata()->server_id(), Eq(kSyncId));
+  EXPECT_THAT(entity->metadata()->server_version(), Eq(kServerVersion));
+  EXPECT_THAT(entity->metadata()->creation_time(),
+              Eq(syncer::TimeToProtoTime(kCreationTime)));
+  syncer::EntityData data;
+  *data.specifics.mutable_bookmark() = specifics.bookmark();
+  EXPECT_TRUE(entity->MatchesData(data));
   EXPECT_THAT(tracker.GetEntityForSyncId("unknown id"), IsNull());
 }
 
 TEST(SyncedBookmarkTrackerTest, ShouldReturnNullForDisassociatedNodes) {
-  SyncedBookmarkTracker tracker;
+  SyncedBookmarkTracker tracker(std::vector<NodeMetadataPair>(),
+                                std::make_unique<sync_pb::ModelTypeState>());
   const std::string kSyncId = "SYNC_ID";
   const int64_t kId = 1;
+  const int64_t kServerVersion = 1000;
+  const base::Time kModificationTime(base::Time::Now() -
+                                     base::TimeDelta::FromSeconds(1));
+  const sync_pb::EntitySpecifics specifics =
+      GenerateSpecifics(std::string(), std::string());
   bookmarks::BookmarkNode node(kId, GURL());
-  tracker.Add(kSyncId, &node);
+  tracker.Add(kSyncId, &node, kServerVersion, kModificationTime, specifics);
   ASSERT_THAT(tracker.GetEntityForSyncId(kSyncId), NotNull());
   tracker.Remove(kSyncId);
   EXPECT_THAT(tracker.GetEntityForSyncId(kSyncId), IsNull());