Move synced bookmark preprocessing logic to dedicated library

Logic in model_type_worker.cc has grown to large already and is now
moved to a separate file, with dedicated tests.

The code is moved as-is with minor trivial naming changes and tests
are newly added.

Bug: 978430,1007199
Change-Id: Iba79cc3014ae631e1e2029e6fc8a0a9a832a38cf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1953729
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#723373}

components/sync/BUILD.gn

@@ -111,6 +111,8 @@ jumbo_static_library("rest_of_sync") {
     "engine_impl/apply_control_data_updates.h",
     "engine_impl/backoff_delay_provider.cc",
     "engine_impl/backoff_delay_provider.h",
+    "engine_impl/bookmark_update_preprocessing.cc",
+    "engine_impl/bookmark_update_preprocessing.h",
     "engine_impl/commit.cc",
     "engine_impl/commit.h",
     "engine_impl/commit_contribution.cc",
@@ -600,6 +602,7 @@ source_set("unit_tests") {
     "engine/ui_model_worker_unittest.cc",
     "engine_impl/apply_control_data_updates_unittest.cc",
     "engine_impl/backoff_delay_provider_unittest.cc",
+    "engine_impl/bookmark_update_preprocessing_unittest.cc",
     "engine_impl/cycle/data_type_debug_info_emitter_unittest.cc",
     "engine_impl/cycle/nudge_tracker_unittest.cc",
     "engine_impl/cycle/status_controller_unittest.cc",

components/sync/engine_impl/bookmark_update_preprocessing.cc

+// Copyright 2019 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.
+
+#include "components/sync/engine_impl/bookmark_update_preprocessing.h"
+
+#include <array>
+#include <string>
+
+#include "base/guid.h"
+#include "base/logging.h"
+#include "base/metrics/histogram_functions.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/strings/stringprintf.h"
+#include "components/sync/base/hash_util.h"
+#include "components/sync/base/unique_position.h"
+#include "components/sync/engine_impl/syncer_proto_util.h"
+#include "components/sync/model/entity_data.h"
+#include "components/sync/protocol/sync.pb.h"
+
+namespace syncer {
+
+namespace {
+
+// Enumeration of possible values for the positioning schemes used in Sync
+// entities. Used in UMA metrics. Do not re-order or delete these entries; they
+// are used in a UMA histogram. Please edit SyncPositioningScheme in enums.xml
+// if a value is added.
+enum class SyncPositioningScheme {
+  kUniquePosition = 0,
+  kPositionInParent = 1,
+  kInsertAfterItemId = 2,
+  kMissing = 3,
+  kMaxValue = kMissing
+};
+
+// Used in metric "Sync.BookmarkGUIDSource2". These values are persisted to
+// logs. Entries should not be renumbered and numeric values should never be
+// reused.
+enum class BookmarkGuidSource {
+  // GUID came from specifics.
+  kSpecifics = 0,
+  // GUID came from originator_client_item_id and is valid.
+  kValidOCII = 1,
+  // GUID not found in the specifics and originator_client_item_id is invalid,
+  // so field left empty.
+  kLeftEmpty = 2,
+  kMaxValue = kLeftEmpty,
+};
+
+inline void LogGuidSource(BookmarkGuidSource source) {
+  base::UmaHistogramEnumeration("Sync.BookmarkGUIDSource2", source);
+}
+
+}  // namespace
+
+void AdaptUniquePositionForBookmark(const sync_pb::SyncEntity& update_entity,
+                                    EntityData* data) {
+  DCHECK(data);
+  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::kUniquePosition;
+  } 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()
+                             : GenerateSyncableBookmarkHash(
+                                   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::kPositionInParent;
+    } 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::kInsertAfterItemId;
+    }
+  } 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::kMissing;
+  }
+  if (has_position_scheme) {
+    UMA_HISTOGRAM_ENUMERATION("Sync.Entities.PositioningScheme",
+                              sync_positioning_scheme);
+  }
+}
+
+void AdaptTitleForBookmark(const sync_pb::SyncEntity& update_entity,
+                           sync_pb::EntitySpecifics* specifics,
+                           bool specifics_were_encrypted) {
+  DCHECK(specifics);
+  if (specifics_were_encrypted || update_entity.deleted()) {
+    // If encrypted, the name field is never populated (unencrypted) for privacy
+    // reasons. Encryption was also introduced after moving the name out of
+    // SyncEntity so this hack is not needed at all.
+    return;
+  }
+  // Legacy clients populate the name field in the SyncEntity instead of the
+  // title field in the BookmarkSpecifics.
+  if (!specifics->bookmark().has_title() && !update_entity.name().empty()) {
+    specifics->mutable_bookmark()->set_title(update_entity.name());
+  }
+}
+
+void AdaptGuidForBookmark(const sync_pb::SyncEntity& update_entity,
+                          sync_pb::EntitySpecifics* specifics) {
+  DCHECK(specifics);
+  // Tombstones and permanent entities don't have a GUID.
+  if (update_entity.deleted() ||
+      !update_entity.server_defined_unique_tag().empty()) {
+    return;
+  }
+  // Legacy clients don't populate the guid field in the BookmarkSpecifics, so
+  // we use the originator_client_item_id instead, if it is a valid GUID.
+  // Otherwise, we leave the field empty.
+  if (specifics->bookmark().has_guid()) {
+    LogGuidSource(BookmarkGuidSource::kSpecifics);
+  } else if (base::IsValidGUID(update_entity.originator_client_item_id())) {
+    specifics->mutable_bookmark()->set_guid(
+        update_entity.originator_client_item_id());
+    LogGuidSource(BookmarkGuidSource::kValidOCII);
+  } else {
+    LogGuidSource(BookmarkGuidSource::kLeftEmpty);
+  }
+}
+
+void AdaptGuidForBookmarkEntityData(EntityData* entity_data) {
+  DCHECK(entity_data);
+  DCHECK(entity_data->specifics.has_bookmark());
+  DCHECK(!entity_data->specifics.has_encrypted());
+
+  if (entity_data->is_deleted() ||
+      !entity_data->server_defined_unique_tag.empty()) {
+    return;
+  }
+
+  if (entity_data->specifics.bookmark().has_guid()) {
+    LogGuidSource(BookmarkGuidSource::kSpecifics);
+  } else if (base::IsValidGUID(entity_data->originator_client_item_id)) {
+    entity_data->specifics.mutable_bookmark()->set_guid(
+        entity_data->originator_client_item_id);
+    LogGuidSource(BookmarkGuidSource::kValidOCII);
+  } else {
+    LogGuidSource(BookmarkGuidSource::kLeftEmpty);
+  }
+}
+
+}  // namespace syncer

