[ModelTypeWorker] Clean up bookmarks-only code before refactoring

This CL prepares for a later refactoring by making semantically
non-equivalent changes first. This CL:
 - guards bookmark specific code by model_type == BOOKMARKS;
 - moving one line around to have all bookmark specific changes after
   the generic ones.

The follow-up CL will extract data type specific code into helper
functions to separate them from the main flow. This prepares for adding
further WALLET_DATA specific code into the worker.

Bug: 881289
Change-Id: Ia197e92294b56ecbe49b8f28f4581b4a8bb581a3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1818482
Auto-Submit: Jan Krcal <jkrcal@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#699666}

components/sync/engine_impl/model_type_worker.cc

@@ -263,62 +263,63 @@ ModelTypeWorker::DecryptionStatus ModelTypeWorker::PopulateUpdateResponseData(
   data->name = update_entity.name();
   data->is_folder = update_entity.folder();
   data->parent_id = update_entity.parent_id_string();
+  data->server_defined_unique_tag = update_entity.server_defined_unique_tag();
 
-  // Handle deprecated positioning fields. Relevant only for bookmarks.
-  bool has_position_scheme = false;
-  SyncPositioningScheme sync_positioning_scheme;
-  if (update_entity.has_unique_position()) {
-    data->unique_position = update_entity.unique_position();
-    has_position_scheme = true;
-    sync_positioning_scheme = SyncPositioningScheme::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;
-    }
+  // Populate |originator_cache_guid| and |originator_client_item_id|. This is
+  // currently relevant only for bookmarks.
+  data->originator_cache_guid = update_entity.originator_cache_guid();
+  data->originator_client_item_id = update_entity.originator_client_item_id();
 
-    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_position_in_parent()) {
-      data->unique_position =
-          UniquePosition::FromInt64(update_entity.position_in_parent(), suffix)
-              .ToProto();
+  // Handle deprecated positioning fields. Relevant only for bookmarks.
+  if (model_type == syncer::BOOKMARKS) {
+    bool has_position_scheme = false;
+    SyncPositioningScheme sync_positioning_scheme;
+    if (update_entity.has_unique_position()) {
+      data->unique_position = update_entity.unique_position();
       has_position_scheme = true;
-      sync_positioning_scheme = SyncPositioningScheme::POSITION_IN_PARENT;
-    } 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();
+      sync_positioning_scheme = SyncPositioningScheme::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_position_in_parent()) {
+        data->unique_position = UniquePosition::FromInt64(
+                                    update_entity.position_in_parent(), suffix)
+                                    .ToProto();
+        has_position_scheme = true;
+        sync_positioning_scheme = SyncPositioningScheme::POSITION_IN_PARENT;
+      } 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();
+        has_position_scheme = true;
+        sync_positioning_scheme = SyncPositioningScheme::INSERT_AFTER_ITEM_ID;
+      }
+    } else if (SyncerProtoUtil::ShouldMaintainPosition(update_entity) &&
+               !update_entity.deleted()) {
+      DLOG(ERROR) << "Missing required position information in update.";
       has_position_scheme = true;
-      sync_positioning_scheme = SyncPositioningScheme::INSERT_AFTER_ITEM_ID;
+      sync_positioning_scheme = SyncPositioningScheme::MISSING;
+    }
+    if (has_position_scheme) {
+      UMA_HISTOGRAM_ENUMERATION("Sync.Entities.PositioningScheme",
+                                sync_positioning_scheme);
     }
-  } else if (SyncerProtoUtil::ShouldMaintainPosition(update_entity) &&
-             !update_entity.deleted()) {
-    DLOG(ERROR) << "Missing required position information in update.";
-    has_position_scheme = true;
-    sync_positioning_scheme = SyncPositioningScheme::MISSING;
-  }
-  if (has_position_scheme) {
-    UMA_HISTOGRAM_ENUMERATION("Sync.Entities.PositioningScheme",
-                              sync_positioning_scheme);
   }
 
-  // Populate |originator_cache_guid| and |originator_client_item_id|. This is
-  // relevant only for bookmarks.
-  data->originator_cache_guid = update_entity.originator_cache_guid();
-  data->originator_client_item_id = update_entity.originator_client_item_id();
-
-  data->server_defined_unique_tag = update_entity.server_defined_unique_tag();
-
   // Deleted entities must use the default instance of EntitySpecifics in
   // order for EntityData to correctly reflect that they are deleted.
   const sync_pb::EntitySpecifics& specifics =

components/sync/engine_impl/model_type_worker_unittest.cc

@@ -1333,8 +1333,6 @@ TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseData) {
   entity.set_id_string("SomeID");
   entity.set_parent_id_string("ParentID");
   entity.set_folder(false);
-  *entity.mutable_unique_position() =
-      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");
@@ -1352,19 +1350,11 @@ TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseData) {
   EXPECT_FALSE(data.id.empty());
   EXPECT_FALSE(data.parent_id.empty());
   EXPECT_FALSE(data.is_folder);
-  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());
   EXPECT_EQ(kTag1, data.specifics.preference().name());
   EXPECT_EQ(kValue1, data.specifics.preference().value());
