[Sync::USS] Persist unique_position with bookmarks sync metadata

In order to compute the unique position of a newly created
bookmark node, the unique positions of its siblings are required.

This CL persists the unique position of synced bookmark nodes such that
they are available across client restarts.

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

components/sync/base/unique_position.cc

@@ -17,8 +17,8 @@
 
 namespace syncer {
 
-const size_t UniquePosition::kSuffixLength = 28;
-const size_t UniquePosition::kCompressBytesThreshold = 128;
+constexpr size_t UniquePosition::kSuffixLength;
+constexpr size_t UniquePosition::kCompressBytesThreshold;
 
 // static.
 bool UniquePosition::IsValidSuffix(const std::string& suffix) {
@@ -150,11 +150,12 @@ bool UniquePosition::Equals(const UniquePosition& other) const {
   return compressed_ == other.compressed_;
 }
 
-void UniquePosition::ToProto(sync_pb::UniquePosition* proto) const {
-  proto->Clear();
+sync_pb::UniquePosition UniquePosition::ToProto() const {
+  sync_pb::UniquePosition proto;
 
   // This is the current preferred foramt.
-  proto->set_custom_compressed_v1(compressed_);
+  proto.set_custom_compressed_v1(compressed_);
+  return proto;
 
   // Older clients used to write other formats.  We don't bother doing that
   // anymore because that form of backwards compatibility is expensive.  We no
@@ -164,9 +165,7 @@ void UniquePosition::ToProto(sync_pb::UniquePosition* proto) const {
 
 void UniquePosition::SerializeToString(std::string* blob) const {
   DCHECK(blob);
-  sync_pb::UniquePosition proto;
-  ToProto(&proto);
-  proto.SerializeToString(blob);
+  ToProto().SerializeToString(blob);
 }
 
 int64_t UniquePosition::ToInt64() const {

components/sync/base/unique_position.h

@@ -40,8 +40,8 @@ namespace syncer {
 // though it could be adapted to be more generally useful.
 class UniquePosition {
  public:
-  static const size_t kSuffixLength;
-  static const size_t kCompressBytesThreshold;
+  static constexpr size_t kSuffixLength = 28;
+  static constexpr size_t kCompressBytesThreshold = 128;
 
   static bool IsValidSuffix(const std::string& suffix);
   static bool IsValidBytes(const std::string& bytes);
@@ -82,7 +82,7 @@ class UniquePosition {
   bool Equals(const UniquePosition& other) const;
 
   // Serializes the position's internal state to a protobuf.
-  void ToProto(sync_pb::UniquePosition* proto) const;
+  sync_pb::UniquePosition ToProto() const;
 
   // Serializes the protobuf representation of this object as a string.
   void SerializeToString(std::string* blob) const;

components/sync/base/unique_position_unittest.cc

@@ -13,6 +13,7 @@
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/sha1.h"
+#include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "components/sync/protocol/unique_position.pb.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -21,6 +22,14 @@ namespace syncer {
 
 namespace {
 
+// This function exploits internal knowledge of how the protobufs are serialized
+// to help us build UniquePositions from strings described in this file.
+static UniquePosition FromBytes(const std::string& bytes) {
+  sync_pb::UniquePosition proto;
+  proto.set_value(bytes);
+  return UniquePosition::FromProto(proto);
+}
+
 class UniquePositionTest : public ::testing::Test {
  protected:
   // Accessor to fetch the length of the position's internal representation
@@ -31,19 +40,8 @@ class UniquePositionTest : public ::testing::Test {
   // so you can see how well the algorithm performs in various insertion
   // scenarios.
   size_t GetLength(const UniquePosition& pos) {
-    sync_pb::UniquePosition proto;
-    pos.ToProto(&proto);
-    return proto.ByteSize();
+    return pos.ToProto().ByteSize();
   }
-};
-
-// This function exploits internal knowledge of how the protobufs are serialized
-// to help us build UniquePositions from strings described in this file.
-static UniquePosition FromBytes(const std::string& bytes) {
-  sync_pb::UniquePosition proto;
-  proto.set_value(bytes);
-  return UniquePosition::FromProto(proto);
-}
 
 const size_t kMinLength = UniquePosition::kSuffixLength;
 const size_t kGenericPredecessorLength = kMinLength + 2;
@@ -70,12 +68,45 @@ const UniquePosition kSmallPositionPlusOne =
 const UniquePosition kHugePosition = FromBytes(
     std::string(UniquePosition::kCompressBytesThreshold, '\xFF') + '\xAB');
 
-const std::string kMinSuffix =
-    std::string(UniquePosition::kSuffixLength - 1, '\x00') + '\x01';
-const std::string kMaxSuffix(UniquePosition::kSuffixLength, '\xFF');
-const std::string kNormalSuffix(
-    "\x68\x44\x6C\x6B\x32\x58\x78\x34\x69\x70\x46\x34\x79\x49"
-    "\x44\x4F\x66\x4C\x58\x41\x31\x34\x68\x59\x56\x43\x6F\x3D");
+const UniquePosition kPositionArray[7] = {
+    kGenericPredecessor,   kGenericSuccessor, kBigPosition,
+    kBigPositionLessTwo,   kBiggerPosition,   kSmallPosition,
+    kSmallPositionPlusOne,
+};
+
+const UniquePosition kSortedPositionArray[7] = {
+    kSmallPosition,    kSmallPositionPlusOne, kGenericPredecessor,
+    kGenericSuccessor, kBigPositionLessTwo,   kBigPosition,
+    kBiggerPosition,
+};
+
+const size_t kNumPositions = base::size(kPositionArray);
+const size_t kNumSortedPositions = base::size(kSortedPositionArray);
+};
+
+static constexpr char kMinSuffix[] = {
+    '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00',
+    '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00',
+    '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x00',
+    '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x01'};
+static_assert(base::size(kMinSuffix) == UniquePosition::kSuffixLength,
+              "Wrong size of kMinSuffix.");
+
+static constexpr char kMaxSuffix[] = {
+    '\xFF', '\xFF', '\xFF', '\xFF', '\xFF', '\xFF', '\xFF',
+    '\xFF', '\xFF', '\xFF', '\xFF', '\xFF', '\xFF', '\xFF',
+    '\xFF', '\xFF', '\xFF', '\xFF', '\xFF', '\xFF', '\xFF',
+    '\xFF', '\xFF', '\xFF', '\xFF', '\xFF', '\xFF', '\xFF'};
+static_assert(base::size(kMaxSuffix) == UniquePosition::kSuffixLength,
+              "Wrong size of kMaxSuffix.");
+
+static constexpr char kNormalSuffix[] = {
+    '\x68', '\x44', '\x6C', '\x6B', '\x32', '\x58', '\x78',
+    '\x34', '\x69', '\x70', '\x46', '\x34', '\x79', '\x49',
+    '\x44', '\x4F', '\x66', '\x4C', '\x58', '\x41', '\x31',
+    '\x34', '\x68', '\x59', '\x56', '\x43', '\x6F', '\x3D'};
+static_assert(base::size(kNormalSuffix) == UniquePosition::kSuffixLength,
+              "Wrong size of kNormalSuffix.");
 
 ::testing::AssertionResult LessThan(const char* m_expr,
                                     const char* n_expr,
@@ -149,21 +180,6 @@ TEST_F(UniquePositionTest, DeserializeObsoleteGzippedPosition) {
 
 class RelativePositioningTest : public UniquePositionTest {};
 
-const UniquePosition kPositionArray[] = {
-    kGenericPredecessor,   kGenericSuccessor, kBigPosition,
-    kBigPositionLessTwo,   kBiggerPosition,   kSmallPosition,
-    kSmallPositionPlusOne,
-};
-
-const UniquePosition kSortedPositionArray[] = {
-    kSmallPosition,    kSmallPositionPlusOne, kGenericPredecessor,
-    kGenericSuccessor, kBigPositionLessTwo,   kBigPosition,
-    kBiggerPosition,
-};
-
-static const size_t kNumPositions = arraysize(kPositionArray);
-static const size_t kNumSortedPositions = arraysize(kSortedPositionArray);
-
 struct PositionLessThan {
   bool operator()(const UniquePosition& a, const UniquePosition& b) {
     return a.LessThan(b);
@@ -469,13 +485,16 @@ TEST_F(PositionScenariosTest, TwoClientsInsertAtEnd_B) {
 
 INSTANTIATE_TEST_CASE_P(MinSuffix,
                         PositionInsertTest,
-                        ::testing::Values(kMinSuffix));
+                        ::testing::Values(std::string(kMinSuffix,
+                                                      base::size(kMinSuffix))));
 INSTANTIATE_TEST_CASE_P(MaxSuffix,
                         PositionInsertTest,
-                        ::testing::Values(kMaxSuffix));
-INSTANTIATE_TEST_CASE_P(NormalSuffix,
-                        PositionInsertTest,
-                        ::testing::Values(kNormalSuffix));
+                        ::testing::Values(std::string(kMaxSuffix,
+                                                      base::size(kMaxSuffix))));
+INSTANTIATE_TEST_CASE_P(
+    NormalSuffix,
+    PositionInsertTest,
+    ::testing::Values(std::string(kNormalSuffix, base::size(kNormalSuffix))));
 
 class PositionFromIntTest : public UniquePositionTest {
  public:
@@ -650,9 +669,7 @@ TEST_F(CompressedPositionTest, SerializeAndDeserialize) {
        it != positions_.end(); ++it) {
     SCOPED_TRACE("iteration: " + it->ToDebugString());
 
-    sync_pb::UniquePosition proto;
-    it->ToProto(&proto);
-    UniquePosition deserialized = UniquePosition::FromProto(proto);
+    UniquePosition deserialized = UniquePosition::FromProto(it->ToProto());
 
     EXPECT_PRED_FORMAT2(Equals, *it, deserialized);
   }

components/sync/engine_impl/commit_util.cc

@@ -160,8 +160,8 @@ void BuildCommitItem(const syncable::Entry& meta_entry,
       // in sync.proto for more information.
       sync_entry->set_position_in_parent(
           meta_entry.GetUniquePosition().ToInt64());
-      meta_entry.GetUniquePosition().ToProto(
-          sync_entry->mutable_unique_position());
+      sync_entry->mutable_unique_position()->CopyFrom(
+          meta_entry.GetUniquePosition().ToProto());
     }
     // Always send specifics for bookmarks.
     SetEntrySpecifics(meta_entry, sync_entry);

components/sync/engine_impl/non_blocking_type_commit_contribution_unittest.cc

@@ -105,7 +105,7 @@ TEST(NonBlockingTypeCommitContribution, PopulateCommitProtoBookmark) {
   data.is_folder = true;
   syncer::UniquePosition uniquePosition = syncer::UniquePosition::FromInt64(
       10, syncer::UniquePosition::RandomSuffix());
-  uniquePosition.ToProto(&data.unique_position);
+  data.unique_position = uniquePosition.ToProto();
 
   CommitRequestData request_data;
   request_data.entity = data.PassToPtr();

components/sync/engine_impl/syncer_util_unittest.cc

@@ -57,7 +57,7 @@ class GetUpdatePositionTest : public ::testing::Test {
   }
 
   void InitProtoPosition() {
-    test_position.ToProto(update.mutable_unique_position());
+    update.mutable_unique_position()->CopyFrom(test_position.ToProto());
   }
 
   void InitInt64Position(int64_t pos_value) {

components/sync/model/entity_data_unittest.cc

@@ -39,7 +39,7 @@ TEST_F(EntityDataTest, Swap) {
   UniquePosition unique_position =
       UniquePosition::InitialPosition(UniquePosition::RandomSuffix());
 
-  unique_position.ToProto(&data.unique_position);
+  data.unique_position = unique_position.ToProto();
 
   // Remember addresses of some data within EntitySpecific and UniquePosition
   // to make sure that the underlying data isn't copied.

components/sync/protocol/entity_metadata.proto

@@ -8,6 +8,8 @@ option optimize_for = LITE_RUNTIME;
 
 package sync_pb;
 
+import "unique_position.proto";
+
 // Sync proto to store entity metadata in model type storage.
 message EntityMetadata {
   // A hash based on the client tag and model type.
@@ -57,4 +59,10 @@ message EntityMetadata {
   // value will be the empty string only in the following cases: the entity is
   // in sync with the server, has never been synced, or is deleted.
   optional string base_specifics_hash = 10;
+
+  // Used for positioning entities among their siblings. Relevant only for data
+  // types that support positions (e.g bookmarks). Refer to its definition in
+  // unique_position.proto for more information about its internal
+  // representation.
+  optional UniquePosition unique_position = 11;
 }

components/sync/test/fake_server/bookmark_entity_builder.cc

@@ -75,11 +75,11 @@ sync_pb::EntitySpecifics BookmarkEntityBuilder::CreateBaseEntitySpecifics()
 std::unique_ptr<LoopbackServerEntity> BookmarkEntityBuilder::Build(
     const sync_pb::EntitySpecifics& entity_specifics,
     bool is_folder) {
-  sync_pb::UniquePosition unique_position;
   // TODO(pvalenzuela): Allow caller customization of the position integer.
   const string suffix = GenerateSyncableBookmarkHash(
       originator_cache_guid_, originator_client_item_id_);
-  syncer::UniquePosition::FromInt64(0, suffix).ToProto(&unique_position);
+  sync_pb::UniquePosition unique_position =
+      syncer::UniquePosition::FromInt64(0, suffix).ToProto();
 
   if (parent_id_.empty()) {
     parent_id_ =

components/sync_bookmarks/bookmark_model_type_processor.cc

@@ -330,6 +330,7 @@ void BookmarkModelTypeProcessor::ProcessRemoteCreate(
   }
   bookmark_tracker_->Add(update_entity.id, bookmark_node,
                          update.response_version, update_entity.creation_time,
+                         update_entity.unique_position,
                          update_entity.specifics);
 }
 
@@ -446,6 +447,7 @@ void BookmarkModelTypeProcessor::AssociatePermanentFolder(
   if (permanent_node != nullptr) {
     bookmark_tracker_->Add(update_entity.id, permanent_node,
                            update.response_version, update_entity.creation_time,
+                           update_entity.unique_position,
                            update_entity.specifics);
   }
 }

components/sync_bookmarks/synced_bookmark_tracker.cc

@@ -82,6 +82,7 @@ void SyncedBookmarkTracker::Add(const std::string& sync_id,
                                 const bookmarks::BookmarkNode* bookmark_node,
                                 int64_t server_version,
                                 base::Time creation_time,
+                                const sync_pb::UniquePosition& unique_position,
                                 const sync_pb::EntitySpecifics& specifics) {
   DCHECK_GT(specifics.ByteSize(), 0);
   auto metadata = std::make_unique<sync_pb::EntityMetadata>();
@@ -91,6 +92,7 @@ void SyncedBookmarkTracker::Add(const std::string& sync_id,
   metadata->set_creation_time(syncer::TimeToProtoTime(creation_time));
   metadata->set_sequence_number(0);
   metadata->set_acked_sequence_number(0);
+  metadata->mutable_unique_position()->CopyFrom(unique_position);
   HashSpecifics(specifics, metadata->mutable_specifics_hash());
   sync_id_to_entities_map_[sync_id] =
       std::make_unique<Entity>(bookmark_node, std::move(metadata));

components/sync_bookmarks/synced_bookmark_tracker.h

@@ -15,6 +15,7 @@
 #include "base/time/time.h"
 #include "components/sync/protocol/bookmark_model_metadata.pb.h"
 #include "components/sync/protocol/entity_metadata.pb.h"
+#include "components/sync/protocol/unique_position.pb.h"
 
 namespace bookmarks {
 class BookmarkNode;
@@ -83,6 +84,7 @@ class SyncedBookmarkTracker {
            const bookmarks::BookmarkNode* bookmark_node,
            int64_t server_version,
            base::Time modification_time,
+           const sync_pb::UniquePosition& unique_position,
            const sync_pb::EntitySpecifics& specifics);
 
   // Adds an existing entry for the |sync_id| and the corresponding metadata in

components/sync_bookmarks/synced_bookmark_tracker_unittest.cc

@@ -8,6 +8,7 @@
 #include "base/sha1.h"
 #include "components/bookmarks/browser/bookmark_node.h"
 #include "components/sync/base/time.h"
+#include "components/sync/base/unique_position.h"
 #include "components/sync/model/entity_data.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -38,11 +39,15 @@ TEST(SyncedBookmarkTrackerTest, ShouldGetAssociatedNodes) {
   const int64_t kServerVersion = 1000;
   const base::Time kCreationTime(base::Time::Now() -
                                  base::TimeDelta::FromSeconds(1));
+  const syncer::UniquePosition unique_position =
+      syncer::UniquePosition::InitialPosition(
+          syncer::UniquePosition::RandomSuffix());
   const sync_pb::EntitySpecifics specifics =
       GenerateSpecifics(/*title=*/std::string(), /*url=*/std::string());
 
   bookmarks::BookmarkNode node(kId, kUrl);
-  tracker.Add(kSyncId, &node, kServerVersion, kCreationTime, specifics);
+  tracker.Add(kSyncId, &node, kServerVersion, kCreationTime,
+              unique_position.ToProto(), specifics);
   const SyncedBookmarkTracker::Entity* entity =
       tracker.GetEntityForSyncId(kSyncId);
   ASSERT_THAT(entity, NotNull());
@@ -51,6 +56,10 @@ TEST(SyncedBookmarkTrackerTest, ShouldGetAssociatedNodes) {
   EXPECT_THAT(entity->metadata()->server_version(), Eq(kServerVersion));
   EXPECT_THAT(entity->metadata()->creation_time(),
               Eq(syncer::TimeToProtoTime(kCreationTime)));
+  EXPECT_TRUE(
+      syncer::UniquePosition::FromProto(entity->metadata()->unique_position())
+          .Equals(unique_position));
+
   syncer::EntityData data;
   *data.specifics.mutable_bookmark() = specifics.bookmark();
   EXPECT_TRUE(entity->MatchesData(data));
@@ -65,10 +74,12 @@ TEST(SyncedBookmarkTrackerTest, ShouldReturnNullForDisassociatedNodes) {
   const int64_t kServerVersion = 1000;
   const base::Time kModificationTime(base::Time::Now() -
                                      base::TimeDelta::FromSeconds(1));
+  const sync_pb::UniquePosition unique_position;
   const sync_pb::EntitySpecifics specifics =
       GenerateSpecifics(/*title=*/std::string(), /*url=*/std::string());
   bookmarks::BookmarkNode node(kId, GURL());
-  tracker.Add(kSyncId, &node, kServerVersion, kModificationTime, specifics);
+  tracker.Add(kSyncId, &node, kServerVersion, kModificationTime,
+              unique_position, specifics);
   ASSERT_THAT(tracker.GetEntityForSyncId(kSyncId), NotNull());
   tracker.Remove(kSyncId);
   EXPECT_THAT(tracker.GetEntityForSyncId(kSyncId), IsNull());
@@ -83,10 +94,12 @@ TEST(SyncedBookmarkTrackerTest,
   const int64_t kServerVersion = 1000;
   const base::Time kModificationTime(base::Time::Now() -
                                      base::TimeDelta::FromSeconds(1));
+  const sync_pb::UniquePosition unique_position;
   const sync_pb::EntitySpecifics specifics =
       GenerateSpecifics(/*title=*/std::string(), /*url=*/std::string());
   bookmarks::BookmarkNode node(kId, GURL());
-  tracker.Add(kSyncId, &node, kServerVersion, kModificationTime, specifics);
+  tracker.Add(kSyncId, &node, kServerVersion, kModificationTime,
+              unique_position, specifics);
 
   EXPECT_THAT(tracker.HasLocalChanges(), Eq(false));
   tracker.IncrementSequenceNumber(kSyncId);