[Sync::USS] Fix for frequent saving of bookmark model file

Before this CL, BookmarkModelTypeProcessor schedules a save upon
changes in ModelTypeState even if they dont't involve changes in
Sync entity metadata.
This results in frequent writes of the bookmarks file which
could be potentially large for some users.

This CL makes sure that the file is saved only upon changes in the
entity metadata.

Bug: 945820
Change-Id: Ib0d581b1b585e5cc4bff1721bf8767abc047316d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1538526
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#644372}

components/sync_bookmarks/bookmark_model_type_processor.cc

@@ -8,6 +8,7 @@
 
 #include "base/bind.h"
 #include "base/callback.h"
+#include "base/feature_list.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "components/bookmarks/browser/bookmark_model.h"
@@ -33,6 +34,15 @@ namespace sync_bookmarks {
 
 namespace {
 
+// Enables scheduling bookmark model saving only upon changes in entity sync
+// metadata. This would stop persisting changes to the model type state that
+// doesn't involve changes to the entity metadata as well.
+// TODO(crbug.com/945820): This should be removed in M80 if not issues are
+// observed.
+const base::Feature kSyncScheduleForEntityMetadataChangesOnly{
+    "SyncScheduleForEntityMetadataChangesOnly",
+    base::FEATURE_ENABLED_BY_DEFAULT};
+
 class ScopedRemoteUpdateBookmarks {
  public:
   // |bookmark_model|, |bookmark_undo_service| and |observer| must not be null
@@ -210,8 +220,14 @@ void BookmarkModelTypeProcessor::OnUpdateReceived(
   bookmark_tracker_->set_model_type_state(
       std::make_unique<sync_pb::ModelTypeState>(model_type_state));
   updates_handler.Process(updates, got_new_encryption_requirements);
-  // Schedule save just in case one is needed.
-  schedule_save_closure_.Run();
+  // There are cases when we receive non-empty updates that don't result in
+  // model changes (e.g. reflections). In that case, issue a write to persit the
+  // progress marker in order to avoid downloading those updates again.
+  if (!updates.empty() || !base::FeatureList::IsEnabled(
+                              kSyncScheduleForEntityMetadataChangesOnly)) {
+    // Schedule save just in case one is needed.
+    schedule_save_closure_.Run();
+  }
 }
 
 const SyncedBookmarkTracker* BookmarkModelTypeProcessor::GetTrackerForTest()

components/sync_bookmarks/bookmark_model_type_processor_unittest.cc

@@ -258,8 +258,9 @@ TEST_F(BookmarkModelTypeProcessorTest, ShouldUpdateModelAfterRemoteUpdate) {
   EXPECT_THAT(bookmark_node->url(), Eq(GURL(kNewUrl)));
 }
 
-TEST_F(BookmarkModelTypeProcessorTest,
-       ShouldScheduleSaveAfterRemoteUpdateWithOnlyMetadataChange) {
+TEST_F(
+    BookmarkModelTypeProcessorTest,
+    ShouldScheduleSaveAfterRemoteUpdateWithOnlyMetadataChangeAndReflections) {
   const std::string kNodeId = "node_id";
   const std::string kTitle = "title";
   const std::string kUrl = "http://www.url.com";
@@ -444,7 +445,6 @@ TEST_F(BookmarkModelTypeProcessorTest,
   sync_pb::ModelTypeState model_type_state(CreateDummyModelTypeState());
   model_type_state.set_encryption_key_name(kEncryptionKeyName);
 
-  EXPECT_CALL(*schedule_save_closure(), Run());
   // Push empty updates list to the processor together with the updated model
   // type state.
   processor()->OnUpdateReceived(model_type_state,