[sync] Precompute UniquePosition in sync thread

The patch adopts the C++ class syncer::UniquePosition instead of the
lower-level raw protocol buffer (sync_pb::UniquePosition) throughout
various layers, most importantly syncer::EntityData.

This has so many advantages that it's surprising that it wasn't done
before:
1. Improves performance, possibly reducing UI jank, due to reducing
   the load on the UI thread for incoming updates. This is notable in
   BookmarkModelMerger, which relies on UniquePosition during sorting,
   and incurred in repeated computation of UniquePosition (which could
   worst-case, for old data, require compression operations).

2. Makes APIs more type safe by adopting higher-level abstractions.
   This is because operations like equality are not identical, depending
   on whether the underlying proto representation is used or the
   higher-level object.

3. Simplifies the implementation and unit tests, improving readability.

Change-Id: I98a370ec17d32ac558d51ff7e4c9386496c3b5db
Bug: 1205351
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2874301
Reviewed-by: Rushan Suleymanov <rushans@google.com>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#880305}

components/sync/base/unique_position.h

@@ -76,6 +76,12 @@ class UniquePosition {
   // Creates an empty, invalid value.
   UniquePosition();
 
+  // Type is copyable and movable.
+  UniquePosition(const UniquePosition&) = default;
+  UniquePosition(UniquePosition&&) = default;
+  UniquePosition& operator=(const UniquePosition&) = default;
+  UniquePosition& operator=(UniquePosition&&) = default;
+
   bool LessThan(const UniquePosition& other) const;
   bool Equals(const UniquePosition& other) const;
 

components/sync/engine/bookmark_update_preprocessing.cc

@@ -111,7 +111,8 @@ void AdaptUniquePositionForBookmark(const sync_pb::SyncEntity& update_entity,
   }
 
   if (update_entity.has_unique_position()) {
-    data->unique_position = update_entity.unique_position();
+    data->unique_position =
+        UniquePosition::FromProto(update_entity.unique_position());
   } else if (update_entity.has_position_in_parent() ||
              update_entity.has_insert_after_item_id()) {
     bool missing_originator_fields = false;
@@ -129,12 +130,11 @@ void AdaptUniquePositionForBookmark(const sync_pb::SyncEntity& update_entity,
 
     if (update_entity.has_position_in_parent()) {
       data->unique_position =
-          UniquePosition::FromInt64(update_entity.position_in_parent(), suffix)
-              .ToProto();
+          UniquePosition::FromInt64(update_entity.position_in_parent(), suffix);
     } else {
       // If update_entity has insert_after_item_id, use 0 index.
       DCHECK(update_entity.has_insert_after_item_id());
-      data->unique_position = UniquePosition::FromInt64(0, suffix).ToProto();
+      data->unique_position = UniquePosition::FromInt64(0, suffix);
     }
   } else {
     DLOG(ERROR) << "Missing required position information in update: "

components/sync/engine/bookmark_update_preprocessing_unittest.cc

@@ -40,8 +40,7 @@ TEST(BookmarkUpdatePreprocessingTest, ShouldPropagateUniquePosition) {
   EntityData entity_data;
   AdaptUniquePositionForBookmark(entity, &entity_data);
 
-  EXPECT_TRUE(
-      syncer::UniquePosition::FromProto(entity_data.unique_position).IsValid());
+  EXPECT_TRUE(entity_data.unique_position.IsValid());
 }
 
 TEST(BookmarkUpdatePreprocessingTest,
@@ -54,8 +53,7 @@ TEST(BookmarkUpdatePreprocessingTest,
   EntityData entity_data;
   AdaptUniquePositionForBookmark(entity, &entity_data);
 
-  EXPECT_TRUE(
-      syncer::UniquePosition::FromProto(entity_data.unique_position).IsValid());
+  EXPECT_TRUE(entity_data.unique_position.IsValid());
 }
 
 TEST(BookmarkUpdatePreprocessingTest,
@@ -68,8 +66,7 @@ TEST(BookmarkUpdatePreprocessingTest,
   EntityData entity_data;
   AdaptUniquePositionForBookmark(entity, &entity_data);
 
-  EXPECT_TRUE(
-      syncer::UniquePosition::FromProto(entity_data.unique_position).IsValid());
+  EXPECT_TRUE(entity_data.unique_position.IsValid());
 }
 
 // Tests that AdaptGuidForBookmark() propagates GUID in specifics if the field

components/sync/engine/commit_contribution_impl.cc

@@ -224,15 +224,11 @@ void CommitContributionImpl::PopulateCommitProto(
     if (type == BOOKMARKS) {
       // position_in_parent field is set only for legacy reasons.  See comments
       // in sync.proto for more information.
-      const UniquePosition unique_position =
-          UniquePosition::FromProto(entity_data.unique_position);
+      const UniquePosition& unique_position = entity_data.unique_position;
       if (unique_position.IsValid()) {
         commit_proto->set_position_in_parent(unique_position.ToInt64());
       }
-      commit_proto->mutable_unique_position()->CopyFrom(
-          entity_data.unique_position);
-      // TODO(mamir): check if parent_id_string needs to be populated for
-      // non-deletions.
+      *commit_proto->mutable_unique_position() = unique_position.ToProto();
       if (!entity_data.parent_id.empty()) {
         commit_proto->set_parent_id_string(entity_data.parent_id);
       }

components/sync/engine/commit_contribution_impl_unittest.cc

@@ -112,9 +112,8 @@ TEST(CommitContributionImplTest, PopulateCommitProtoBookmark) {
   data->name = "Name:";
   data->parent_id = "ParentOf:";
   data->is_folder = true;
-  syncer::UniquePosition uniquePosition = syncer::UniquePosition::FromInt64(
+  data->unique_position = syncer::UniquePosition::FromInt64(
       10, syncer::UniquePosition::RandomSuffix());
-  data->unique_position = uniquePosition.ToProto();
 
   CommitRequestData request_data;
   request_data.sequence_number = 2;

components/sync/engine/entity_data.cc

@@ -21,51 +21,19 @@ namespace syncer {
 
 namespace {
 
-std::string UniquePositionToString(
-    const sync_pb::UniquePosition& unique_position) {
-  return UniquePosition::FromProto(unique_position).ToDebugString();
+std::string UniquePositionToString(const UniquePosition& unique_position) {
+  return unique_position.ToDebugString();
 }
 
 }  // namespace
 
 EntityData::EntityData() = default;
 
-EntityData::EntityData(EntityData&& other)
-    : id(std::move(other.id)),
-      client_tag_hash(std::move(other.client_tag_hash)),
-      originator_cache_guid(std::move(other.originator_cache_guid)),
-      originator_client_item_id(std::move(other.originator_client_item_id)),
-      server_defined_unique_tag(std::move(other.server_defined_unique_tag)),
-      name(std::move(other.name)),
-      creation_time(other.creation_time),
-      modification_time(other.modification_time),
-      parent_id(std::move(other.parent_id)),
-      is_folder(other.is_folder),
-      is_bookmark_guid_in_specifics_preprocessed(
-          other.is_bookmark_guid_in_specifics_preprocessed) {
-  specifics.Swap(&other.specifics);
-  unique_position.Swap(&other.unique_position);
-}
+EntityData::EntityData(EntityData&& other) = default;
 
 EntityData::~EntityData() = default;
 
-EntityData& EntityData::operator=(EntityData&& other) {
-  id = std::move(other.id);
-  client_tag_hash = std::move(other.client_tag_hash);
-  originator_cache_guid = std::move(other.originator_cache_guid);
-  originator_client_item_id = std::move(other.originator_client_item_id);
-  server_defined_unique_tag = std::move(other.server_defined_unique_tag);
-  name = std::move(other.name);
-  creation_time = other.creation_time;
-  modification_time = other.modification_time;
-  parent_id = std::move(other.parent_id);
-  is_folder = other.is_folder;
-  is_bookmark_guid_in_specifics_preprocessed =
-      other.is_bookmark_guid_in_specifics_preprocessed;
-  specifics.Swap(&other.specifics);
-  unique_position.Swap(&other.unique_position);
-  return *this;
-}
+EntityData& EntityData::operator=(EntityData&& other) = default;
 
 #define ADD_TO_DICT(dict, value) \
   dict->SetString(base::ToUpperASCII(#value), value);

components/sync/engine/entity_data.h

@@ -12,6 +12,7 @@
 #include "base/time/time.h"
 #include "base/values.h"
 #include "components/sync/base/client_tag_hash.h"
+#include "components/sync/base/unique_position.h"
 #include "components/sync/protocol/sync.pb.h"
 
 // TODO(crbug.com/947443): Code outside components/sync depends on this file
@@ -85,7 +86,7 @@ struct EntityData {
 
   // Unique position of an entity among its siblings. This is supposed to be
   // set only for datatypes that support positioning (e.g. Bookmarks).
-  sync_pb::UniquePosition unique_position;
+  UniquePosition unique_position;
 
   // True if EntityData represents deleted entity; otherwise false.
   // Note that EntityData would be considered to represent a deletion if its

components/sync/engine/model_type_worker_unittest.cc

@@ -1563,8 +1563,7 @@ TEST(ModelTypeWorkerPopulateUpdateResponseDataTest,
             ModelTypeWorker::PopulateUpdateResponseData(
                 FakeCryptographer(), BOOKMARKS, entity, &response_data));
   const EntityData& data = response_data.entity;
-  EXPECT_TRUE(
-      syncer::UniquePosition::FromProto(data.unique_position).IsValid());
+  EXPECT_TRUE(data.unique_position.IsValid());
 }
 
 TEST(ModelTypeWorkerPopulateUpdateResponseDataTest,
@@ -1582,8 +1581,7 @@ TEST(ModelTypeWorkerPopulateUpdateResponseDataTest,
             ModelTypeWorker::PopulateUpdateResponseData(
                 FakeCryptographer(), BOOKMARKS, entity, &response_data));
   const EntityData& data = response_data.entity;
-  EXPECT_TRUE(
-      syncer::UniquePosition::FromProto(data.unique_position).IsValid());
+  EXPECT_TRUE(data.unique_position.IsValid());
 }
 
 TEST(ModelTypeWorkerPopulateUpdateResponseDataTest,
@@ -1601,8 +1599,7 @@ TEST(ModelTypeWorkerPopulateUpdateResponseDataTest,
             ModelTypeWorker::PopulateUpdateResponseData(
                 FakeCryptographer(), BOOKMARKS, entity, &response_data));
   const EntityData& data = response_data.entity;
-  EXPECT_TRUE(
-      syncer::UniquePosition::FromProto(data.unique_position).IsValid());
+  EXPECT_TRUE(data.unique_position.IsValid());
 }
 
 TEST(ModelTypeWorkerPopulateUpdateResponseDataTest,
@@ -1622,8 +1619,7 @@ TEST(ModelTypeWorkerPopulateUpdateResponseDataTest,
             ModelTypeWorker::PopulateUpdateResponseData(
                 FakeCryptographer(), BOOKMARKS, entity, &response_data));
   const EntityData& data = response_data.entity;
-  EXPECT_FALSE(
-      syncer::UniquePosition::FromProto(data.unique_position).IsValid());
+  EXPECT_FALSE(data.unique_position.IsValid());
 }
 
 TEST(ModelTypeWorkerPopulateUpdateResponseDataTest, NonBookmarkWithNoPosition) {
@@ -1638,8 +1634,7 @@ TEST(ModelTypeWorkerPopulateUpdateResponseDataTest, NonBookmarkWithNoPosition) {
             ModelTypeWorker::PopulateUpdateResponseData(
                 FakeCryptographer(), PREFERENCES, entity, &response_data));
   const EntityData& data = response_data.entity;
-  EXPECT_FALSE(
-      syncer::UniquePosition::FromProto(data.unique_position).IsValid());
+  EXPECT_FALSE(data.unique_position.IsValid());
 }
 
 TEST(ModelTypeWorkerPopulateUpdateResponseDataTest, BookmarkWithGUID) {

components/sync_bookmarks/bookmark_local_changes_builder.cc

@@ -7,6 +7,7 @@
 #include <memory>
 #include <string>
 #include <utility>
+#include <vector>
 
 #include "base/strings/utf_string_conversions.h"
 #include "components/bookmarks/browser/bookmark_model.h"
@@ -90,7 +91,8 @@ syncer::CommitRequestDataList BookmarkLocalChangesBuilder::BuildCommitRequests(
       // 2. Bookmarks (maybe ancient legacy bookmarks only?) use/used |name| to
       //    encode the title.
       data->is_folder = node->is_folder();
-      data->unique_position = metadata->unique_position();
+      data->unique_position =
+          syncer::UniquePosition::FromProto(metadata->unique_position());
       // Assign specifics only for the non-deletion case. In case of deletion,
       // EntityData should contain empty specifics to indicate deletion.
       data->specifics = CreateSpecificsFromBookmarkNode(

components/sync_bookmarks/bookmark_model_merger.cc

@@ -317,8 +317,7 @@ bool IsValidUpdate(const UpdateResponseData& update) {
   DCHECK(!update_entity.is_deleted());
   DCHECK(update_entity.server_defined_unique_tag.empty());
 
-  if (!syncer::UniquePosition::FromProto(update_entity.unique_position)
-           .IsValid()) {
+  if (!update_entity.unique_position.IsValid()) {
     // Ignore updates with invalid positions.
     DLOG(ERROR)
         << "Remote update with invalid position: "
@@ -411,11 +410,7 @@ void BookmarkModelMerger::RemoteTreeNode::EmplaceSelfAndDescendantsByGUID(
 bool BookmarkModelMerger::RemoteTreeNode::UniquePositionLessThan(
     const RemoteTreeNode& lhs,
     const RemoteTreeNode& rhs) {
-  const syncer::UniquePosition a_pos =
-      syncer::UniquePosition::FromProto(lhs.entity().unique_position);
-  const syncer::UniquePosition b_pos =
-      syncer::UniquePosition::FromProto(rhs.entity().unique_position);
-  return a_pos.LessThan(b_pos);
+  return lhs.entity().unique_position.LessThan(rhs.entity().unique_position);
 }
 
 // static
@@ -866,7 +861,7 @@ void BookmarkModelMerger::ProcessLocalCreation(
   const sync_pb::EntitySpecifics specifics = CreateSpecificsFromBookmarkNode(
       node, bookmark_model_, /*force_favicon_load=*/true);
   const SyncedBookmarkTracker::Entity* entity = bookmark_tracker_->Add(
-      node, sync_id, server_version, creation_time, pos.ToProto(), specifics);
+      node, sync_id, server_version, creation_time, pos, specifics);
   // Mark the entity that it needs to be committed.
   bookmark_tracker_->IncrementSequenceNumber(entity);
   for (size_t i = 0; i < node->children().size(); ++i) {

components/sync_bookmarks/bookmark_model_merger_unittest.cc

@@ -81,7 +81,7 @@ class UpdateResponseDataBuilder {
                             const syncer::UniquePosition& unique_position) {
     data_.id = server_id;
     data_.parent_id = parent_id;
-    data_.unique_position = unique_position.ToProto();
+    data_.unique_position = unique_position;
     data_.is_folder = true;
 
     sync_pb::BookmarkSpecifics* bookmark_specifics =

components/sync_bookmarks/bookmark_model_observer_impl.cc

@@ -60,8 +60,8 @@ void BookmarkModelObserverImpl::BookmarkNodeMoved(
 
   const std::string& sync_id = entity->metadata()->server_id();
   const base::Time modification_time = base::Time::Now();
-  const sync_pb::UniquePosition unique_position =
-      ComputePosition(*new_parent, new_index, sync_id).ToProto();
+  const syncer::UniquePosition unique_position =
+      ComputePosition(*new_parent, new_index, sync_id);
 
   sync_pb::EntitySpecifics specifics =
       CreateSpecificsFromBookmarkNode(node, model, /*force_favicon_load=*/true);
@@ -88,13 +88,8 @@ void BookmarkModelObserverImpl::BookmarkNodeAdded(
   // Should be removed after figuring out the reason for the crash.
   CHECK(parent_entity);
 
-  // Assign a temp server id for the entity. Will be overriden by the actual
-  // server id upon receiving commit response.
-  // Local bookmark creations should have used a random GUID so it's safe to
-  // use it as originator client item ID, without the risk for collision.
-  const sync_pb::UniquePosition unique_position =
-      ComputePosition(*parent, index, node->guid().AsLowercaseString())
-          .ToProto();
+  const syncer::UniquePosition unique_position =
+      ComputePosition(*parent, index, node->guid().AsLowercaseString());
 
   sync_pb::EntitySpecifics specifics =
       CreateSpecificsFromBookmarkNode(node, model, /*force_favicon_load=*/true);
@@ -294,7 +289,7 @@ void BookmarkModelObserverImpl::BookmarkNodeChildrenReordered(
         child.get(), model, /*force_favicon_load=*/true);
 
     bookmark_tracker_->Update(entity, entity->metadata()->server_version(),
-                              modification_time, position.ToProto(), specifics);
+                              modification_time, position, specifics);
     // Mark the entity that it needs to be committed.
     bookmark_tracker_->IncrementSequenceNumber(entity);
   }
@@ -383,9 +378,11 @@ void BookmarkModelObserverImpl::ProcessUpdate(
     return;
   }
 
-  bookmark_tracker_->Update(entity, entity->metadata()->server_version(),
-                            /*modification_time=*/base::Time::Now(),
-                            entity->metadata()->unique_position(), specifics);
+  bookmark_tracker_->Update(
+      entity, entity->metadata()->server_version(),
+      /*modification_time=*/base::Time::Now(),
+      syncer::UniquePosition::FromProto(entity->metadata()->unique_position()),
+      specifics);
   // Mark the entity that it needs to be committed.
   bookmark_tracker_->IncrementSequenceNumber(entity);
   nudge_for_commit_closure_.Run();