[Sync::USS] Compute the unique position for bookmarks if missing

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

components/sync/engine_impl/model_type_worker.cc

@@ -19,10 +19,14 @@
 #include "base/strings/stringprintf.h"
 #include "base/trace_event/memory_usage_estimator.h"
 #include "components/sync/base/cancelation_signal.h"
+#include "components/sync/base/hash_util.h"
+#include "components/sync/base/model_type.h"
 #include "components/sync/base/time.h"
+#include "components/sync/base/unique_position.h"
 #include "components/sync/engine/model_type_processor.h"
 #include "components/sync/engine_impl/commit_contribution.h"
 #include "components/sync/engine_impl/non_blocking_type_commit_contribution.h"
+#include "components/sync/engine_impl/syncer_proto_util.h"
 #include "components/sync/protocol/proto_memory_estimations.h"
 
 namespace {
@@ -184,7 +188,42 @@ ModelTypeWorker::DecryptionStatus ModelTypeWorker::PopulateUpdateResponseData(
   data.non_unique_name = update_entity.name();
   data.is_folder = update_entity.folder();
   data.parent_id = update_entity.parent_id_string();
-  data.unique_position = update_entity.unique_position();
+
+  // Handle deprecated positioning fields. Relevant only for bookmarks.
+  // TODO(crbug.com/516866): Add coressponding UMA metrics for each case below.
+  if (update_entity.has_unique_position()) {
+    data.unique_position = update_entity.unique_position();
+  } else if (update_entity.has_position_in_parent() ||
+             update_entity.has_insert_after_item_id()) {
+    bool missing_originator_fields = false;
+    if (!update_entity.has_originator_cache_guid() ||
+        !update_entity.has_originator_client_item_id()) {
+      DLOG(ERROR) << "Update is missing requirements for bookmark position.";
+      missing_originator_fields = true;
+    }
+
+    std::string suffix =
+        missing_originator_fields
+            ? UniquePosition::RandomSuffix()
+            : GenerateSyncableHash(
+                  syncer::GetModelType(update_entity),
+                  /*client_tag=*/update_entity.originator_cache_guid() +
+                      update_entity.originator_client_item_id());
+
+    if (update_entity.has_originator_cache_guid()) {
+      data.unique_position =
+          UniquePosition::FromInt64(update_entity.position_in_parent(), suffix)
+              .ToProto();
+
+    } else {
+      // If update_entity has insert_after_item_id, use 0 index.
+      data.unique_position = UniquePosition::FromInt64(0, suffix).ToProto();
+    }
+  } else if (SyncerProtoUtil::ShouldMaintainPosition(update_entity) &&
+             !update_entity.deleted()) {
+    DLOG(ERROR) << "Missing required position information in update.";
+  }
+
   data.server_defined_unique_tag = update_entity.server_defined_unique_tag();
 
   // Deleted entities must use the default instance of EntitySpecifics in

components/sync/engine_impl/model_type_worker_unittest.cc

@@ -15,6 +15,7 @@
 #include "components/sync/base/cancelation_signal.h"
 #include "components/sync/base/fake_encryptor.h"
 #include "components/sync/base/hash_util.h"
+#include "components/sync/base/unique_position.h"
 #include "components/sync/engine/model_type_processor.h"
 #include "components/sync/engine_impl/commit_contribution.h"
 #include "components/sync/engine_impl/cycle/non_blocking_type_debug_info_emitter.h"
@@ -1342,7 +1343,9 @@ TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseData) {
   entity.set_id_string("SomeID");
   entity.set_parent_id_string("ParentID");
   entity.set_folder(false);
-  entity.mutable_unique_position()->set_custom_compressed_v1("POSITION");
+  entity.mutable_unique_position()->CopyFrom(
+      UniquePosition::InitialPosition(UniquePosition::RandomSuffix())
+          .ToProto());
   entity.set_version(1);
   entity.set_client_defined_unique_tag("CLIENT_TAG");
   entity.set_server_defined_unique_tag("SERVER_TAG");
@@ -1360,7 +1363,8 @@ TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseData) {
   EXPECT_FALSE(data.id.empty());
   EXPECT_FALSE(data.parent_id.empty());
   EXPECT_FALSE(data.is_folder);
-  EXPECT_TRUE(data.unique_position.has_custom_compressed_v1());
+  EXPECT_TRUE(
+      syncer::UniquePosition::FromProto(data.unique_position).IsValid());
   EXPECT_EQ("CLIENT_TAG", data.client_tag_hash);
   EXPECT_EQ("SERVER_TAG", data.server_defined_unique_tag);
   EXPECT_FALSE(data.is_deleted());
@@ -1368,6 +1372,48 @@ TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseData) {
   EXPECT_EQ(kValue1, data.specifics.preference().value());
 }
 
+TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseDataWithPositionInParent) {
+  InitializeCommitOnly();
+  sync_pb::SyncEntity entity;
+
+  entity.set_position_in_parent(5);
+  entity.set_client_defined_unique_tag("CLIENT_TAG");
+  entity.set_server_defined_unique_tag("SERVER_TAG");
+  entity.mutable_specifics()->CopyFrom(GenerateSpecifics(kTag1, kValue1));
+
+  UpdateResponseData response_data;
+  FakeEncryptor encryptor;
+  Cryptographer cryptographer(&encryptor);
+
+  EXPECT_EQ(ModelTypeWorker::SUCCESS,
+            ModelTypeWorker::PopulateUpdateResponseData(&cryptographer, entity,
+                                                        &response_data));
+  const EntityData& data = response_data.entity.value();
+  EXPECT_TRUE(
+      syncer::UniquePosition::FromProto(data.unique_position).IsValid());
+}
+
+TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseDataWithInsertAfterItemId) {
+  InitializeCommitOnly();
+  sync_pb::SyncEntity entity;
+
+  entity.set_insert_after_item_id("ITEM_ID");
+  entity.set_client_defined_unique_tag("CLIENT_TAG");
+  entity.set_server_defined_unique_tag("SERVER_TAG");
+  entity.mutable_specifics()->CopyFrom(GenerateSpecifics(kTag1, kValue1));
+
+  UpdateResponseData response_data;
+  FakeEncryptor encryptor;
+  Cryptographer cryptographer(&encryptor);
+
+  EXPECT_EQ(ModelTypeWorker::SUCCESS,
+            ModelTypeWorker::PopulateUpdateResponseData(&cryptographer, entity,
+                                                        &response_data));
+  const EntityData& data = response_data.entity.value();
+  EXPECT_TRUE(
+      syncer::UniquePosition::FromProto(data.unique_position).IsValid());
+}
+
 class GetLocalChangesRequestTest : public testing::Test {
  public:
   GetLocalChangesRequestTest();