[Sync::USS] Implement SyncedBookmarkTracker::HasLocalChanges()

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

components/sync_bookmarks/synced_bookmark_tracker.cc

@@ -37,6 +37,10 @@ SyncedBookmarkTracker::Entity::Entity(
 
 SyncedBookmarkTracker::Entity::~Entity() = default;
 
+bool SyncedBookmarkTracker::Entity::IsUnsynced() const {
+  return metadata_->sequence_number() > metadata_->acked_sequence_number();
+}
+
 bool SyncedBookmarkTracker::Entity::MatchesData(
     const syncer::EntityData& data) const {
   if (metadata_->is_deleted() || data.is_deleted()) {
@@ -55,10 +59,6 @@ bool SyncedBookmarkTracker::Entity::MatchesSpecificsHash(
   return hash == metadata_->specifics_hash();
 }
 
-bool SyncedBookmarkTracker::Entity::IsUnsynced() const {
-  return metadata_->sequence_number() > metadata_->acked_sequence_number();
-}
-
 SyncedBookmarkTracker::SyncedBookmarkTracker(
     std::vector<NodeMetadataPair> nodes_metadata,
     std::unique_ptr<sync_pb::ModelTypeState> model_type_state)
@@ -89,6 +89,8 @@ 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_sequence_number(0);
+  metadata->set_acked_sequence_number(0);
   HashSpecifics(specifics, metadata->mutable_specifics_hash());
   sync_id_to_entities_map_[sync_id] =
       std::make_unique<Entity>(bookmark_node, std::move(metadata));
@@ -113,6 +115,17 @@ void SyncedBookmarkTracker::Remove(const std::string& sync_id) {
   sync_id_to_entities_map_.erase(sync_id);
 }
 
+void SyncedBookmarkTracker::IncrementSequenceNumber(
+    const std::string& sync_id) {
+  Entity* entity = sync_id_to_entities_map_.find(sync_id)->second.get();
+  DCHECK(entity);
+  DCHECK(!entity->metadata()->is_deleted());
+  // TODO(crbug.com/516866): Update base hash specifics here if the entity is
+  // not already out of sync.
+  entity->metadata()->set_sequence_number(
+      entity->metadata()->sequence_number() + 1);
+}
+
 sync_pb::BookmarkModelMetadata
 SyncedBookmarkTracker::BuildBookmarkModelMetadata() const {
   sync_pb::BookmarkModelMetadata model_metadata;
@@ -130,7 +143,13 @@ SyncedBookmarkTracker::BuildBookmarkModelMetadata() const {
 }
 
 bool SyncedBookmarkTracker::HasLocalChanges() const {
-  NOTIMPLEMENTED();
+  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()) {
+      return true;
+    }
+  }
   return false;
 }
 

components/sync_bookmarks/synced_bookmark_tracker.h

@@ -96,6 +96,10 @@ class SyncedBookmarkTracker {
   // |sync_id_to_entities_map_|.
   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|.
+  void IncrementSequenceNumber(const std::string& sync_id);
+
   sync_pb::BookmarkModelMetadata BuildBookmarkModelMetadata() const;
 
   // Returns true if there are any local entities to be committed.

components/sync_bookmarks/synced_bookmark_tracker_unittest.cc

@@ -39,7 +39,7 @@ TEST(SyncedBookmarkTrackerTest, ShouldGetAssociatedNodes) {
   const base::Time kCreationTime(base::Time::Now() -
                                  base::TimeDelta::FromSeconds(1));
   const sync_pb::EntitySpecifics specifics =
-      GenerateSpecifics(std::string(), std::string());
+      GenerateSpecifics(/*title=*/std::string(), /*url=*/std::string());
 
   bookmarks::BookmarkNode node(kId, kUrl);
   tracker.Add(kSyncId, &node, kServerVersion, kCreationTime, specifics);
@@ -66,7 +66,7 @@ TEST(SyncedBookmarkTrackerTest, ShouldReturnNullForDisassociatedNodes) {
   const base::Time kModificationTime(base::Time::Now() -
                                      base::TimeDelta::FromSeconds(1));
   const sync_pb::EntitySpecifics specifics =
-      GenerateSpecifics(std::string(), std::string());
+      GenerateSpecifics(/*title=*/std::string(), /*url=*/std::string());
   bookmarks::BookmarkNode node(kId, GURL());
   tracker.Add(kSyncId, &node, kServerVersion, kModificationTime, specifics);
   ASSERT_THAT(tracker.GetEntityForSyncId(kSyncId), NotNull());
@@ -74,6 +74,27 @@ TEST(SyncedBookmarkTrackerTest, ShouldReturnNullForDisassociatedNodes) {
   EXPECT_THAT(tracker.GetEntityForSyncId(kSyncId), IsNull());
 }
 
+TEST(SyncedBookmarkTrackerTest,
+     ShouldRequireCommitRequestWhenSequenceNumberIsIncremented) {
+  SyncedBookmarkTracker tracker(std::vector<NodeMetadataPair>(),
+                                std::make_unique<sync_pb::ModelTypeState>());
+  const std::string kSyncId = "SYNC_ID";
+  const int64_t kId = 1;
+  const int64_t kServerVersion = 1000;
+  const base::Time kModificationTime(base::Time::Now() -
+                                     base::TimeDelta::FromSeconds(1));
+  const sync_pb::EntitySpecifics specifics =
+      GenerateSpecifics(/*title=*/std::string(), /*url=*/std::string());
+  bookmarks::BookmarkNode node(kId, GURL());
+  tracker.Add(kSyncId, &node, kServerVersion, kModificationTime, specifics);
+
+  EXPECT_THAT(tracker.HasLocalChanges(), Eq(false));
+  tracker.IncrementSequenceNumber(kSyncId);
+  EXPECT_THAT(tracker.HasLocalChanges(), Eq(true));
+  // TODO(crbug.com/516866): Test HasLocalChanges after submitting commit
+  // request in a separate test probably.
+}
+
 }  // namespace
 
 }  // namespace sync_bookmarks