components/sync/engine_impl/bookmark_update_preprocessing.h

+// Copyright 2019 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.
+
+// This file contains various utility functions used by ModelTypeWorker to
+// preprocess remote bookmark updates, to deal with backward-compatibility of
+// data and migrate updates such that they resemble updates from a modern
+// client.
+
+#ifndef COMPONENTS_SYNC_ENGINE_IMPL_BOOKMARK_UPDATE_PREPROCESSING_H_
+#define COMPONENTS_SYNC_ENGINE_IMPL_BOOKMARK_UPDATE_PREPROCESSING_H_
+
+namespace sync_pb {
+class SyncEntity;
+class EntitySpecifics;
+}  // namespace sync_pb
+
+namespace syncer {
+
+struct EntityData;
+
+// Populates |data->unique_position| from the various supported proto fields in
+// |update_entity|. |data| must not be null.
+void AdaptUniquePositionForBookmark(const sync_pb::SyncEntity& update_entity,
+                                    EntityData* data);
+
+// Populates |specifics->bookmark().title()| from the various supported sources,
+// or no-op if specifics already have the field set. |specifics| must not be
+// null.
+void AdaptTitleForBookmark(const sync_pb::SyncEntity& update_entity,
+                           sync_pb::EntitySpecifics* specifics,
+                           bool specifics_were_encrypted);
+
+// Populates |specifics->bookmark().guid()| from the various supported sources,
+// or no-op if specifics already have the field set. |specifics| must not be
+// null.
+void AdaptGuidForBookmark(const sync_pb::SyncEntity& update_entity,
+                          sync_pb::EntitySpecifics* specifics);
+
+// Analogous to AdaptGuidForBookmark() but deals with EntityData structs.
+// |entity_data| must not be null.
+// TODO(crbug.com/1007199): Reconcile with AdaptGuidForBookmark().
+void AdaptGuidForBookmarkEntityData(EntityData* entity_data);
+
+}  // namespace syncer
+
+#endif  // COMPONENTS_SYNC_ENGINE_IMPL_BOOKMARK_UPDATE_PREPROCESSING_H_

components/sync/engine_impl/bookmark_update_preprocessing_unittest.cc

