[Sync::USS] Refactor BookmarkModelTypeProcessor

This CL extracts the logics for handling remote updates and committing
local updates into BookmarkRemoteUpdatesHandler and
BookmarkLocalChangesBuilder respectively.

This is to simplfy the BookmarkModelTypeProcessor.

Before this CL: processor scheduled metadata save only if there is a
change in the metadata without change in the model, because the model
would trigger the save in such cases.

After this CL: processor schedules metadata save every time  remote
updates are processed regardless from they entailed a change in the
model or not. This simplifies the code, and the bookmark storage
should squash subsequent save requests, so consecutive save requests
from the model and the processor should result in only one actually
save on disk.


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

components/sync_bookmarks/BUILD.gn

@@ -10,12 +10,16 @@ static_library("sync_bookmarks") {
     "bookmark_change_processor.h",
     "bookmark_data_type_controller.cc",
     "bookmark_data_type_controller.h",
+    "bookmark_local_changes_builder.cc",
+    "bookmark_local_changes_builder.h",
     "bookmark_model_associator.cc",
     "bookmark_model_associator.h",
     "bookmark_model_observer_impl.cc",
     "bookmark_model_observer_impl.h",
     "bookmark_model_type_processor.cc",
     "bookmark_model_type_processor.h",
+    "bookmark_remote_updates_handler.cc",
+    "bookmark_remote_updates_handler.h",
     "bookmark_sync_service.cc",
     "bookmark_sync_service.h",
     "synced_bookmark_tracker.cc",
@@ -41,6 +45,7 @@ source_set("unit_tests") {
     "bookmark_data_type_controller_unittest.cc",
     "bookmark_model_observer_impl_unittest.cc",
     "bookmark_model_type_processor_unittest.cc",
+    "bookmark_remote_updates_handler_unittest.cc",
     "synced_bookmark_tracker_unittest.cc",
   ]
 

components/sync_bookmarks/bookmark_local_changes_builder.cc

+// Copyright 2018 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_bookmarks/bookmark_local_changes_builder.h"
+
+#include <string>
+#include <utility>
+
+#include "base/strings/utf_string_conversions.h"
+#include "components/bookmarks/browser/bookmark_node.h"
+#include "components/sync/base/time.h"
+#include "components/sync/protocol/bookmark_model_metadata.pb.h"
+#include "components/sync_bookmarks/synced_bookmark_tracker.h"
+
+namespace sync_bookmarks {
+
+namespace {
+
+sync_pb::EntitySpecifics SpecificsFromBookmarkNode(
+    const bookmarks::BookmarkNode* node) {
+  sync_pb::EntitySpecifics specifics;
+  sync_pb::BookmarkSpecifics* bm_specifics = specifics.mutable_bookmark();
+  bm_specifics->set_url(node->url().spec());
+  // TODO(crbug.com/516866): Set the favicon.
+  bm_specifics->set_title(base::UTF16ToUTF8(node->GetTitle()));
+  bm_specifics->set_creation_time_us(
+      node->date_added().ToDeltaSinceWindowsEpoch().InMicroseconds());
+
+  bm_specifics->set_icon_url(node->icon_url() ? node->icon_url()->spec()
+                                              : std::string());
+
+  for (const std::pair<std::string, std::string>& pair :
+       *node->GetMetaInfoMap()) {
+    sync_pb::MetaInfo* meta_info = bm_specifics->add_meta_info();
+    meta_info->set_key(pair.first);
+    meta_info->set_value(pair.second);
+  }
+  return specifics;
+}
+
+}  // namespace
+
+BookmarkLocalChangesBuilder::BookmarkLocalChangesBuilder(
+    const SyncedBookmarkTracker* const bookmark_tracker)
+    : bookmark_tracker_(bookmark_tracker) {
+  DCHECK(bookmark_tracker);
+}
+
+std::vector<syncer::CommitRequestData>
+BookmarkLocalChangesBuilder::BuildCommitRequests(size_t max_entries) const {
+  DCHECK(bookmark_tracker_);
+  const std::vector<const SyncedBookmarkTracker::Entity*>
+      entities_with_local_changes =
+          bookmark_tracker_->GetEntitiesWithLocalChanges(max_entries);
+  DCHECK_LE(entities_with_local_changes.size(), max_entries);
+
+  std::vector<syncer::CommitRequestData> commit_requests;
+  for (const SyncedBookmarkTracker::Entity* entity :
+       entities_with_local_changes) {
+    DCHECK(entity->IsUnsynced());
+    const sync_pb::EntityMetadata* metadata = entity->metadata();
+
+    syncer::CommitRequestData request;
+    syncer::EntityData data;
+    data.id = metadata->server_id();
+    data.creation_time = syncer::ProtoTimeToTime(metadata->creation_time());
+    data.modification_time =
+        syncer::ProtoTimeToTime(metadata->modification_time());
+    if (!metadata->is_deleted()) {
+      const bookmarks::BookmarkNode* node = entity->bookmark_node();
+      DCHECK(node);
+      const bookmarks::BookmarkNode* parent = node->parent();
+      const SyncedBookmarkTracker::Entity* parent_entity =
+          bookmark_tracker_->GetEntityForBookmarkNode(parent);
+      DCHECK(parent_entity);
+      data.parent_id = parent_entity->metadata()->server_id();
+      // TODO(crbug.com/516866): Double check that custom passphrase works well
+      // with this implementation, because:
+      // 1. NonBlockingTypeCommitContribution::AdjustCommitProto() clears the
+      //    title out.
+      // 2. Bookmarks (maybe ancient legacy bookmarks only?) use/used |name| to
+      //    encode the title.
+      data.is_folder = node->is_folder();
+      // TODO(crbug.com/516866): Set the non_unique_name similar to directory
+      // implementation.
+      // https://cs.chromium.org/chromium/src/components/sync/syncable/write_node.cc?l=41&rcl=1675007db1e0eb03417e81442688bb11cd181f58
+      data.non_unique_name = base::UTF16ToUTF8(node->GetTitle());
+      data.unique_position = parent_entity->metadata()->unique_position();
+      // In case of deletion, make an EntityData with empty specifics to
+      // indicate deletion.
+      data.specifics = SpecificsFromBookmarkNode(node);
+    }
+    request.entity = data.PassToPtr();
+    request.sequence_number = metadata->sequence_number();
+    request.base_version = metadata->server_version();
+    // Specifics hash has been computed in the tracker when this entity has been
+    // added/updated.
+    request.specifics_hash = metadata->specifics_hash();
+
+    commit_requests.push_back(std::move(request));
+  }
+  return commit_requests;
+}
+
+}  // namespace sync_bookmarks

components/sync_bookmarks/bookmark_local_changes_builder.h

+// Copyright 2018 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.
+
+#ifndef COMPONENTS_SYNC_BOOKMARKS_BOOKMARK_LOCAL_CHANGES_BUILDER_H_
+#define COMPONENTS_SYNC_BOOKMARKS_BOOKMARK_LOCAL_CHANGES_BUILDER_H_
+
+#include <vector>
+
+#include "components/sync/engine/non_blocking_sync_common.h"
+
+namespace sync_bookmarks {
+
+class SyncedBookmarkTracker;
+
+class BookmarkLocalChangesBuilder {
+ public:
+  // |bookmark_tracker| must not be null and must outlive this object.
+  explicit BookmarkLocalChangesBuilder(
+      const SyncedBookmarkTracker* bookmark_tracker);
+  // Builds the commit requests list.
+  std::vector<syncer::CommitRequestData> BuildCommitRequests(
+      size_t max_entries) const;
+
+ private:
+  const SyncedBookmarkTracker* const bookmark_tracker_;
+
+  DISALLOW_COPY_AND_ASSIGN(BookmarkLocalChangesBuilder);
+};
+
+}  // namespace sync_bookmarks
+
+#endif  // COMPONENTS_SYNC_BOOKMARKS_BOOKMARK_LOCAL_CHANGES_BUILDER_H_

components/sync_bookmarks/bookmark_model_type_processor.h

@@ -22,7 +22,6 @@ class BookmarkUndoService;
 
 namespace bookmarks {
 class BookmarkModel;
-class BookmarkNode;
 }
 
 namespace sync_bookmarks {
@@ -71,10 +70,6 @@ class BookmarkModelTypeProcessor : public syncer::ModelTypeProcessor,
                           const base::RepeatingClosure& schedule_save_closure,
                           bookmarks::BookmarkModel* model);
 
-  // Public for testing.
-  static std::vector<const syncer::UpdateResponseData*> ReorderUpdatesForTest(
-      const syncer::UpdateResponseDataList& updates);
-
   const SyncedBookmarkTracker* GetTrackerForTest() const;
 
   base::WeakPtr<syncer::ModelTypeControllerDelegate> GetWeakPtr();
@@ -82,51 +77,6 @@ class BookmarkModelTypeProcessor : public syncer::ModelTypeProcessor,
  private:
   SEQUENCE_CHECKER(sequence_checker_);
 
-  // Reorders incoming updates such that parent creation is before child
-  // creation and child deletion is before parent deletion, and deletions should
-  // come last. The returned pointers point to the elements in the original
-  // |updates|.
-  static std::vector<const syncer::UpdateResponseData*> ReorderUpdates(
-      const syncer::UpdateResponseDataList& updates);
-
-  // Given a remote update entity, it returns the parent bookmark node of the
-  // corresponding node. It returns null if the parent node cannot be found.
-  const bookmarks::BookmarkNode* GetParentNode(
-      const syncer::EntityData& update_entity) const;
-
-  // Processes a remote creation of a bookmark node.
-  // 1. For permanent folders, they are only registered in |bookmark_tracker_|.
-  // 2. If the nodes parent cannot be found, the remote creation update is
-  //    ignored.
-  // 3. Otherwise, a new node is created in the local bookmark model and
-  //    registered in |bookmark_tracker_|.
-  void ProcessRemoteCreate(const syncer::UpdateResponseData& update);
-
-  // Processes a remote update of a bookmark node. |update| must not be a
-  // deletion, and the server_id must be already tracked, otherwise, it is a
-  // creation that gets handeled in ProcessRemoteCreate(). |tracked_entity| is
-  // the tracked entity for that server_id. It is passed as a dependency instead
-  // of performing a lookup inside ProcessRemoteUpdate() to avoid wasting CPU
-  // cycles for doing another lookup (this code runs on the UI thread).
-  void ProcessRemoteUpdate(const syncer::UpdateResponseData& update,
-                           const SyncedBookmarkTracker::Entity* tracked_entity);
-
-  // Process a remote delete of a bookmark node. |update_entity| must not be a
-  // deletion. |tracked_entity| is the tracked entity for that server_id. It is
-  // passed as a dependency instead of performing a lookup inside
-  // ProcessRemoteDelete() to avoid wasting CPU cycles for doing another lookup
-  // (this code runs on the UI thread).
-  void ProcessRemoteDelete(const syncer::EntityData& update_entity,
-                           const SyncedBookmarkTracker::Entity* tracked_entity);
-
-  // Associates the permanent bookmark folders with the corresponding server
-  // side ids and registers the association in |bookmark_tracker_|.
-  // |update_entity| must contain server_defined_unique_tag that is used to
-  // determine the corresponding permanent node. All permanent nodes are assumed
-  // to be directly children nodes of |kBookmarksRootId|. This method is used in
-  // the initial sync cycle only.
-  void AssociatePermanentFolder(const syncer::UpdateResponseData& update);
-
   // If preconditions are met, inform sync that we are ready to connect.
   void ConnectIfReady();
 
@@ -135,10 +85,6 @@ class BookmarkModelTypeProcessor : public syncer::ModelTypeProcessor,
   // entities.
   void NudgeForCommitIfNeeded();
 
-  // Builds the commit requests list.
-  std::vector<syncer::CommitRequestData> BuildCommitRequestsForLocalChanges(
-      size_t max_entries);
-
   // Instantiates the required objects to track metadata and starts observing
   // changes from the bookmark model.
   void StartTrackingMetadata(

components/sync_bookmarks/bookmark_model_type_processor_unittest.cc

@@ -25,9 +25,6 @@ namespace sync_bookmarks {
 
 namespace {
 
-// The parent tag for children of the root entity. Entities with this parent are
-// referred to as top level enities.
-const char kRootParentTag[] = "0";
 const char kBookmarkBarTag[] = "bookmark_bar";
 const char kBookmarkBarId[] = "bookmark_bar_id";
 const char kBookmarksRootId[] = "32904_google_chrome_bookmarks";
@@ -119,12 +116,6 @@ syncer::UpdateResponseData CreateTombstone(const std::string& server_id) {
   return response_data;
 }
 
-syncer::UpdateResponseData CreateBookmarkRootUpdateData() {
-  return CreateUpdateData({syncer::ModelTypeToRootTag(syncer::BOOKMARKS),
-                           std::string(), std::string(), kRootParentTag,
-                           syncer::ModelTypeToRootTag(syncer::BOOKMARKS)});
-}
-
 class TestSyncClient : public syncer::FakeSyncClient {
  public:
   explicit TestSyncClient(bookmarks::BookmarkModel* bookmark_model)
@@ -169,38 +160,6 @@ class BookmarkModelTypeProcessorTest : public testing::Test {
   BookmarkModelTypeProcessor processor_;
 };
 
-TEST(BookmarkModelTypeProcessorReorderUpdatesTest, ShouldIgnoreRootNodes) {
-  syncer::UpdateResponseDataList updates;
-  updates.push_back(CreateBookmarkRootUpdateData());
-  std::vector<const syncer::UpdateResponseData*> ordered_updates =
-      BookmarkModelTypeProcessor::ReorderUpdatesForTest(updates);
-  // Root node update should be filtered out.
-  EXPECT_THAT(ordered_updates.size(), Eq(0U));
-}
-
-// TODO(crbug.com/516866): This should change to cover the general case of
-// parents before children for non-deletions, and another test should be added
-// for children before parents for deletions.
-TEST(BookmarkModelTypeProcessorReorderUpdatesTest,
-     ShouldPlacePermanentNodesFirstForNonDeletions) {
-  const std::string kNode1Id = "node1";
-  const std::string kNode2Id = "node2";
-  syncer::UpdateResponseDataList updates;
-  updates.push_back(CreateUpdateData(
-      {kNode1Id, std::string(), std::string(), kNode2Id, std::string()}));
-  updates.push_back(CreateUpdateData({kNode2Id, std::string(), std::string(),
-                                      kBookmarksRootId, kBookmarkBarTag}));
-  std::vector<const syncer::UpdateResponseData*> ordered_updates =
-      BookmarkModelTypeProcessor::ReorderUpdatesForTest(updates);
-
-  // No update should be dropped.
-  ASSERT_THAT(ordered_updates.size(), Eq(2U));
-
-  // Updates should be ordered such that parent node update comes first.
-  EXPECT_THAT(ordered_updates[0]->entity.value().id, Eq(kNode2Id));
-  EXPECT_THAT(ordered_updates[1]->entity.value().id, Eq(kNode1Id));
-}
-
 TEST_F(BookmarkModelTypeProcessorTest, ShouldUpdateModelAfterRemoteCreation) {
   syncer::UpdateResponseDataList updates;
   // Add update for the permanent folder "Bookmarks bar".
@@ -219,9 +178,6 @@ TEST_F(BookmarkModelTypeProcessorTest, ShouldUpdateModelAfterRemoteCreation) {
       bookmark_model()->bookmark_bar_node();
   EXPECT_TRUE(bookmarkbar->empty());
 
-  // Save will be scheduled in the model upon model change. No save should be
-  // scheduled from the processor.
-  EXPECT_CALL(*schedule_save_closure(), Run()).Times(0);
   processor()->OnUpdateReceived(CreateDummyModelTypeState(), updates);
 
   ASSERT_THAT(bookmarkbar->GetChild(0), NotNull());
@@ -255,9 +211,6 @@ TEST_F(BookmarkModelTypeProcessorTest, ShouldUpdateModelAfterRemoteUpdate) {
       CreateUpdateData({kNodeId, kNewTitle, kNewUrl, kBookmarkBarId,
                         /*server_tag=*/std::string()}));
 
-  // Save will be scheduled in the model upon model change. No save should be
-  // scheduled from the processor.
-  EXPECT_CALL(*schedule_save_closure(), Run()).Times(0);
   processor()->OnUpdateReceived(CreateDummyModelTypeState(), updates);
 
   // Check if the bookmark has been updated properly.
@@ -347,9 +300,6 @@ TEST_F(BookmarkModelTypeProcessorTest, ShouldUpdateModelAfterRemoteDelete) {
   updates.push_back(CreateTombstone(kTitle1Id));
   updates.push_back(CreateTombstone(kFolder1Id));
 
-  // Save will be scheduled in the model upon model change. No save should be
-  // scheduled from the processor.
-  EXPECT_CALL(*schedule_save_closure(), Run()).Times(0);
   processor()->OnUpdateReceived(CreateDummyModelTypeState(), updates);
 
   // The structure should be

components/sync_bookmarks/bookmark_remote_updates_handler.h

+// Copyright 2018 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.
+
+#ifndef COMPONENTS_SYNC_BOOKMARKS_BOOKMARK_REMOTE_UPDATES_HANDLER_H_
+#define COMPONENTS_SYNC_BOOKMARKS_BOOKMARK_REMOTE_UPDATES_HANDLER_H_
+
+#include <vector>
+
+#include "components/sync/engine/non_blocking_sync_common.h"
+#include "components/sync_bookmarks/synced_bookmark_tracker.h"
+
+namespace bookmarks {
+class BookmarkModel;
+class BookmarkNode;
+}  // namespace bookmarks
+
+namespace sync_bookmarks {
+
+// Responsible for processing remote updates received from the sync server.
+class BookmarkRemoteUpdatesHandler {
+ public:
+  // |bookmark_model| and |bookmark_tracker| must not be null and most outlive
+  // this object.
+  BookmarkRemoteUpdatesHandler(bookmarks::BookmarkModel* bookmark_model,
+                               SyncedBookmarkTracker* bookmark_tracker);
+  // Processes the updates received from the sync server in |updates| and
+  // updates the |bookmark_model_| and |bookmark_tracker_| accordingly.
+  void Process(const syncer::UpdateResponseDataList& updates);
+
+  // Public for testing.
+  static std::vector<const syncer::UpdateResponseData*> ReorderUpdatesForTest(
+      const syncer::UpdateResponseDataList& updates);
+
+ private:
+  // Reorders incoming updates such that parent creation is before child
+  // creation and child deletion is before parent deletion, and deletions should
+  // come last. The returned pointers point to the elements in the original
+  // |updates|.
+  static std::vector<const syncer::UpdateResponseData*> ReorderUpdates(
+      const syncer::UpdateResponseDataList& updates);
+
+  // Given a remote update entity, it returns the parent bookmark node of the
+  // corresponding node. It returns null if the parent node cannot be found.
+  const bookmarks::BookmarkNode* GetParentNode(
+      const syncer::EntityData& update_entity) const;
+
+  // Processes a remote creation of a bookmark node.
+  // 1. For permanent folders, they are only registered in |bookmark_tracker_|.
+  // 2. If the nodes parent cannot be found, the remote creation update is
+  //    ignored.
+  // 3. Otherwise, a new node is created in the local bookmark model and
+  //    registered in |bookmark_tracker_|.
+  void ProcessRemoteCreate(const syncer::UpdateResponseData& update);
+
+  // Processes a remote update of a bookmark node. |update| must not be a
+  // deletion, and the server_id must be already tracked, otherwise, it is a
+  // creation that gets handeled in ProcessRemoteCreate(). |tracked_entity| is
+  // the tracked entity for that server_id. It is passed as a dependency instead
+  // of performing a lookup inside ProcessRemoteUpdate() to avoid wasting CPU
+  // cycles for doing another lookup (this code runs on the UI thread).
+  void ProcessRemoteUpdate(const syncer::UpdateResponseData& update,
+                           const SyncedBookmarkTracker::Entity* tracked_entity);
+
+  // Process a remote delete of a bookmark node. |update_entity| must not be a
+  // deletion. |tracked_entity| is the tracked entity for that server_id. It is
+  // passed as a dependency instead of performing a lookup inside
+  // ProcessRemoteDelete() to avoid wasting CPU cycles for doing another lookup
+  // (this code runs on the UI thread).
+  void ProcessRemoteDelete(const syncer::EntityData& update_entity,
+                           const SyncedBookmarkTracker::Entity* tracked_entity);
+
+  // Associates the permanent bookmark folders with the corresponding server
+  // side ids and registers the association in |bookmark_tracker_|.
+  // |update_entity| must contain server_defined_unique_tag that is used to
+  // determine the corresponding permanent node. All permanent nodes are assumed
+  // to be directly children nodes of |kBookmarksRootId|. This method is used in
+  // the initial sync cycle only.
+  void AssociatePermanentFolder(const syncer::UpdateResponseData& update);
+
+  bookmarks::BookmarkModel* const bookmark_model_;
+  SyncedBookmarkTracker* const bookmark_tracker_;
+
+  DISALLOW_COPY_AND_ASSIGN(BookmarkRemoteUpdatesHandler);
+};
+
+}  // namespace sync_bookmarks
+
+#endif  // COMPONENTS_SYNC_BOOKMARKS_BOOKMARK_REMOTE_UPDATES_HANDLER_H_

components/sync_bookmarks/bookmark_remote_updates_handler_unittest.cc

+// Copyright 2018 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_bookmarks/bookmark_remote_updates_handler.h"
+
+#include <string>
+
+#include "components/sync/base/model_type.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using testing::Eq;
+
+namespace sync_bookmarks {
+
+namespace {
+
+// The parent tag for children of the root entity. Entities with this parent are
+// referred to as top level enities.
+const char kRootParentTag[] = "0";
+const char kBookmarkBarTag[] = "bookmark_bar";
+const char kBookmarksRootId[] = "32904_google_chrome_bookmarks";
+
+struct BookmarkInfo {
+  std::string server_id;
+  std::string title;
+  std::string url;  // empty for folders.
+  std::string parent_id;
+  std::string server_tag;
+};
+
+syncer::UpdateResponseData CreateUpdateData(const BookmarkInfo& bookmark_info) {
+  syncer::EntityData data;
+  data.id = bookmark_info.server_id;
+  data.parent_id = bookmark_info.parent_id;
+  data.server_defined_unique_tag = bookmark_info.server_tag;
+
+  sync_pb::BookmarkSpecifics* bookmark_specifics =
+      data.specifics.mutable_bookmark();
+  bookmark_specifics->set_title(bookmark_info.title);
+  if (bookmark_info.url.empty()) {
+    data.is_folder = true;
+  } else {
+    bookmark_specifics->set_url(bookmark_info.url);
+  }
+
+  syncer::UpdateResponseData response_data;
+  response_data.entity = data.PassToPtr();
+  // Similar to what's done in the loopback_server.
+  response_data.response_version = 0;
+  return response_data;
+}
+
+syncer::UpdateResponseData CreateBookmarkRootUpdateData() {
+  return CreateUpdateData({syncer::ModelTypeToRootTag(syncer::BOOKMARKS),
+                           std::string(), std::string(), kRootParentTag,
+                           syncer::ModelTypeToRootTag(syncer::BOOKMARKS)});
+}
+
+TEST(BookmarkRemoteUpdatesHandlerReorderUpdatesTest, ShouldIgnoreRootNodes) {
+  syncer::UpdateResponseDataList updates;
+  updates.push_back(CreateBookmarkRootUpdateData());
+  std::vector<const syncer::UpdateResponseData*> ordered_updates =
+      BookmarkRemoteUpdatesHandler::ReorderUpdatesForTest(updates);
+  // Root node update should be filtered out.
+  EXPECT_THAT(ordered_updates.size(), Eq(0U));
+}
+
+// TODO(crbug.com/516866): This should change to cover the general case of
+// parents before children for non-deletions, and another test should be added
+// for children before parents for deletions.
+TEST(BookmarkRemoteUpdatesHandlerReorderUpdatesTest,
+     ShouldPlacePermanentNodesFirstForNonDeletions) {
+  const std::string kNode1Id = "node1";
+  const std::string kNode2Id = "node2";
+  syncer::UpdateResponseDataList updates;
+  updates.push_back(CreateUpdateData(
+      {kNode1Id, std::string(), std::string(), kNode2Id, std::string()}));
+  updates.push_back(CreateUpdateData({kNode2Id, std::string(), std::string(),
+                                      kBookmarksRootId, kBookmarkBarTag}));
+  std::vector<const syncer::UpdateResponseData*> ordered_updates =
+      BookmarkRemoteUpdatesHandler::ReorderUpdatesForTest(updates);
+
+  // No update should be dropped.
+  ASSERT_THAT(ordered_updates.size(), Eq(2U));
+
+  // Updates should be ordered such that parent node update comes first.
+  EXPECT_THAT(ordered_updates[0]->entity.value().id, Eq(kNode2Id));
+  EXPECT_THAT(ordered_updates[1]->entity.value().id, Eq(kNode1Id));
+}
+
+}  // namespace
+
+}  // namespace sync_bookmarks