Small cleanup in SyncedBookmarkTracker

Introduces a GetMutableEntityForSyncId helper, the code for which was
duplicated many times before, and updates a few comments.

Bug: none
Change-Id: I775a5912990be1d80d7476ff2af8177b05928f14
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1866633Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#707302}

components/sync_bookmarks/synced_bookmark_tracker.cc

@@ -209,6 +209,12 @@ const SyncedBookmarkTracker::Entity* SyncedBookmarkTracker::GetEntityForSyncId(
   return it != sync_id_to_entities_map_.end() ? it->second.get() : nullptr;
 }
 
+SyncedBookmarkTracker::Entity* SyncedBookmarkTracker::GetMutableEntityForSyncId(
+    const std::string& sync_id) {
+  auto it = sync_id_to_entities_map_.find(sync_id);
+  return it != sync_id_to_entities_map_.end() ? it->second.get() : nullptr;
+}
+
 const SyncedBookmarkTracker::Entity*
 SyncedBookmarkTracker::GetEntityForBookmarkNode(
     const bookmarks::BookmarkNode* node) const {
@@ -248,9 +254,7 @@ void SyncedBookmarkTracker::Update(
     const sync_pb::UniquePosition& unique_position,
     const sync_pb::EntitySpecifics& specifics) {
   DCHECK_GT(specifics.ByteSize(), 0);
-  auto it = sync_id_to_entities_map_.find(sync_id);
-  DCHECK(it != sync_id_to_entities_map_.end());
-  Entity* entity = it->second.get();
+  Entity* entity = GetMutableEntityForSyncId(sync_id);
   DCHECK(entity);
   DCHECK_EQ(entity->metadata()->server_id(), sync_id);
   entity->metadata()->set_server_version(server_version);
@@ -264,25 +268,20 @@ void SyncedBookmarkTracker::Update(
 
 void SyncedBookmarkTracker::UpdateServerVersion(const std::string& sync_id,
                                                 int64_t server_version) {
-  auto it = sync_id_to_entities_map_.find(sync_id);
-  DCHECK(it != sync_id_to_entities_map_.end());
-  Entity* entity = it->second.get();
+  Entity* entity = GetMutableEntityForSyncId(sync_id);
   DCHECK(entity);
   entity->metadata()->set_server_version(server_version);
 }
 
 void SyncedBookmarkTracker::MarkCommitMayHaveStarted(
     const std::string& sync_id) {
-  auto it = sync_id_to_entities_map_.find(sync_id);
-  DCHECK(it != sync_id_to_entities_map_.end());
-  Entity* entity = it->second.get();
+  Entity* entity = GetMutableEntityForSyncId(sync_id);
   DCHECK(entity);
   entity->set_commit_may_have_started(true);
 }
 
 void SyncedBookmarkTracker::MarkDeleted(const std::string& sync_id) {
-  auto it = sync_id_to_entities_map_.find(sync_id);
-  Entity* entity = it->second.get();
+  Entity* entity = GetMutableEntityForSyncId(sync_id);
   DCHECK(entity);
   entity->metadata()->set_is_deleted(true);
   // Clear all references to the deleted bookmark node.
@@ -308,7 +307,7 @@ void SyncedBookmarkTracker::IncrementSequenceNumber(
   // TODO(crbug.com/516866): The below CHECK is added to debug some crashes.
   // Should be switched to a DCHECK after figuring out the reason for the crash.
   CHECK_NE(0U, sync_id_to_entities_map_.count(sync_id));
-  Entity* entity = sync_id_to_entities_map_.find(sync_id)->second.get();
+  Entity* entity = GetMutableEntityForSyncId(sync_id);
   DCHECK(entity);
   // TODO(crbug.com/516866): Update base hash specifics here if the entity is
   // not already out of sync.
@@ -472,9 +471,7 @@ void SyncedBookmarkTracker::UpdateUponCommitResponse(
     int64_t acked_sequence_number,
     int64_t server_version) {
   // TODO(crbug.com/516866): Update specifics if we decide to keep it.
-  auto it = sync_id_to_entities_map_.find(old_id);
-  Entity* entity =
-      it != sync_id_to_entities_map_.end() ? it->second.get() : nullptr;
+  Entity* entity = GetMutableEntityForSyncId(old_id);
   if (!entity) {
     DLOG(WARNING) << "Trying to update a non existing entity.";
     return;
@@ -516,15 +513,13 @@ void SyncedBookmarkTracker::UpdateBookmarkNodePointer(
     return;
   }
   bookmark_node_to_entities_map_[new_node] =
-      std::move(bookmark_node_to_entities_map_[old_node]);
+      bookmark_node_to_entities_map_[old_node];
   bookmark_node_to_entities_map_[new_node]->set_bookmark_node(new_node);
   bookmark_node_to_entities_map_.erase(old_node);
 }
 
 void SyncedBookmarkTracker::AckSequenceNumber(const std::string& sync_id) {
-  auto it = sync_id_to_entities_map_.find(sync_id);
-  Entity* entity =
-      it != sync_id_to_entities_map_.end() ? it->second.get() : nullptr;
+  Entity* entity = GetMutableEntityForSyncId(sync_id);
   DCHECK(entity);
   entity->metadata()->set_acked_sequence_number(
       entity->metadata()->sequence_number());

components/sync_bookmarks/synced_bookmark_tracker.h

@@ -31,10 +31,10 @@ namespace sync_bookmarks {
 using NodeMetadataPair = std::pair<const bookmarks::BookmarkNode*,
                                    std::unique_ptr<sync_pb::EntityMetadata>>;
 
-// This class is responsible for keeping the mapping between bookmarks node in
+// This class is responsible for keeping the mapping between bookmark nodes in
 // the local model and the server-side corresponding sync entities. It manages
-// the metadata for its entity and caches entity data upon a local change until
-// commit confirmation is received.
+// the metadata for its entities and caches entity data upon a local change
+// until commit confirmation is received.
 class SyncedBookmarkTracker {
  public:
   class Entity {
@@ -55,7 +55,7 @@ class SyncedBookmarkTracker {
     // Check whether |specifics| matches the stored specifics_hash.
     bool MatchesSpecificsHash(const sync_pb::EntitySpecifics& specifics) const;
 
-    // Returns null for tomstones.
+    // Returns null for tombstones.
     const bookmarks::BookmarkNode* bookmark_node() const {
       return bookmark_node_;
     }
@@ -163,7 +163,7 @@ class SyncedBookmarkTracker {
   void Remove(const std::string& sync_id);
 
   // Increment sequence number in the metadata for the entity with |sync_id|.
-  // Tracker must contain a non-tomstone entity with server id = |sync_id|.
+  // Tracker must contain a non-tombstone entity with server id = |sync_id|.
   void IncrementSequenceNumber(const std::string& sync_id);
 
   sync_pb::BookmarkModelMetadata BuildBookmarkModelMetadata() const;
@@ -232,6 +232,9 @@ class SyncedBookmarkTracker {
       const bookmarks::BookmarkModel* bookmark_model) const;
 
  private:
+  // Returns null if no entity is found.
+  Entity* GetMutableEntityForSyncId(const std::string& sync_id);
+
   // Reorders |entities| that represents local non-deletions such that parent
   // creation/update is before child creation/update. Returns the ordered list.
   std::vector<const Entity*> ReorderUnsyncedEntitiesExceptDeletions(