+// Copyright 2019 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.
+
+#include "components/sync/engine_impl/bookmark_update_preprocessing.h"
+
+#include <stdint.h>
+
+#include <string>
+
+#include "base/guid.h"
+#include "base/test/metrics/histogram_tester.h"
+#include "components/sync/base/unique_position.h"
+#include "components/sync/model/entity_data.h"
+#include "components/sync/protocol/sync.pb.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace syncer {
+
+namespace {
+
+using testing::Eq;
+using testing::IsEmpty;
+
+enum class ExpectedSyncPositioningScheme {
+  kUniquePosition = 0,
+  kPositionInParent = 1,
+  kInsertAfterItemId = 2,
+  kMissing = 3,
+  kMaxValue = kMissing
+};
+
+enum class ExpectedBookmarkGuidSource {
+  kSpecifics = 0,
+  kValidOCII = 1,
+  kLeftEmpty = 2,
+  kMaxValue = kLeftEmpty,
+};
+
+TEST(BookmarkUpdatePreprocessingTest, ShouldPropagateUniquePosition) {
+  sync_pb::SyncEntity entity;
+  entity.set_originator_cache_guid(base::GenerateGUID());
+  entity.set_originator_client_item_id("1");
+  *entity.mutable_unique_position() =
+      UniquePosition::InitialPosition(UniquePosition::RandomSuffix()).ToProto();
+
+  base::HistogramTester histogram_tester;
+  EntityData entity_data;
+  AdaptUniquePositionForBookmark(entity, &entity_data);
+
+  EXPECT_TRUE(
+      syncer::UniquePosition::FromProto(entity_data.unique_position).IsValid());
+
+  histogram_tester.ExpectUniqueSample(
+      "Sync.Entities.PositioningScheme",
+      /*sample=*/
+      ExpectedSyncPositioningScheme::kUniquePosition,
+      /*count=*/1);
+}
+
+TEST(BookmarkUpdatePreprocessingTest,
+     ShouldComputeUniquePositionFromPositionInParent) {
+  sync_pb::SyncEntity entity;
+  entity.set_originator_cache_guid(base::GenerateGUID());
+  entity.set_originator_client_item_id("1");
+  entity.set_position_in_parent(5);
+
+  base::HistogramTester histogram_tester;
+  EntityData entity_data;
+  AdaptUniquePositionForBookmark(entity, &entity_data);
+
+  EXPECT_TRUE(
+      syncer::UniquePosition::FromProto(entity_data.unique_position).IsValid());
+
+  histogram_tester.ExpectUniqueSample(
+      "Sync.Entities.PositioningScheme",
+      /*sample=*/
+      ExpectedSyncPositioningScheme::kPositionInParent,
+      /*count=*/1);
+}
+
+TEST(BookmarkUpdatePreprocessingTest,
+     ShouldComputeUniquePositionFromInsertAfterItemId) {
+  sync_pb::SyncEntity entity;
+  entity.set_originator_cache_guid(base::GenerateGUID());
+  entity.set_originator_client_item_id("1");
+  entity.set_insert_after_item_id("ITEM_ID");
+
+  base::HistogramTester histogram_tester;
+  EntityData entity_data;
+  AdaptUniquePositionForBookmark(entity, &entity_data);
+
+  EXPECT_TRUE(
+      syncer::UniquePosition::FromProto(entity_data.unique_position).IsValid());
+
+  histogram_tester.ExpectUniqueSample(
+      "Sync.Entities.PositioningScheme",
+      /*sample=*/
+      ExpectedSyncPositioningScheme::kInsertAfterItemId,
+      /*count=*/1);
+}
+
+// Tests that AdaptGuidForBookmark() propagates GUID in specifics if the field
+// is set (even if it doesn't match the originator client item ID).
+TEST(BookmarkUpdatePreprocessingTest, ShouldPropagateGuidFromSpecifics) {
+  const std::string kGuidInSpecifics = base::GenerateGUID();
+
+  sync_pb::SyncEntity entity;
+  entity.set_originator_cache_guid(base::GenerateGUID());
+  entity.set_originator_client_item_id(base::GenerateGUID());
+  entity.mutable_specifics()->mutable_bookmark()->set_guid(kGuidInSpecifics);
+
+  base::HistogramTester histogram_tester;
+  sync_pb::EntitySpecifics specifics = entity.specifics();
+  AdaptGuidForBookmark(entity, &specifics);
+
+  EXPECT_THAT(specifics.bookmark().guid(), Eq(kGuidInSpecifics));
+
+  histogram_tester.ExpectUniqueSample("Sync.BookmarkGUIDSource2",
+                                      /*sample=*/
+                                      ExpectedBookmarkGuidSource::kSpecifics,
+                                      /*count=*/1);
+}
+
+// Tests that AdaptGuidForBookmark() uses the originator client item ID as GUID
+// when it is a valid GUID, and the GUID in specifics is not set.
+TEST(BookmarkUpdatePreprocessingTest, ShouldUseOriginatorClientItemIdAsGuid) {
+  const std::string kOriginatorClientItemId = base::GenerateGUID();
+
+  sync_pb::SyncEntity entity;
+  entity.set_originator_cache_guid(base::GenerateGUID());
+  entity.set_originator_client_item_id(kOriginatorClientItemId);
+  entity.mutable_specifics()->mutable_bookmark();
+
+  base::HistogramTester histogram_tester;
+  sync_pb::EntitySpecifics specifics = entity.specifics();
+  AdaptGuidForBookmark(entity, &specifics);
+
+  EXPECT_THAT(specifics.bookmark().guid(), Eq(kOriginatorClientItemId));
+
+  histogram_tester.ExpectUniqueSample("Sync.BookmarkGUIDSource2",
+                                      /*sample=*/
+                                      ExpectedBookmarkGuidSource::kValidOCII,
+                                      /*count=*/1);
+}
+
+// Tests that AdaptGuidForBookmark() leaves the GUID in specifics empty when it
+// is originally empty and originator client item ID is not a valid GUID.
+TEST(BookmarkUpdatePreprocessingTest, ShouldLeaveEmptyGuid) {
+  const std::string kOriginatorClientItemId = "1";
+
+  sync_pb::SyncEntity entity;
+  entity.set_originator_cache_guid(base::GenerateGUID());
+  entity.set_originator_client_item_id(kOriginatorClientItemId);
+  entity.mutable_specifics()->mutable_bookmark();
+
+  base::HistogramTester histogram_tester;
+  sync_pb::EntitySpecifics specifics = entity.specifics();
+  AdaptGuidForBookmark(entity, &specifics);
+
+  EXPECT_THAT(specifics.bookmark().guid(), IsEmpty());
+
+  histogram_tester.ExpectUniqueSample("Sync.BookmarkGUIDSource2",
+                                      /*sample=*/
+                                      ExpectedBookmarkGuidSource::kLeftEmpty,
+                                      /*count=*/1);
+}
+
+}  // namespace
+
+}  // namespace syncer

