Revert "[Sync] Postpone reupload of bookmarks until next updates received"

This reverts commit 56f7b697.

Reason for revert: win-asan failures
https://bugs.chromium.org/p/chromium/issues/detail?id=1106392

Original change's description:
> [Sync] Postpone reupload of bookmarks until next updates received
> 
> When reupload is initiated after loading from the disk right away, it
> may lead to upload bookmarks which were removed from the other device.
> This would restore removed bookmarks. In practise it may be caused by
> some devices which a rarely used.
> 
> Initiating reupload after receiving latest updates from the server helps
> to prevent such behaviour.
> 
> Bug: 1061411
> Change-Id: Ibc5646b19bf133de496140fa152b544bc06ebcf9
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2299721
> Commit-Queue: Rushan Suleymanov <rushans@google.com>
> Reviewed-by: Mikel Astiz <mastiz@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#788775}

TBR=mastiz@chromium.org,rushans@google.com

Change-Id: Ib00d0dfc3cd570aaf1081482cf1bd727b3dbdf8e
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1061411
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2302773Reviewed-by: Jonah Ryan-Davis <jonahr@google.com>
Commit-Queue: Jonah Ryan-Davis <jonahr@google.com>
Cr-Commit-Position: refs/heads/master@{#789078}

components/sync_bookmarks/bookmark_model_type_processor.cc

@@ -246,9 +246,6 @@ void BookmarkModelTypeProcessor::OnUpdateReceived(
   bookmark_tracker_->set_model_type_state(model_type_state);
   bookmark_tracker_->UpdateLastSyncTime();
   updates_handler.Process(updates, got_new_encryption_requirements);
-  if (bookmark_tracker_->ReuploadBookmarksOnLoadIfNeeded()) {
-    NudgeForCommitIfNeeded();
-  }
   // 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.

components/sync_bookmarks/bookmark_model_type_processor_unittest.cc

@@ -19,7 +19,6 @@
 #include "components/bookmarks/test/test_bookmark_client.h"
 #include "components/favicon/core/test/mock_favicon_service.h"
 #include "components/sync/base/unique_position.h"
-#include "components/sync/engine/commit_queue.h"
 #include "components/sync/model/data_type_activation_request.h"
 #include "components/sync_bookmarks/switches.h"
 #include "components/undo/bookmark_undo_service.h"
@@ -64,7 +63,6 @@ syncer::UpdateResponseData CreateUpdateResponseData(
   data.parent_id = bookmark_info.parent_id;
   data.server_defined_unique_tag = bookmark_info.server_tag;
   data.unique_position = unique_position.ToProto();
-  data.originator_client_item_id = guid;
 
   sync_pb::BookmarkSpecifics* bookmark_specifics =
       data.specifics.mutable_bookmark();
@@ -151,24 +149,6 @@ void InitWithSyncedBookmarks(const std::vector<BookmarkInfo>& bookmarks,
   AssertState(processor, bookmarks);
 }
 
-class MockCommitQueue : public syncer::CommitQueue {
- public:
-  MOCK_METHOD0(NudgeForCommit, void());
-};
-
-class ProxyCommitQueue : public syncer::CommitQueue {
- public:
-  explicit ProxyCommitQueue(CommitQueue* commit_queue)
-      : commit_queue_(commit_queue) {
-    DCHECK(commit_queue_);
-  }
-
-  void NudgeForCommit() override { commit_queue_->NudgeForCommit(); }
-
- private:
-  CommitQueue* commit_queue_ = nullptr;
-};
-
 class TestBookmarkClientWithFavicon : public bookmarks::TestBookmarkClient {
  public:
   // This method must be used to tell the bookmark_model about favicon.
@@ -212,18 +192,17 @@ class TestBookmarkClientWithFavicon : public bookmarks::TestBookmarkClient {
 class BookmarkModelTypeProcessorTest : public testing::Test {
  public:
   BookmarkModelTypeProcessorTest()
-      : processor_(std::make_unique<BookmarkModelTypeProcessor>(
-            &bookmark_undo_service_)),
+      : processor_(&bookmark_undo_service_),
         bookmark_model_(bookmarks::TestBookmarkClient::CreateModelWithClient(
             std::make_unique<TestBookmarkClientWithFavicon>())) {
     // 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_->SetFaviconService(&favicon_service_);
+    processor_.SetFaviconService(&favicon_service_);
   }
 
   void SimulateModelReadyToSync() {
-    processor_->ModelReadyToSync(
+    processor_.ModelReadyToSync(
         /*metadata_str=*/std::string(), schedule_save_closure_.Get(),
         bookmark_model_.get());
   }
@@ -231,19 +210,7 @@ class BookmarkModelTypeProcessorTest : public testing::Test {
   void SimulateOnSyncStarting() {
     syncer::DataTypeActivationRequest request;
     request.cache_guid = kCacheGuid;
-    processor_->OnSyncStarting(request, base::DoNothing());
-  }
-
-  void SimulateConnectSync() {
-    processor_->ConnectSync(
-        std::make_unique<ProxyCommitQueue>(&mock_commit_queue_));
-  }
-
-  // Simulate browser restart.
-  void ResetModelTypeProcessor() {
-    processor_ =
-        std::make_unique<BookmarkModelTypeProcessor>(&bookmark_undo_service_);
-    processor_->SetFaviconService(&favicon_service_);
+    processor_.OnSyncStarting(request, base::DoNothing());
   }
 
   void DestroyBookmarkModel() { bookmark_model_.reset(); }
@@ -257,8 +224,7 @@ class BookmarkModelTypeProcessorTest : public testing::Test {
     return &bookmark_undo_service_;
   }
   favicon::FaviconService* favicon_service() { return &favicon_service_; }
-  MockCommitQueue* mock_commit_queue() { return &mock_commit_queue_; }
-  BookmarkModelTypeProcessor* processor() { return processor_.get(); }
+  BookmarkModelTypeProcessor* processor() { return &processor_; }
   base::MockCallback<base::RepeatingClosure>* schedule_save_closure() {
     return &schedule_save_closure_;
   }
@@ -276,8 +242,7 @@ class BookmarkModelTypeProcessorTest : public testing::Test {
   NiceMock<base::MockCallback<base::RepeatingClosure>> schedule_save_closure_;
   BookmarkUndoService bookmark_undo_service_;
   NiceMock<favicon::MockFaviconService> favicon_service_;
-  NiceMock<MockCommitQueue> mock_commit_queue_;
-  std::unique_ptr<BookmarkModelTypeProcessor> processor_;
+  BookmarkModelTypeProcessor processor_;
   std::unique_ptr<bookmarks::BookmarkModel> bookmark_model_;
 };
 
@@ -575,7 +540,6 @@ TEST_F(BookmarkModelTypeProcessorTest,
        ShouldNotRecommitEntitiesWhenEncryptionIsUpToDate) {
   SimulateModelReadyToSync();
   SimulateOnSyncStarting();
-  SimulateConnectSync();
   // Initialize the process to make sure the tracker has been created.
   InitWithSyncedBookmarks({}, processor());
   const SyncedBookmarkTracker* tracker = processor()->GetTrackerForTest();
@@ -597,7 +561,6 @@ TEST_F(BookmarkModelTypeProcessorTest,
       /*response_version=*/0);
   response_data.encryption_key_name = kEncryptionKeyName;
 
-  EXPECT_CALL(*mock_commit_queue(), NudgeForCommit()).Times(0);
   syncer::UpdateResponseDataList updates;
   updates.push_back(std::move(response_data));
   processor()->OnUpdateReceived(model_type_state, std::move(updates));
@@ -724,49 +687,31 @@ TEST_F(BookmarkModelTypeProcessorTest, ShouldReuploadLegacyBookmarksOnStart) {
 
   SimulateModelReadyToSync();
   SimulateOnSyncStarting();
-  SimulateConnectSync();
   InitWithSyncedBookmarks(bookmarks, processor());
 
   sync_pb::BookmarkModelMetadata model_metadata =
       BuildBookmarkModelMetadataWithoutFullTitles();
+  // Ensure that bookmark is legacy.
+  model_metadata.clear_bookmarks_full_title_reuploaded();
   ASSERT_FALSE(processor()->GetTrackerForTest()->HasLocalChanges());
 
-  // Simulate browser restart, enable sync reupload and initialize the processor
-  // again.
-  ResetModelTypeProcessor();
-
   base::test::ScopedFeatureList features;
   features.InitAndEnableFeature(switches::kSyncReuploadBookmarkFullTitles);
+  BookmarkModelTypeProcessor new_processor(bookmark_undo_service());
 
   std::string metadata_str;
   model_metadata.SerializeToString(&metadata_str);
-  processor()->ModelReadyToSync(metadata_str, base::DoNothing(),
-                                bookmark_model());
-  SimulateOnSyncStarting();
-  SimulateConnectSync();
-
-  ASSERT_THAT(processor()->GetTrackerForTest(), NotNull());
-  const SyncedBookmarkTracker::Entity* entity =
-      processor()->GetTrackerForTest()->GetEntityForSyncId(kNodeId);
-  ASSERT_THAT(entity, NotNull());
-
-  // Entity should be synced before until first update is received.
-  ASSERT_FALSE(entity->IsUnsynced());
-  ASSERT_FALSE(processor()
-                   ->GetTrackerForTest()
-                   ->BuildBookmarkModelMetadata()
-                   .bookmarks_full_title_reuploaded());
-
-  // Synchronize with the server and get any updates.
-  EXPECT_CALL(*mock_commit_queue(), NudgeForCommit());
-  processor()->OnUpdateReceived(CreateDummyModelTypeState(),
-                                syncer::UpdateResponseDataList());
+  new_processor.ModelReadyToSync(metadata_str, base::DoNothing(),
+                                 bookmark_model());
 
   // Check that all entities are unsynced now and metadata is marked as
   // reuploaded.
-  EXPECT_TRUE(entity->IsUnsynced());
-  EXPECT_TRUE(processor()
-                  ->GetTrackerForTest()
+  const size_t entities_count =
+      processor()->GetTrackerForTest()->GetAllEntities().size();
+  EXPECT_EQ(1u, new_processor.GetTrackerForTest()
+                    ->GetEntitiesWithLocalChanges(entities_count)
+                    .size());
+  EXPECT_TRUE(new_processor.GetTrackerForTest()
                   ->BuildBookmarkModelMetadata()
                   .bookmarks_full_title_reuploaded());
 }

components/sync_bookmarks/synced_bookmark_tracker.cc

@@ -265,6 +265,8 @@ SyncedBookmarkTracker::CreateFromBookmarkModelAndMetadata(
     return nullptr;
   }
 
+  tracker->ReuploadBookmarksOnLoadIfNeeded();
+
   return tracker;
 }
 
@@ -730,11 +732,11 @@ SyncedBookmarkTracker::ReorderUnsyncedEntitiesExceptDeletions(
   return ordered_entities;
 }
 
-bool SyncedBookmarkTracker::ReuploadBookmarksOnLoadIfNeeded() {
+void SyncedBookmarkTracker::ReuploadBookmarksOnLoadIfNeeded() {
   if (bookmarks_full_title_reuploaded_ ||
       !base::FeatureList::IsEnabled(
           switches::kSyncReuploadBookmarkFullTitles)) {
-    return false;
+    return;
   }
   for (const auto& sync_id_and_entity : sync_id_to_entities_map_) {
     const SyncedBookmarkTracker::Entity* entity =
@@ -747,8 +749,7 @@ bool SyncedBookmarkTracker::ReuploadBookmarksOnLoadIfNeeded() {
     }
     IncrementSequenceNumber(entity);
   }
-  SetBookmarksFullTitleReuploaded();
-  return true;
+  bookmarks_full_title_reuploaded_ = true;
 }
 
 void SyncedBookmarkTracker::TraverseAndAppend(

components/sync_bookmarks/synced_bookmark_tracker.h

@@ -289,15 +289,6 @@ class SyncedBookmarkTracker {
   void CheckAllNodesTracked(
       const bookmarks::BookmarkModel* bookmark_model) const;
 
-  // This method is used to mark all entities except permanent nodes as
-  // unsynced. This will cause reuploading of all bookmarks. The reupload
-  // will be initiated only when the |bookmarks_full_title_reuploaded| field in
-  // BookmarksMetadata is false. This field is used to prevent reuploading after
-  // each browser restart. Returns true if the reupload was initiated.
-  // TODO(crbug.com/1066962): remove this code when most of bookmarks are
-  // reuploaded.
-  bool ReuploadBookmarksOnLoadIfNeeded();
-
  private:
   // Enumeration of possible reasons why persisted metadata are considered
   // corrupted and don't match the bookmark model. Used in UMA metrics. Do not
@@ -343,6 +334,16 @@ class SyncedBookmarkTracker {
   std::vector<const Entity*> ReorderUnsyncedEntitiesExceptDeletions(
       const std::vector<const Entity*>& entities) const;
 
+  // This method is used to mark all entities except permanent nodes as
+  // unsynced. This will cause reuploading of all bookmarks. This reupload
+  // should be initiated only when the |bookmarks_full_title_reuploaded| field
+  // in BookmarksMetadata is false. This field is used to prevent reuploading
+  // after each browser restart. It is set to true in
+  // BuildBookmarkModelMetadata.
+  // TODO(crbug.com/1066962): remove this code when most of bookmarks are
+  // reuploaded.
+  void ReuploadBookmarksOnLoadIfNeeded();
+
   // Recursive method that starting from |node| appends all corresponding
   // entities with updates in top-down order to |ordered_entities|.
   void TraverseAndAppend(const bookmarks::BookmarkNode* node,