-
-  histogram_tester.ExpectUniqueSample(
-      "Sync.Entities.PositioningScheme",
-      /*sample=*/
-      ExpectedSyncPositioningScheme::UNIQUE_POSITION,
-      /*count=*/1);
 }
 
 TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseDataForBookmarkTombstone) {
@@ -1395,7 +1385,8 @@ TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseDataForBookmarkTombstone) {
   EXPECT_TRUE(data.is_deleted());
 }
 
-TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseDataWithPositionInParent) {
+TEST_F(ModelTypeWorkerTest,
+       PopulateUpdateResponseDataForBookmarkWithPositionInParent) {
   InitializeCommitOnly();
   sync_pb::SyncEntity entity;
 
@@ -1410,7 +1401,7 @@ TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseDataWithPositionInParent) {
 
   EXPECT_EQ(ModelTypeWorker::SUCCESS,
             ModelTypeWorker::PopulateUpdateResponseData(
-                &cryptographer, PREFERENCES, entity, &response_data));
+                &cryptographer, BOOKMARKS, entity, &response_data));
   const EntityData& data = *response_data.entity;
   EXPECT_TRUE(
       syncer::UniquePosition::FromProto(data.unique_position).IsValid());
@@ -1422,7 +1413,8 @@ TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseDataWithPositionInParent) {
       /*count=*/1);
 }
 
-TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseDataWithInsertAfterItemId) {
+TEST_F(ModelTypeWorkerTest,
+       PopulateUpdateResponseDataForBookmarkWithInsertAfterItemId) {
   InitializeCommitOnly();
   sync_pb::SyncEntity entity;
 
@@ -1437,7 +1429,7 @@ TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseDataWithInsertAfterItemId) {
 
   EXPECT_EQ(ModelTypeWorker::SUCCESS,
             ModelTypeWorker::PopulateUpdateResponseData(
-                &cryptographer, PREFERENCES, entity, &response_data));
+                &cryptographer, BOOKMARKS, entity, &response_data));
   const EntityData& data = *response_data.entity;
   EXPECT_TRUE(
       syncer::UniquePosition::FromProto(data.unique_position).IsValid());
@@ -1449,7 +1441,7 @@ TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseDataWithInsertAfterItemId) {
 }
 
 TEST_F(ModelTypeWorkerTest,
-       PopulateUpdateResponseDataWithBookmarkMissingPosition) {
+       PopulateUpdateResponseDataForBookmarkWithMissingPosition) {
   InitializeCommitOnly();
   sync_pb::SyncEntity entity;
 
@@ -1466,7 +1458,7 @@ TEST_F(ModelTypeWorkerTest,
 
   EXPECT_EQ(ModelTypeWorker::SUCCESS,
             ModelTypeWorker::PopulateUpdateResponseData(
-                &cryptographer, PREFERENCES, entity, &response_data));
+                &cryptographer, BOOKMARKS, entity, &response_data));
   const EntityData& data = *response_data.entity;
   EXPECT_FALSE(
       syncer::UniquePosition::FromProto(data.unique_position).IsValid());
@@ -1477,7 +1469,7 @@ TEST_F(ModelTypeWorkerTest,
 }
 
 TEST_F(ModelTypeWorkerTest,
-       PopulateUpdateResponseDataWithNonBookmarkHasNoPosition) {
+       PopulateUpdateResponseDataForNonBookmarkWithNoPosition) {
   InitializeCommitOnly();
   sync_pb::SyncEntity entity;
 
@@ -1498,7 +1490,7 @@ TEST_F(ModelTypeWorkerTest,
                                     /*count=*/0);
 }
 
-TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseDataWithBookmarkGUID) {
+TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseDataForBookmarkWithGUID) {
   const std::string kGuid1 = base::GenerateGUID();
   const std::string kGuid2 = base::GenerateGUID();
 
@@ -1524,7 +1516,8 @@ TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseDataWithBookmarkGUID) {
   EXPECT_EQ(kGuid2, data.originator_client_item_id);
 }
 
-TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseDataWithMissingBookmarkGUID) {
+TEST_F(ModelTypeWorkerTest,
+       PopulateUpdateResponseDataForBookmarkWithMissingGUID) {
   const std::string kGuid1 = base::GenerateGUID();
 
   NormalInitialize();
@@ -1549,7 +1542,7 @@ TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseDataWithMissingBookmarkGUID) {
 }
 
 TEST_F(ModelTypeWorkerTest,
-       PopulateUpdateResponseDataWithMissingBookmarkGUIDAndInvalidOCII) {
+       PopulateUpdateResponseDataForBookmarkWithMissingGUIDAndInvalidOCII) {
   const std::string kInvalidOCII = "INVALID OCII";
 
   NormalInitialize();