components/sync/engine_impl/model_type_worker.cc

@@ -12,148 +12,25 @@
 
 #include "base/bind.h"
 #include "base/format_macros.h"
-#include "base/guid.h"
 #include "base/logging.h"
-#include "base/metrics/histogram_functions.h"
-#include "base/metrics/histogram_macros.h"
 #include "base/strings/stringprintf.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/trace_event/memory_usage_estimator.h"
 #include "components/sync/base/cancelation_signal.h"
 #include "components/sync/base/client_tag_hash.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/bookmark_update_preprocessing.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 syncer {
 
 namespace {
 
-// Enumeration of possible values for the positioning schemes used in Sync
-// entities. Used in UMA metrics. Do not re-order or delete these entries; they
-// are used in a UMA histogram. Please edit SyncPositioningScheme in enums.xml
-// if a value is added.
-enum class SyncPositioningScheme {
-  UNIQUE_POSITION = 0,
-  POSITION_IN_PARENT = 1,
-  INSERT_AFTER_ITEM_ID = 2,
-  MISSING = 3,
-  kMaxValue = MISSING
-};
-
-// Used in metrics: "Sync.BookmarkGUIDSource". These values are persisted to
-// logs. Entries should not be renumbered and numeric values should never be
-// reused.
-enum class BookmarkGUIDSource {
-  // GUID came from specifics.
-  kSpecifics = 0,
-  // GUID came from originator_client_item_id and is valid.
-  kValidOCII = 1,
-  // GUID not found in the specifics and originator_client_item_id is invalid,
-  // so field left empty.
-  kLeftEmpty = 2,
-
-  kMaxValue = kLeftEmpty,
-};
-
-inline void LogGUIDSource(BookmarkGUIDSource source) {
-  base::UmaHistogramEnumeration("Sync.BookmarkGUIDSource2", source);
-}
-
-void AdaptUniquePositionForBookmarks(const sync_pb::SyncEntity& update_entity,
-                                     syncer::EntityData* data) {
-  DCHECK(data);
-  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;
-    }
-
-    std::string suffix = missing_originator_fields
-                             ? UniquePosition::RandomSuffix()
-                             : GenerateSyncableBookmarkHash(
-                                   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::MISSING;
-  }
-  if (has_position_scheme) {
-    UMA_HISTOGRAM_ENUMERATION("Sync.Entities.PositioningScheme",
-                              sync_positioning_scheme);
-  }
-}
-
-void AdaptTitleForBookmarks(const sync_pb::SyncEntity& update_entity,
-                            sync_pb::EntitySpecifics* specifics,
-                            bool specifics_were_encrypted) {
-  DCHECK(specifics);
-  if (specifics_were_encrypted || update_entity.deleted()) {
-    // If encrypted, the name field is never populated (unencrypted) for privacy
-    // reasons. Encryption was also introduced after moving the name out of
-    // SyncEntity so this hack is not needed at all.
-    return;
-  }
-  // Legacy clients populate the name field in the SyncEntity instead of the
-  // title field in the BookmarkSpecifics.
-  if (!specifics->bookmark().has_title() && !update_entity.name().empty()) {
-    specifics->mutable_bookmark()->set_title(update_entity.name());
-  }
-}
-
-void AdaptGuidForBookmarks(const sync_pb::SyncEntity& update_entity,
-                           sync_pb::EntitySpecifics* specifics) {
-  DCHECK(specifics);
-  // Tombstones and permanent entities don't have a GUID.
-  if (update_entity.deleted() ||
-      !update_entity.server_defined_unique_tag().empty()) {
-    return;
-  }
-  // Legacy clients don't populate the guid field in the BookmarkSpecifics, so
-  // we use the originator_client_item_id instead, if it is a valid GUID.
-  // Otherwise, we leave the field empty.
-  if (specifics->bookmark().has_guid()) {
-    LogGUIDSource(BookmarkGUIDSource::kSpecifics);
-  } else if (base::IsValidGUID(update_entity.originator_client_item_id())) {
-    specifics->mutable_bookmark()->set_guid(
-        update_entity.originator_client_item_id());
-    LogGUIDSource(BookmarkGUIDSource::kValidOCII);
-  } else {
-    LogGUIDSource(BookmarkGUIDSource::kLeftEmpty);
-  }
-}
-
 void AdaptUpdateForCompatibilityAfterDecryption(
     ModelType model_type,
     const sync_pb::SyncEntity& update_entity,
@@ -161,8 +38,8 @@ void AdaptUpdateForCompatibilityAfterDecryption(
     bool specifics_were_encrypted) {
   DCHECK(specifics);
   if (model_type == BOOKMARKS) {
-    AdaptTitleForBookmarks(update_entity, specifics, specifics_were_encrypted);
-    AdaptGuidForBookmarks(update_entity, specifics);
+    AdaptTitleForBookmark(update_entity, specifics, specifics_were_encrypted);
+    AdaptGuidForBookmark(update_entity, specifics);
   }
 }
 
@@ -350,7 +227,7 @@ ModelTypeWorker::DecryptionStatus ModelTypeWorker::PopulateUpdateResponseData(
   // Adapt the update for compatibility (all except specifics that may need
   // encryption).
   if (model_type == BOOKMARKS) {
-    AdaptUniquePositionForBookmarks(update_entity, data.get());
+    AdaptUniquePositionForBookmark(update_entity, data.get());
   }
   // TODO(crbug.com/881289): Generate client tag hash for wallet data.
 
@@ -642,20 +519,11 @@ void ModelTypeWorker::DecryptStoredEntities() {
     decrypted_update->encryption_key_name = encryption_key_name;
     decrypted_update->entity = std::move(it->second->entity);
     decrypted_update->entity->specifics = std::move(specifics);
-    // TODO(crbug.com/1007199): Reconcile with AdaptGuidForBookmarks().
-    if (decrypted_update->entity->specifics.has_bookmark() &&
-        decrypted_update->entity->server_defined_unique_tag.empty()) {
-      if (decrypted_update->entity->specifics.bookmark().has_guid()) {
-        LogGUIDSource(BookmarkGUIDSource::kSpecifics);
-      } else if (base::IsValidGUID(
-                     decrypted_update->entity->originator_client_item_id)) {
-        decrypted_update->entity->specifics.mutable_bookmark()->set_guid(
-            decrypted_update->entity->originator_client_item_id);
-        LogGUIDSource(BookmarkGUIDSource::kValidOCII);
-      } else {
-        LogGUIDSource(BookmarkGUIDSource::kLeftEmpty);
-      }
+
+    if (decrypted_update->entity->specifics.has_bookmark()) {
+      AdaptGuidForBookmarkEntityData(decrypted_update->entity.get());
     }
+
     pending_updates_.push_back(std::move(decrypted_update));
     it = entries_pending_decryption_.erase(it);
   }