[Sync::USS] Commit Local Bookmark Creations

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

chrome/browser/sync/test/integration/single_client_bookmarks_sync_test.cc

@@ -244,6 +244,48 @@ IN_PROC_BROWSER_TEST_F(SingleClientBookmarksSyncTest, Sanity) {
     VerifyBookmarkModelMatchesFakeServer(kSingleProfileIndex);
 }
 
+IN_PROC_BROWSER_TEST_P(SingleClientBookmarksSyncTestIncludingUssTests,
+                       CommitLocalCreations) {
+  ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
+
+  // Starting state:
+  // other_node
+  //    -> top
+  //      -> tier1_a
+  //        -> http://mail.google.com  "tier1_a_url0"
+  //        -> http://www.pandora.com  "tier1_a_url1"
+  //        -> http://www.facebook.com "tier1_a_url2"
+  //      -> tier1_b
+  //        -> http://www.nhl.com "tier1_b_url0"
+  const BookmarkNode* top = AddFolder(
+      kSingleProfileIndex, GetOtherNode(kSingleProfileIndex), 0, "top");
+  const BookmarkNode* tier1_a =
+      AddFolder(kSingleProfileIndex, top, 0, "tier1_a");
+  const BookmarkNode* tier1_b =
+      AddFolder(kSingleProfileIndex, top, 1, "tier1_b");
+  const BookmarkNode* tier1_a_url0 =
+      AddURL(kSingleProfileIndex, tier1_a, 0, "tier1_a_url0",
+             GURL("http://mail.google.com"));
+  const BookmarkNode* tier1_a_url1 =
+      AddURL(kSingleProfileIndex, tier1_a, 1, "tier1_a_url1",
+             GURL("http://www.pandora.com"));
+  const BookmarkNode* tier1_a_url2 =
+      AddURL(kSingleProfileIndex, tier1_a, 2, "tier1_a_url2",
+             GURL("http://www.facebook.com"));
+  const BookmarkNode* tier1_b_url0 =
+      AddURL(kSingleProfileIndex, tier1_b, 0, "tier1_b_url0",
+             GURL("http://www.nhl.com"));
+  EXPECT_TRUE(tier1_a_url0);
+  EXPECT_TRUE(tier1_a_url1);
+  EXPECT_TRUE(tier1_a_url2);
+  EXPECT_TRUE(tier1_b_url0);
+  // Setup sync, wait for its completion, and make sure changes were synced.
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  ASSERT_TRUE(
+      UpdatedProgressMarkerChecker(GetSyncService(kSingleProfileIndex)).Wait());
+  EXPECT_TRUE(ModelMatchesVerifier(kSingleProfileIndex));
+}
+
 IN_PROC_BROWSER_TEST_P(SingleClientBookmarksSyncTestIncludingUssTests,
                        InjectedBookmark) {
   std::string title = "Montreal Canadiens";

components/sync/engine/non_blocking_sync_common.h

@@ -23,13 +23,17 @@ struct CommitRequestData {
   CommitRequestData(const CommitRequestData& other);
   ~CommitRequestData();
 
+  // Fields sent to the sync server.
   EntityDataPtr entity;
+  int64_t base_version = 0;
+
+  // Fields not sent to the sync server. However, they are kept to be sent back
+  // to the processor in the response.
 
-  // Strictly incrementing number for in-progress commits.  More information
-  // about its meaning can be found in comments in the files that make use of
-  // this struct.
+  // Strictly incrementing number for in-progress commits.
+  // More information about its meaning can be found in comments in the files
+  // that make use of this struct.
   int64_t sequence_number = 0;
-  int64_t base_version = 0;
   std::string specifics_hash;
 };
 
@@ -39,6 +43,10 @@ struct CommitResponseData {
   ~CommitResponseData();
 
   std::string id;
+  // The sync id that was sent in the request. Non-empty only if different from
+  // |id|. It could be different because the server can change the sync id
+  // (e.g. for newly created bookmarks),
+  std::string id_in_request;
   std::string client_tag_hash;
   int64_t sequence_number = 0;
   int64_t response_version = 0;

components/sync/engine_impl/non_blocking_type_commit_contribution.cc

@@ -97,6 +97,12 @@ SyncerError NonBlockingTypeCommitContribution::ProcessCommitResponse(
         CommitResponseData response_data;
         const CommitRequestData& commit_request = commit_requests_[i];
         response_data.id = entry_response.id_string();
+        if (response_data.id != commit_request.entity->id) {
+          // Server has changed the sync id in the request. Write back the
+          // original sync id. This is useful for data types without a notion of
+          // a client tag such as bookmarks.
+          response_data.id_in_request = commit_request.entity->id;
+        }
         response_data.response_version = entry_response.version();
         response_data.client_tag_hash = commit_request.entity->client_tag_hash;
         response_data.sequence_number = commit_request.sequence_number;
@@ -190,20 +196,17 @@ void NonBlockingTypeCommitContribution::PopulateCommitProto(
 
 void NonBlockingTypeCommitContribution::AdjustCommitProto(
     sync_pb::SyncEntity* commit_proto) {
-  // Initial commits need our help to generate a client ID.
   if (commit_proto->version() == kUncommittedVersion) {
-    DCHECK(commit_proto->id_string().empty()) << commit_proto->id_string();
-    // TODO(crbug.com/516866): This is incorrect for bookmarks for two reasons:
-    // 1) Won't be able to match previously committed bookmarks to the ones
-    //    with server ID.
-    // 2) Recommitting an item in a case of failing to receive commit response
-    //    would result in generating a different client ID, which in turn
-    //    would result in a duplication.
-    // We should generate client ID on the frontend side instead.
-    commit_proto->set_id_string(base::GenerateGUID());
     commit_proto->set_version(0);
-  } else {
-    DCHECK(!commit_proto->id_string().empty());
+    // Initial commits need our help to generate a client ID if they don't have
+    // any. Bookmarks create their own IDs on the frontend side to be able to
+    // match them after commits. For other data types we generate one here. And
+    // since bookmarks don't have client tags, their server id should be stable
+    // across restarts in case of recommitting an item, it doesn't result in
+    // creating a duplicate.
+    if (commit_proto->id_string().empty()) {
+      commit_proto->set_id_string(base::GenerateGUID());
+    }
   }
 
   // Encrypt the specifics and hide the title if necessary.

components/sync/protocol/entity_metadata.proto

@@ -43,9 +43,9 @@ message EntityMetadata {
   // those changes are based.
   optional int64 server_version = 6 [default = -1];
 
-  // Entity creation and modification timestamps.
-  // Assigned by the client and synced by the server, though the server usually
-  // doesn't bother to inspect their values.
+  // Entity creation and modification timestamps. Assigned by the client and
+  // synced by the server, though the server usually doesn't bother to inspect
+  // their values. They are encoded as milliseconds since the Unix epoch.
   optional int64 creation_time = 7;
   optional int64 modification_time = 8;
 

components/sync_bookmarks/BUILD.gn

@@ -39,6 +39,7 @@ source_set("unit_tests") {
 
   sources = [
     "bookmark_data_type_controller_unittest.cc",
+    "bookmark_model_observer_impl_unittest.cc",
     "bookmark_model_type_processor_unittest.cc",
     "synced_bookmark_tracker_unittest.cc",
   ]

components/sync_bookmarks/bookmark_model_observer_impl.cc

@@ -4,6 +4,17 @@
 
 #include "components/sync_bookmarks/bookmark_model_observer_impl.h"
 
+#include <utility>
+
+#include "base/guid.h"
+#include "base/strings/utf_string_conversions.h"
+#include "components/bookmarks/browser/bookmark_model.h"
+#include "components/bookmarks/browser/bookmark_node.h"
+#include "components/sync/base/hash_util.h"
+#include "components/sync/base/unique_position.h"
+#include "components/sync/engine/non_blocking_sync_common.h"
+#include "components/sync_bookmarks/synced_bookmark_tracker.h"
+
 namespace sync_bookmarks {
 
 BookmarkModelObserverImpl::BookmarkModelObserverImpl(
@@ -40,7 +51,104 @@ void BookmarkModelObserverImpl::BookmarkNodeAdded(
     bookmarks::BookmarkModel* model,
     const bookmarks::BookmarkNode* parent,
     int index) {
-  NOTIMPLEMENTED();
+  const bookmarks::BookmarkNode* node = parent->GetChild(index);
+  // TODO(crbug.com/516866): continue only if
+  // model->client()->CanSyncNode(node).
+
+  const SyncedBookmarkTracker::Entity* parent_entity =
+      bookmark_tracker_->GetEntityForBookmarkNode(parent);
+  if (!parent_entity) {
+    DLOG(WARNING) << "Bookmark parent lookup failed";
+    return;
+  }
+  // Similar to the diectory implementation here:
+  // https://cs.chromium.org/chromium/src/components/sync/syncable/mutable_entry.cc?l=237&gsn=CreateEntryKernel
+  // Assign a temp server id for the entity. Will be overriden by the actual
+  // server id upon receiving commit response.
+  const std::string sync_id = base::GenerateGUID();
+  const int64_t server_version = syncer::kUncommittedVersion;
+  const base::Time creation_time = base::Time::Now();
+  const sync_pb::UniquePosition unique_position =
+      ComputePosition(*parent, index, sync_id).ToProto();
+
+  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());
+  // TODO(crbug.com/516866): update the implementation to be similar to the
+  // directory implementation
+  // https://cs.chromium.org/chromium/src/components/sync_bookmarks/bookmark_change_processor.cc?l=882&rcl=f38001d936d8b2abb5743e85cbc88c72746ae3d2
+  if (node->GetMetaInfoMap()) {
+    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);
+    }
+  }
+  bookmark_tracker_->Add(sync_id, node, server_version, creation_time,
+                         unique_position, specifics);
+  // Mark the entity that it needs to be committed.
+  bookmark_tracker_->IncrementSequenceNumber(sync_id);
+  nudge_for_commit_closure_.Run();
+}
+
+syncer::UniquePosition BookmarkModelObserverImpl::ComputePosition(
+    const bookmarks::BookmarkNode& parent,
+    int index,
+    const std::string& sync_id) {
+  const std::string& suffix = syncer::GenerateSyncableBookmarkHash(
+      bookmark_tracker_->model_type_state().cache_guid(), sync_id);
+  DCHECK_NE(0, parent.child_count());
+
+  if (parent.child_count() == 1) {
+    // No siblings, the parent has no other children.
+    return syncer::UniquePosition::InitialPosition(suffix);
+  }
+  if (index == 0) {
+    const bookmarks::BookmarkNode* successor_node = parent.GetChild(1);
+    const SyncedBookmarkTracker::Entity* successor_entity =
+        bookmark_tracker_->GetEntityForBookmarkNode(successor_node);
+    DCHECK(successor_entity);
+    // Insert at the beginning.
+    return syncer::UniquePosition::Before(
+        syncer::UniquePosition::FromProto(
+            successor_entity->metadata()->unique_position()),
+        suffix);
+  }
+  if (index == parent.child_count() - 1) {
+    // Insert at the end.
+    const bookmarks::BookmarkNode* predecessor_node =
+        parent.GetChild(index - 1);
+    const SyncedBookmarkTracker::Entity* predecessor_entity =
+        bookmark_tracker_->GetEntityForBookmarkNode(predecessor_node);
+    DCHECK(predecessor_entity);
+    return syncer::UniquePosition::After(
+        syncer::UniquePosition::FromProto(
+            predecessor_entity->metadata()->unique_position()),
+        suffix);
+  }
+  // Insert in the middle.
+  const bookmarks::BookmarkNode* successor_node = parent.GetChild(index + 1);
+  const SyncedBookmarkTracker::Entity* successor_entity =
+      bookmark_tracker_->GetEntityForBookmarkNode(successor_node);
+  DCHECK(successor_entity);
+  const bookmarks::BookmarkNode* predecessor_node = parent.GetChild(index - 1);
+  const SyncedBookmarkTracker::Entity* predecessor_entity =
+      bookmark_tracker_->GetEntityForBookmarkNode(predecessor_node);
+  DCHECK(predecessor_entity);
+  return syncer::UniquePosition::Between(
+      syncer::UniquePosition::FromProto(
+          predecessor_entity->metadata()->unique_position()),
+      syncer::UniquePosition::FromProto(
+          successor_entity->metadata()->unique_position()),
+      suffix);
 }
 
 void BookmarkModelObserverImpl::BookmarkNodeRemoved(

components/sync_bookmarks/bookmark_model_observer_impl.h

@@ -6,11 +6,16 @@
 #define COMPONENTS_SYNC_BOOKMARKS_BOOKMARK_MODEL_OBSERVER_IMPL_H_
 
 #include <set>
+#include <string>
 
 #include "base/callback.h"
 #include "components/bookmarks/browser/bookmark_model_observer.h"
 #include "url/gurl.h"
 
+namespace syncer {
+class UniquePosition;
+}
+
 namespace sync_bookmarks {
 
 class SyncedBookmarkTracker;
@@ -54,6 +59,10 @@ class BookmarkModelObserverImpl : public bookmarks::BookmarkModelObserver {
       const bookmarks::BookmarkNode* node) override;
 
  private:
+  syncer::UniquePosition ComputePosition(const bookmarks::BookmarkNode& parent,
+                                         int index,
+                                         const std::string& sync_id);
+
   // Points to the tracker owned by the processor. It keeps the mapping between
   // bookmark nodes and corresponding sync server entities.
   SyncedBookmarkTracker* const bookmark_tracker_;

components/sync_bookmarks/bookmark_model_observer_impl_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_model_observer_impl.h"
+
+#include <memory>
+#include <vector>
+
+#include "base/strings/utf_string_conversions.h"
+#include "base/test/mock_callback.h"
+#include "components/bookmarks/browser/bookmark_model.h"
+#include "components/bookmarks/test/test_bookmark_client.h"
+#include "components/sync/base/time.h"
+#include "components/sync/base/unique_position.h"
+#include "components/sync_bookmarks/synced_bookmark_tracker.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace sync_bookmarks {
+
+namespace {
+
+using testing::Eq;
+using testing::NiceMock;
+using testing::NotNull;
+
+const char kBookmarkBarId[] = "bookmark_bar_id";
+const char kBookmarkBarTag[] = "bookmark_bar";
+
+class BookmarkModelObserverImplTest : public testing::Test {
+ public:
+  BookmarkModelObserverImplTest()
+      : bookmark_model_(bookmarks::TestBookmarkClient::CreateModel()),
+        bookmark_tracker_(std::vector<NodeMetadataPair>(),
+                          std::make_unique<sync_pb::ModelTypeState>()),
+        observer_(nudge_for_commit_closure_.Get(), &bookmark_tracker_) {
+    bookmark_model_->AddObserver(&observer_);
+    sync_pb::EntitySpecifics specifics;
+    specifics.mutable_bookmark()->set_title(kBookmarkBarTag);
+    bookmark_tracker_.Add(
+        /*sync_id=*/kBookmarkBarId,
+        /*bookmark_node=*/bookmark_model()->bookmark_bar_node(),
+        /*server_version=*/0, /*creation_time=*/base::Time::Now(),
+        syncer::UniquePosition::InitialPosition(
+            syncer::UniquePosition::RandomSuffix())
+            .ToProto(),
+        specifics);
+  }
+
+  bookmarks::BookmarkModel* bookmark_model() { return bookmark_model_.get(); }
+  SyncedBookmarkTracker* bookmark_tracker() { return &bookmark_tracker_; }
+  BookmarkModelObserverImpl* observer() { return &observer_; }
+  base::MockCallback<base::RepeatingClosure>* nudge_for_commit_closure() {
+    return &nudge_for_commit_closure_;
+  }
+
+ private:
+  std::unique_ptr<bookmarks::BookmarkModel> bookmark_model_;
+  NiceMock<base::MockCallback<base::RepeatingClosure>>
+      nudge_for_commit_closure_;
+  SyncedBookmarkTracker bookmark_tracker_;
+  BookmarkModelObserverImpl observer_;
+};
+
+TEST_F(BookmarkModelObserverImplTest,
+       BookmarkAddedShouldPutInTheTrackerAndNudgeForCommit) {
+  const std::string kTitle = "title";
+  const std::string kUrl = "http://www.url.com";
+  const size_t kMaxEntries = 10;
+
+  EXPECT_CALL(*nudge_for_commit_closure(), Run());
+  const bookmarks::BookmarkNode* bookmark_bar_node =
+      bookmark_model()->bookmark_bar_node();
+  const bookmarks::BookmarkNode* bookmark_node = bookmark_model()->AddURL(
+      /*parent=*/bookmark_bar_node, /*index=*/0, base::UTF8ToUTF16(kTitle),
+      GURL(kUrl));
+
+  EXPECT_THAT(bookmark_tracker()->TrackedEntitiesCountForTest(), 2U);
+
+  std::vector<const SyncedBookmarkTracker::Entity*> local_changes =
+      bookmark_tracker()->GetEntitiesWithLocalChanges(kMaxEntries);
+  ASSERT_THAT(local_changes.size(), 1U);
+  EXPECT_THAT(local_changes[0]->bookmark_node(), Eq(bookmark_node));
+}
+
+TEST_F(BookmarkModelObserverImplTest, ShouldPositionSiblings) {
+  const std::string kTitle = "title";
+  const std::string kUrl = "http://www.url.com";
+
+  // Build this structure:
+  // bookmark_bar
+  //  |- node1
+  //  |- node2
+  // Expectation:
+  //  p1 < p2
+
+  const bookmarks::BookmarkNode* bookmark_bar_node =
+      bookmark_model()->bookmark_bar_node();
+  const bookmarks::BookmarkNode* bookmark_node1 = bookmark_model()->AddURL(
+      /*parent=*/bookmark_bar_node, /*index=*/0, base::UTF8ToUTF16(kTitle),
+      GURL(kUrl));
+
+  const bookmarks::BookmarkNode* bookmark_node2 = bookmark_model()->AddURL(
+      /*parent=*/bookmark_bar_node, /*index=*/1, base::UTF8ToUTF16(kTitle),
+      GURL(kUrl));
+
+  ASSERT_THAT(bookmark_tracker()->TrackedEntitiesCountForTest(), Eq(3U));
+  const SyncedBookmarkTracker::Entity* entity1 =
+      bookmark_tracker()->GetEntityForBookmarkNode(bookmark_node1);
+  ASSERT_THAT(entity1, NotNull());
+  syncer::UniquePosition p1 =
+      syncer::UniquePosition::FromProto(entity1->metadata()->unique_position());
+
+  const SyncedBookmarkTracker::Entity* entity2 =
+      bookmark_tracker()->GetEntityForBookmarkNode(bookmark_node2);
+  ASSERT_THAT(entity2, NotNull());
+  syncer::UniquePosition p2 =
+      syncer::UniquePosition::FromProto(entity2->metadata()->unique_position());
+  EXPECT_TRUE(p1.LessThan(p2));
+
+  // Now insert node3 at index 1 to build this structure:
+  // bookmark_bar
+  //  |- node1
+  //  |- node3
+  //  |- node2
+  // Expectation:
+  //  p1 < p2 (still holds)
+  //  p1 < p3
+  //  p3 < p2
+
+  const bookmarks::BookmarkNode* bookmark_node3 = bookmark_model()->AddURL(
+      /*parent=*/bookmark_bar_node, /*index=*/1, base::UTF8ToUTF16(kTitle),
+      GURL(kUrl));
+  EXPECT_THAT(bookmark_tracker()->TrackedEntitiesCountForTest(), Eq(4U));
+
+  const SyncedBookmarkTracker::Entity* entity3 =
+      bookmark_tracker()->GetEntityForBookmarkNode(bookmark_node3);
+  ASSERT_THAT(entity3, NotNull());
+  syncer::UniquePosition p3 =
+      syncer::UniquePosition::FromProto(entity3->metadata()->unique_position());
+  EXPECT_TRUE(p1.LessThan(p2));
+  EXPECT_TRUE(p1.LessThan(p3));
+  EXPECT_TRUE(p3.LessThan(p2));
+}
+
+}  // namespace
+
+}  // namespace sync_bookmarks

components/sync_bookmarks/bookmark_model_type_processor.h

@@ -135,6 +135,16 @@ 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(
+      std::vector<NodeMetadataPair> nodes_metadata,
+      std::unique_ptr<sync_pb::ModelTypeState> model_type_state);
+
   // Stores the start callback in between OnSyncStarting() and
   // DecodeSyncMetadata().
   StartCallback start_callback_;

components/sync_bookmarks/bookmark_model_type_processor_unittest.cc

@@ -8,9 +8,11 @@
 
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/mock_callback.h"
+#include "base/test/scoped_task_environment.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/bookmarks/test/test_bookmark_client.h"
 #include "components/sync/driver/fake_sync_client.h"
+#include "components/sync/model/data_type_activation_request.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -29,6 +31,7 @@ const char kRootParentTag[] = "0";
 const char kBookmarkBarTag[] = "bookmark_bar";
 const char kBookmarkBarId[] = "bookmark_bar_id";
 const char kBookmarksRootId[] = "32904_google_chrome_bookmarks";
+const char kCacheGuid[] = "generated_id";
 
 struct BookmarkInfo {
   std::string server_id;
@@ -60,6 +63,13 @@ syncer::UpdateResponseData CreateUpdateData(const BookmarkInfo& bookmark_info) {
   return response_data;
 }
 
+sync_pb::ModelTypeState CreateDummyModelTypeState() {
+  sync_pb::ModelTypeState model_type_state;
+  model_type_state.set_cache_guid(kCacheGuid);
+  model_type_state.set_initial_sync_done(true);
+  return model_type_state;
+}
+
 void AssertState(const BookmarkModelTypeProcessor* processor,
                  const std::vector<BookmarkInfo>& bookmarks) {
   const SyncedBookmarkTracker* tracker = processor->GetTrackerForTest();
@@ -94,7 +104,7 @@ void InitWithSyncedBookmarks(const std::vector<BookmarkInfo>& bookmarks,
   for (BookmarkInfo bookmark : bookmarks) {
     updates.push_back(CreateUpdateData(bookmark));
   }
-  processor->OnUpdateReceived(sync_pb::ModelTypeState(), updates);
+  processor->OnUpdateReceived(CreateDummyModelTypeState(), updates);
   AssertState(processor, bookmarks);
 }
 
@@ -134,8 +144,14 @@ class BookmarkModelTypeProcessorTest : public testing::Test {
       : bookmark_model_(bookmarks::TestBookmarkClient::CreateModel()),
         sync_client_(bookmark_model_.get()),
         processor_(sync_client()->GetBookmarkUndoServiceIfExists()) {
+    // TODO(crbug.com/516866): This class assumes model is loaded and sync has
+    // started before running tests. We should test other variations (i.e. model
+    // isn't loaded yet and/or sync didn't start yet).
     processor_.DecodeSyncMetadata(std::string(), schedule_save_closure_.Get(),
                                   bookmark_model_.get());
+    syncer::DataTypeActivationRequest request;
+    request.cache_guid = kCacheGuid;
+    processor_.OnSyncStarting(request, base::DoNothing());
   }
 
   TestSyncClient* sync_client() { return &sync_client_; }
@@ -146,9 +162,10 @@ class BookmarkModelTypeProcessorTest : public testing::Test {
   }
 
  private:
+  base::test::ScopedTaskEnvironment task_environment_;
+  NiceMock<base::MockCallback<base::RepeatingClosure>> schedule_save_closure_;
   std::unique_ptr<bookmarks::BookmarkModel> bookmark_model_;
   TestSyncClient sync_client_;
-  NiceMock<base::MockCallback<base::RepeatingClosure>> schedule_save_closure_;
   BookmarkModelTypeProcessor processor_;
 };
 
@@ -205,7 +222,7 @@ TEST_F(BookmarkModelTypeProcessorTest, ShouldUpdateModelAfterRemoteCreation) {
   // 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(sync_pb::ModelTypeState(), updates);
+  processor()->OnUpdateReceived(CreateDummyModelTypeState(), updates);
 
   ASSERT_THAT(bookmarkbar->GetChild(0), NotNull());
   EXPECT_THAT(bookmarkbar->GetChild(0)->GetTitle(), Eq(ASCIIToUTF16(kTitle)));
@@ -241,7 +258,7 @@ TEST_F(BookmarkModelTypeProcessorTest, ShouldUpdateModelAfterRemoteUpdate) {
   // 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(sync_pb::ModelTypeState(), updates);
+  processor()->OnUpdateReceived(CreateDummyModelTypeState(), updates);
 
   // Check if the bookmark has been updated properly.
   EXPECT_THAT(bookmark_bar->GetChild(0), Eq(bookmark_node));
@@ -273,7 +290,7 @@ TEST_F(BookmarkModelTypeProcessorTest,
   updates[0].response_version++;
 
   EXPECT_CALL(*schedule_save_closure(), Run());
-  processor()->OnUpdateReceived(sync_pb::ModelTypeState(), updates);
+  processor()->OnUpdateReceived(CreateDummyModelTypeState(), updates);
 }
 
 TEST_F(BookmarkModelTypeProcessorTest, ShouldUpdateModelAfterRemoteDelete) {
@@ -330,11 +347,10 @@ TEST_F(BookmarkModelTypeProcessorTest, ShouldUpdateModelAfterRemoteDelete) {
   updates.push_back(CreateTombstone(kTitle1Id));
   updates.push_back(CreateTombstone(kFolder1Id));
 
-  const sync_pb::ModelTypeState model_type_state;
   // 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(model_type_state, updates);
+  processor()->OnUpdateReceived(CreateDummyModelTypeState(), updates);
 
   // The structure should be
   // bookmark_bar
@@ -406,8 +422,7 @@ TEST_F(BookmarkModelTypeProcessorTest, ShouldEncodeSyncMetadata) {
   syncer::UpdateResponseDataList updates;
   updates.push_back(CreateTombstone(kNodeId1));
 
-  const sync_pb::ModelTypeState model_type_state;
-  processor()->OnUpdateReceived(model_type_state, updates);
+  processor()->OnUpdateReceived(CreateDummyModelTypeState(), updates);
 
   metadata_str = processor()->EncodeSyncMetadata();
   model_metadata.ParseFromString(metadata_str);

components/sync_bookmarks/bookmark_sync_service.cc

@@ -22,7 +22,9 @@ BookmarkSyncService::BookmarkSyncService(
 
 BookmarkSyncService::~BookmarkSyncService() {}
 
-void BookmarkSyncService::Shutdown() {}
+void BookmarkSyncService::Shutdown() {
+  bookmark_model_type_processor_.reset();
+}
 
 std::string BookmarkSyncService::EncodeBookmarkSyncMetadata() {
   if (!bookmark_model_type_processor_) {

components/sync_bookmarks/synced_bookmark_tracker.cc

@@ -43,6 +43,9 @@ bool SyncedBookmarkTracker::Entity::IsUnsynced() const {
 
 bool SyncedBookmarkTracker::Entity::MatchesData(
     const syncer::EntityData& data) const {
+  // TODO(crbug.com/516866): Check parent id and unique position.
+  // TODO(crbug.com/516866): Compare the actual specifics instead of the
+  // specifics hash.
   if (metadata_->is_deleted() || data.is_deleted()) {
     // In case of deletion, no need to check the specifics.
     return metadata_->is_deleted() == data.is_deleted();
@@ -63,10 +66,13 @@ SyncedBookmarkTracker::SyncedBookmarkTracker(
     std::vector<NodeMetadataPair> nodes_metadata,
     std::unique_ptr<sync_pb::ModelTypeState> model_type_state)
     : model_type_state_(std::move(model_type_state)) {
+  DCHECK(model_type_state_);
   for (NodeMetadataPair& node_metadata : nodes_metadata) {
     const std::string& sync_id = node_metadata.second->server_id();
-    sync_id_to_entities_map_[sync_id] = std::make_unique<Entity>(
-        node_metadata.first, std::move(node_metadata.second));
+    auto entity = std::make_unique<Entity>(node_metadata.first,
+                                           std::move(node_metadata.second));
+    bookmark_node_to_entities_map_[node_metadata.first] = entity.get();
+    sync_id_to_entities_map_[sync_id] = std::move(entity);
   }
 }
 
@@ -78,6 +84,13 @@ const SyncedBookmarkTracker::Entity* SyncedBookmarkTracker::GetEntityForSyncId(
   return it != sync_id_to_entities_map_.end() ? it->second.get() : nullptr;
 }
 
+const SyncedBookmarkTracker::Entity*
+SyncedBookmarkTracker::GetEntityForBookmarkNode(
+    const bookmarks::BookmarkNode* node) const {
+  auto it = bookmark_node_to_entities_map_.find(node);
+  return it != bookmark_node_to_entities_map_.end() ? it->second : nullptr;
+}
+
 void SyncedBookmarkTracker::Add(const std::string& sync_id,
                                 const bookmarks::BookmarkNode* bookmark_node,
                                 int64_t server_version,
@@ -90,12 +103,14 @@ void SyncedBookmarkTracker::Add(const std::string& sync_id,
   metadata->set_server_id(sync_id);
   metadata->set_server_version(server_version);
   metadata->set_creation_time(syncer::TimeToProtoTime(creation_time));
+  metadata->set_modification_time(syncer::TimeToProtoTime(creation_time));
   metadata->set_sequence_number(0);
   metadata->set_acked_sequence_number(0);
   metadata->mutable_unique_position()->CopyFrom(unique_position);
   HashSpecifics(specifics, metadata->mutable_specifics_hash());
-  sync_id_to_entities_map_[sync_id] =
-      std::make_unique<Entity>(bookmark_node, std::move(metadata));
+  auto entity = std::make_unique<Entity>(bookmark_node, std::move(metadata));
+  bookmark_node_to_entities_map_[bookmark_node] = entity.get();
+  sync_id_to_entities_map_[sync_id] = std::move(entity);
 }
 
 void SyncedBookmarkTracker::Update(const std::string& sync_id,
@@ -114,6 +129,9 @@ void SyncedBookmarkTracker::Update(const std::string& sync_id,
 }
 
 void SyncedBookmarkTracker::Remove(const std::string& sync_id) {
+  const Entity* entity = GetEntityForSyncId(sync_id);
+  DCHECK(entity);
+  bookmark_node_to_entities_map_.erase(entity->bookmark_node());
   sync_id_to_entities_map_.erase(sync_id);
 }
 
@@ -155,6 +173,49 @@ bool SyncedBookmarkTracker::HasLocalChanges() const {
   return false;
 }
 
+std::vector<const SyncedBookmarkTracker::Entity*>
+SyncedBookmarkTracker::GetEntitiesWithLocalChanges(size_t max_entries) const {
+  // TODO(crbug.com/516866): Reorder local changes to e.g. parent creation
+  // before child creation and the otherway around for deletions.
+  std::vector<const SyncedBookmarkTracker::Entity*> entities_with_local_changes;
+  for (const std::pair<const std::string, std::unique_ptr<Entity>>& pair :
+       sync_id_to_entities_map_) {
+    Entity* entity = pair.second.get();
+    if (entity->IsUnsynced()) {
+      entities_with_local_changes.push_back(entity);
+    }
+  }
+  return entities_with_local_changes;
+}
+
+void SyncedBookmarkTracker::UpdateUponCommitResponse(
+    const std::string& old_id,
+    const std::string& new_id,
+    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;
+  if (it == sync_id_to_entities_map_.end()) {
+    DLOG(WARNING) << "Trying to update a non existing entity.";
+    return;
+  }
+  const bookmarks::BookmarkNode* node = entity->bookmark_node();
+  // TODO(crbug.com/516866): For tombstones, node would be null and the DCHECK
+  // below would be invalid. Handle deletions here may be or in the processor.
+  DCHECK(node);
+
+  if (old_id != new_id) {
+    auto it = sync_id_to_entities_map_.find(old_id);
+    entity->metadata()->set_server_id(new_id);
+    sync_id_to_entities_map_[new_id] = std::move(it->second);
+    sync_id_to_entities_map_.erase(old_id);
+  }
+  entity->metadata()->set_acked_sequence_number(acked_sequence_number);
+  entity->metadata()->set_server_version(server_version);
+}
+
 std::size_t SyncedBookmarkTracker::TrackedEntitiesCountForTest() const {
   return sync_id_to_entities_map_.size();
 }

components/sync_bookmarks/synced_bookmark_tracker.h

@@ -76,20 +76,24 @@ class SyncedBookmarkTracker {
       std::unique_ptr<sync_pb::ModelTypeState> model_type_state);
   ~SyncedBookmarkTracker();
 
-  // Returns null if not entity is found.
+  // Returns null if no entity is found.
   const Entity* GetEntityForSyncId(const std::string& sync_id) const;
 
+  // Returns null if no entity is found.
+  const SyncedBookmarkTracker::Entity* GetEntityForBookmarkNode(
+      const bookmarks::BookmarkNode* node) const;
+
   // Adds an entry for the |sync_id| and the corresponding local bookmark node
   // and metadata in |sync_id_to_entities_map_|.
   void Add(const std::string& sync_id,
            const bookmarks::BookmarkNode* bookmark_node,
            int64_t server_version,
-           base::Time modification_time,
+           base::Time creation_time,
            const sync_pb::UniquePosition& unique_position,
            const sync_pb::EntitySpecifics& specifics);
 
-  // Adds an existing entry for the |sync_id| and the corresponding metadata in
-  // |sync_id_to_entities_map_|.
+  // Updates an existing entry for the |sync_id| and the corresponding metadata
+  // in |sync_id_to_entities_map_|.
   void Update(const std::string& sync_id,
               int64_t server_version,
               base::Time modification_time,
@@ -112,16 +116,38 @@ class SyncedBookmarkTracker {
     return *model_type_state_;
   }
 
+  void set_model_type_state(
+      std::unique_ptr<sync_pb::ModelTypeState> model_type_state) {
+    model_type_state_ = std::move(model_type_state);
+  }
+
+  std::vector<const Entity*> GetEntitiesWithLocalChanges(
+      size_t max_entries) const;
+
+  // Updates the tracker after receiving the commit response. |old_id| should be
+  // equal to |new_id| for all updates except the initial commit, where the
+  // temporary client-generated ID will be overriden by the server-provided
+  // final ID. In which case |sync_id_to_entities_map_| will be updated
+  // accordingly.
+  void UpdateUponCommitResponse(const std::string& old_id,
+                                const std::string& new_id,
+                                int64_t acked_sequence_number,
+                                int64_t server_version);
+
   // Returns number of tracked entities. Used only in test.
   std::size_t TrackedEntitiesCountForTest() const;
 
  private:
   // A map of sync server ids to sync entities. This should contain entries and
-  // metadata for almost everything. However, since local data are loaded only
-  // when needed (e.g. before a commit cycle), the entities may not always
-  // contain model type data/specifics.
+  // metadata for almost everything.
   std::map<std::string, std::unique_ptr<Entity>> sync_id_to_entities_map_;
 
+  // A map of bookmark nodes to sync entities. It's keyed by the bookmark node
+  // pointers which get assigned when loading the bookmark model. This map is
+  // first initialized in the constructor.
+  std::map<const bookmarks::BookmarkNode*, Entity*>
+      bookmark_node_to_entities_map_;
+
   // The model metadata (progress marker, initial sync done, etc).
   std::unique_ptr<sync_pb::ModelTypeState> model_type_state_;
 

components/sync_bookmarks/synced_bookmark_tracker_unittest.cc

@@ -108,6 +108,37 @@ TEST(SyncedBookmarkTrackerTest,
   // request in a separate test probably.
 }
 
+TEST(SyncedBookmarkTrackerTest, ShouldUpdateUponCommitResponseWithNewId) {
+  SyncedBookmarkTracker tracker(std::vector<NodeMetadataPair>(),
+                                std::make_unique<sync_pb::ModelTypeState>());
+  const std::string kSyncId = "SYNC_ID";
+  const std::string kNewSyncId = "NEW_SYNC_ID";
+  const int64_t kId = 1;
+  const int64_t kServerVersion = 1000;
+  const int64_t kNewServerVersion = 1001;
+  const base::Time kModificationTime(base::Time::Now() -
+                                     base::TimeDelta::FromSeconds(1));
+  const sync_pb::UniquePosition unique_position;
+  const sync_pb::EntitySpecifics specifics =
+      GenerateSpecifics(/*title=*/std::string(), /*url=*/std::string());
+  bookmarks::BookmarkNode node(kId, GURL());
+  tracker.Add(kSyncId, &node, kServerVersion, kModificationTime,
+              unique_position, specifics);
+  ASSERT_THAT(tracker.GetEntityForSyncId(kSyncId), NotNull());
+  // Receive a commit response with a changed id.
+  tracker.UpdateUponCommitResponse(
+      kSyncId, kNewSyncId, /*acked_sequence_number=*/1, kNewServerVersion);
+  // Old id shouldn't be there.
+  EXPECT_THAT(tracker.GetEntityForSyncId(kSyncId), IsNull());
+
+  const SyncedBookmarkTracker::Entity* entity =
+      tracker.GetEntityForSyncId(kNewSyncId);
+  ASSERT_THAT(entity, NotNull());
+  EXPECT_THAT(entity->metadata()->server_id(), Eq(kNewSyncId));
+  EXPECT_THAT(entity->bookmark_node(), Eq(&node));
+  EXPECT_THAT(entity->metadata()->server_version(), Eq(kNewServerVersion));
+}
+
 }  // namespace
 
 }  // namespace sync_bookmarks