[Sync] Reupload new title field for pre-existing bookmarks.

This change initiates bookmark reuploading for pre-existing bookmarks
which are stored locally. A new field is introduced to BookmarksMetadata
to determine if local bookmarks were reuploaded.

To prevent the case when the bookmark specifics is committed without
favicon the entity is marked as unsynced after it was committed. In this
case the entity will be committed again when the favicon is loaded.

Bug: 1061411
Change-Id: I4a26a2babceb99a0f6532d2357884c75aff9fc05
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2127032
Commit-Queue: Rushan Suleymanov <rushans@google.com>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#755858}

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

@@ -25,6 +25,7 @@
 #include "components/sync/test/fake_server/entity_builder_factory.h"
 #include "components/sync/test/fake_server/fake_server_verifier.h"
 #include "components/sync_bookmarks/switches.h"
+#include "content/public/test/test_launcher.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/layout.h"
@@ -99,15 +100,22 @@ void SingleClientBookmarksSyncTest::VerifyBookmarkModelMatchesFakeServer(
   }
 }
 
-class SingleClientBookmarksSyncTestWithEnabledReuploadTitles
+class SingleClientBookmarksSyncTestWithEnabledReuploadRemoteBookmarks
     : public SingleClientBookmarksSyncTest {
  public:
-  SingleClientBookmarksSyncTestWithEnabledReuploadTitles() {
-    features_.InitAndEnableFeature(switches::kSyncReuploadBookmarkFullTitles);
+  SingleClientBookmarksSyncTestWithEnabledReuploadRemoteBookmarks() {
+    feature_list_.InitAndEnableFeature(
+        switches::kSyncReuploadBookmarkFullTitles);
   }
+};
 
- private:
-  base::test::ScopedFeatureList features_;
+class SingleClientBookmarksSyncTestWithEnabledReuploadPreexistingBookmarks
+    : public SingleClientBookmarksSyncTest {
+ public:
+  SingleClientBookmarksSyncTestWithEnabledReuploadPreexistingBookmarks() {
+    feature_list_.InitWithFeatureState(
+        switches::kSyncReuploadBookmarkFullTitles, !content::IsPreTest());
+  }
 };
 
 IN_PROC_BROWSER_TEST_P(SingleClientBookmarksSyncTest, Sanity) {
@@ -1129,8 +1137,9 @@ IN_PROC_BROWSER_TEST_P(SingleClientBookmarksSyncTest,
   EXPECT_EQ(1u, GetBookmarkBarNode(kSingleProfileIndex)->children().size());
 }
 
-IN_PROC_BROWSER_TEST_P(SingleClientBookmarksSyncTestWithEnabledReuploadTitles,
-                       ShouldReuploadFullTitleAfterInitialMerge) {
+IN_PROC_BROWSER_TEST_P(
+    SingleClientBookmarksSyncTestWithEnabledReuploadRemoteBookmarks,
+    ShouldReuploadFullTitleAfterInitialMerge) {
   ASSERT_TRUE(SetupClients());
 
   const std::string title = "Title";
@@ -1156,8 +1165,77 @@ IN_PROC_BROWSER_TEST_P(SingleClientBookmarksSyncTestWithEnabledReuploadTitles,
   EXPECT_TRUE(server_bookmarks.front().specifics().bookmark().has_full_title());
 }
 
+// This test looks similar to
+// ShouldReuploadFullTitleAfterRestartOnIncrementalChange, but current test
+// initiates reupload after restart only (before restart the entity is in synced
+// state).
+IN_PROC_BROWSER_TEST_P(
+    SingleClientBookmarksSyncTestWithEnabledReuploadPreexistingBookmarks,
+    PRE_ShouldReuploadFullTitleForOldClients) {
+  // Prepare legacy bookmark without full_title field in specifics and store it
+  // locally.
+  ASSERT_TRUE(SetupSync());
+
+  const std::string title = "Title";
+  const GURL url = GURL("http://www.foo.com");
+
+  // Make an incremental remote creation of bookmark without full_title.
+  fake_server::EntityBuilderFactory entity_builder_factory;
+  fake_server::BookmarkEntityBuilder bookmark_builder =
+      entity_builder_factory.NewBookmarkEntityBuilder(title);
+  std::unique_ptr<syncer::LoopbackServerEntity> remote_folder =
+      bookmark_builder.BuildBookmark(url, /*is_legacy=*/false);
+  const std::string new_guid = remote_folder->GetSpecifics().bookmark().guid();
+
+  // Makr sure that server-side specifics doesn't have full title.
+  remote_folder->GetSpecifics().mutable_bookmark()->clear_full_title();
+  fake_server_->InjectEntity(std::move(remote_folder));
+
+  ASSERT_TRUE(BookmarksTitleChecker(kSingleProfileIndex, title,
+                                    /*expected_count=*/1)
+                  .Wait());
+}
+
 IN_PROC_BROWSER_TEST_P(
-    SingleClientBookmarksSyncTestWithEnabledReuploadTitles,
+    SingleClientBookmarksSyncTestWithEnabledReuploadPreexistingBookmarks,
+    ShouldReuploadFullTitleForOldClients) {
+  // This test checks that the legacy bookmark which was stored locally will
+  // imply reupload to the server when reupload feature is enabled.
+  ASSERT_EQ(
+      1u,
+      GetFakeServer()->GetSyncEntitiesByModelType(syncer::BOOKMARKS).size());
+  const int64_t old_server_version =
+      GetFakeServer()
+          ->GetSyncEntitiesByModelType(syncer::BOOKMARKS)
+          .front()
+          .version();
+  ASSERT_TRUE(SetupClients());
+#if defined(OS_CHROMEOS)
+  // signin::SetRefreshTokenForPrimaryAccount() is needed on ChromeOS in order
+  // to get a non-empty refresh token on startup.
+  GetClient(0)->SignInPrimaryAccount();
+#endif  // defined(OS_CHROMEOS)
+  ASSERT_TRUE(GetClient(kSingleProfileIndex)->AwaitEngineInitialization());
+  ASSERT_TRUE(
+      UpdatedProgressMarkerChecker(GetSyncService(kSingleProfileIndex)).Wait());
+  ASSERT_GT(GetFakeServer()
+                ->GetSyncEntitiesByModelType(syncer::BOOKMARKS)
+                .front()
+                .version(),
+            old_server_version);
+
+  const std::string title = "Title";
+  const std::vector<sync_pb::SyncEntity> server_bookmarks =
+      GetFakeServer()->GetSyncEntitiesByModelType(syncer::BOOKMARKS);
+  EXPECT_EQ(1u, CountBookmarksWithTitlesMatching(kSingleProfileIndex, title));
+  ASSERT_EQ(1u, server_bookmarks.size());
+  EXPECT_TRUE(server_bookmarks.front().specifics().bookmark().has_full_title());
+}
+
+// TODO(rushans): add the same test as before with favicons.
+
+IN_PROC_BROWSER_TEST_P(
+    SingleClientBookmarksSyncTestWithEnabledReuploadRemoteBookmarks,
     PRE_ShouldReuploadFullTitleAfterRestartOnIncrementalChange) {
   ASSERT_TRUE(SetupSync());
 
@@ -1186,8 +1264,9 @@ IN_PROC_BROWSER_TEST_P(
       server_bookmarks.front().specifics().bookmark().has_full_title());
 }
 
-IN_PROC_BROWSER_TEST_P(SingleClientBookmarksSyncTestWithEnabledReuploadTitles,
-                       ShouldReuploadFullTitleAfterRestartOnIncrementalChange) {
+IN_PROC_BROWSER_TEST_P(
+    SingleClientBookmarksSyncTestWithEnabledReuploadRemoteBookmarks,
+    ShouldReuploadFullTitleAfterRestartOnIncrementalChange) {
   ASSERT_TRUE(SetupClients());
 #if defined(OS_CHROMEOS)
   // signin::SetRefreshTokenForPrimaryAccount() is needed on ChromeOS in order
@@ -1209,7 +1288,14 @@ IN_PROC_BROWSER_TEST_P(SingleClientBookmarksSyncTestWithEnabledReuploadTitles,
 INSTANTIATE_TEST_SUITE_P(All,
                          SingleClientBookmarksSyncTest,
                          ::testing::Values(false, true));
-INSTANTIATE_TEST_SUITE_P(All,
-                         SingleClientBookmarksSyncTestWithEnabledReuploadTitles,
-                         ::testing::Values(false, true));
+// TODO(crbug.com/1067191): remove feature toggle.
+INSTANTIATE_TEST_SUITE_P(
+    All,
+    SingleClientBookmarksSyncTestWithEnabledReuploadRemoteBookmarks,
+    ::testing::Values(false, true));
+INSTANTIATE_TEST_SUITE_P(
+    All,
+    SingleClientBookmarksSyncTestWithEnabledReuploadPreexistingBookmarks,
+    ::testing::Values(false, true));
+
 }  // namespace

components/sync/protocol/bookmark_model_metadata.proto

@@ -31,4 +31,12 @@ message BookmarkModelMetadata {
 
   // A set of all bookmarks metadata.
   repeated BookmarkMetadata bookmarks_metadata = 2;
+
+  // Indicates whether the reupload of bookmarks has been triggered such that
+  // they include the full title, which means that their sequence number has
+  // been increased (independently of whether the commit has succeeded or even
+  // started).
+  // TODO(crbug.com/1066962): remove this code when most of bookmarks are
+  // reuploaded.
+  optional bool bookmarks_full_title_reuploaded = 3;
 }

components/sync_bookmarks/bookmark_local_changes_builder.cc

@@ -98,6 +98,21 @@ syncer::CommitRequestDataList BookmarkLocalChangesBuilder::BuildCommitRequests(
     bookmark_tracker_->MarkCommitMayHaveStarted(entity);
 
     commit_requests.push_back(std::move(request));
+
+    // This codepath prevents permanently staying server-side bookmarks without
+    // favicons due to an automatically-triggered upload. As far as favicon is
+    // loaded the bookmark will be committed again.
+    if (!metadata->is_deleted()) {
+      const bookmarks::BookmarkNode* node = entity->bookmark_node();
+      DCHECK(node);
+
+      if (!node->is_permanent_node() && !node->is_folder() &&
+          !node->is_favicon_loaded() &&
+          base::FeatureList::IsEnabled(
+              switches::kSyncReuploadBookmarkFullTitles)) {
+        bookmark_tracker_->IncrementSequenceNumber(entity);
+      }
+    }
   }
   return commit_requests;
 }

components/sync_bookmarks/bookmark_model_type_processor.cc

@@ -31,6 +31,7 @@
 #include "components/sync_bookmarks/bookmark_model_observer_impl.h"
 #include "components/sync_bookmarks/bookmark_remote_updates_handler.h"
 #include "components/sync_bookmarks/bookmark_specifics_conversions.h"
+#include "components/sync_bookmarks/switches.h"
 #include "components/undo/bookmark_undo_utils.h"
 
 namespace sync_bookmarks {

components/sync_bookmarks/bookmark_model_type_processor_unittest.cc

@@ -229,6 +229,14 @@ class BookmarkModelTypeProcessorTest : public testing::Test {
     return &schedule_save_closure_;
   }
 
+  sync_pb::BookmarkModelMetadata BuildBookmarkModelMetadataWithoutFullTitles() {
+    base::test::ScopedFeatureList features;
+    features.InitAndDisableFeature(switches::kSyncReuploadBookmarkFullTitles);
+    sync_pb::BookmarkModelMetadata model_metadata =
+        processor()->GetTrackerForTest()->BuildBookmarkModelMetadata();
+    return model_metadata;
+  }
+
  private:
   base::test::TaskEnvironment task_environment_;
   NiceMock<base::MockCallback<base::RepeatingClosure>> schedule_save_closure_;
@@ -672,6 +680,48 @@ TEST_F(BookmarkModelTypeProcessorTest,
   EXPECT_FALSE(callback_result.empty());
 }
 
+TEST_F(BookmarkModelTypeProcessorTest, ShouldReuploadLegacyBookmarksOnStart) {
+  const std::string kNodeId = "node_id";
+  const std::string kTitle = "title";
+  const std::string kUrl = "http://www.url.com";
+
+  std::vector<BookmarkInfo> bookmarks = {
+      {kNodeId, kTitle, kUrl, kBookmarkBarId, /*server_tag=*/std::string()}};
+
+  SimulateModelReadyToSync();
+  SimulateOnSyncStarting();
+  InitWithSyncedBookmarks(bookmarks, processor());
+
+  sync_pb::BookmarkModelMetadata model_metadata =
+      BuildBookmarkModelMetadataWithoutFullTitles();
+  // Ensure that bookmark is legacy.
+  ASSERT_FALSE(model_metadata.bookmarks_full_title_reuploaded());
+  ASSERT_TRUE(processor()
+                  ->GetTrackerForTest()
+                  ->GetEntitiesWithLocalChanges(/*max_entries=*/1)
+                  .empty());
+
+  base::test::ScopedFeatureList features;
+  features.InitAndEnableFeature(switches::kSyncReuploadBookmarkFullTitles);
+  BookmarkModelTypeProcessor new_processor(bookmark_undo_service());
+
+  std::string metadata_str;
+  model_metadata.SerializeToString(&metadata_str);
+  new_processor.ModelReadyToSync(metadata_str, base::DoNothing(),
+                                 bookmark_model());
+
+  // Check that all entities are unsynced now and metadata is marked as
+  // reuploaded.
+  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());
+}
+
 }  // namespace
 
 }  // namespace sync_bookmarks

components/sync_bookmarks/synced_bookmark_tracker.cc

@@ -21,6 +21,7 @@
 #include "components/sync/base/unique_position.h"
 #include "components/sync/model/entity_data.h"
 #include "components/sync/protocol/proto_memory_estimations.h"
+#include "components/sync_bookmarks/switches.h"
 #include "ui/base/models/tree_node_iterator.h"
 
 namespace sync_bookmarks {
@@ -151,8 +152,8 @@ size_t SyncedBookmarkTracker::Entity::EstimateMemoryUsage() const {
 std::unique_ptr<SyncedBookmarkTracker> SyncedBookmarkTracker::CreateEmpty(
     sync_pb::ModelTypeState model_type_state) {
   // base::WrapUnique() used because the constructor is private.
-  auto tracker =
-      base::WrapUnique(new SyncedBookmarkTracker(std::move(model_type_state)));
+  auto tracker = base::WrapUnique(new SyncedBookmarkTracker(
+      std::move(model_type_state), /*bookmarks_full_title_reuploaded=*/false));
   return tracker;
 }
 
@@ -167,8 +168,12 @@ SyncedBookmarkTracker::CreateFromBookmarkModelAndMetadata(
     return nullptr;
   }
 
-  auto tracker =
-      CreateEmpty(std::move(*model_metadata.mutable_model_type_state()));
+  const bool bookmarks_full_title_reuploaded =
+      model_metadata.bookmarks_full_title_reuploaded();
+
+  auto tracker = base::WrapUnique(new SyncedBookmarkTracker(
+      std::move(*model_metadata.mutable_model_type_state()),
+      bookmarks_full_title_reuploaded));
 
   const CorruptionReason corruption_reason =
       tracker->InitEntitiesFromModelAndMetadata(model,
@@ -181,6 +186,8 @@ SyncedBookmarkTracker::CreateFromBookmarkModelAndMetadata(
     return nullptr;
   }
 
+  tracker->ReuploadBookmarksOnLoadIfNeeded();
+
   return tracker;
 }
 
@@ -327,6 +334,8 @@ void SyncedBookmarkTracker::IncrementSequenceNumber(const Entity* entity) {
 sync_pb::BookmarkModelMetadata
 SyncedBookmarkTracker::BuildBookmarkModelMetadata() const {
   sync_pb::BookmarkModelMetadata model_metadata;
+  model_metadata.set_bookmarks_full_title_reuploaded(
+      bookmarks_full_title_reuploaded_);
   for (const std::pair<const std::string, std::unique_ptr<Entity>>& pair :
        sync_id_to_entities_map_) {
     DCHECK(pair.second) << " for ID " << pair.first;
@@ -413,8 +422,10 @@ SyncedBookmarkTracker::GetEntitiesWithLocalChanges(size_t max_entries) const {
 }
 
 SyncedBookmarkTracker::SyncedBookmarkTracker(
-    sync_pb::ModelTypeState model_type_state)
-    : model_type_state_(std::move(model_type_state)) {}
+    sync_pb::ModelTypeState model_type_state,
+    bool bookmarks_full_title_reuploaded)
+    : model_type_state_(std::move(model_type_state)),
+      bookmarks_full_title_reuploaded_(bookmarks_full_title_reuploaded) {}
 
 SyncedBookmarkTracker::CorruptionReason
 SyncedBookmarkTracker::InitEntitiesFromModelAndMetadata(
@@ -578,6 +589,26 @@ SyncedBookmarkTracker::ReorderUnsyncedEntitiesExceptDeletions(
   return ordered_entities;
 }
 
+void SyncedBookmarkTracker::ReuploadBookmarksOnLoadIfNeeded() {
+  if (bookmarks_full_title_reuploaded_ ||
+      !base::FeatureList::IsEnabled(
+          switches::kSyncReuploadBookmarkFullTitles)) {
+    return;
+  }
+  for (const auto& sync_id_and_entity : sync_id_to_entities_map_) {
+    const SyncedBookmarkTracker::Entity* entity =
+        sync_id_and_entity.second.get();
+    if (entity->IsUnsynced() || entity->metadata()->is_deleted()) {
+      continue;
+    }
+    if (entity->bookmark_node()->is_permanent_node()) {
+      continue;
+    }
+    IncrementSequenceNumber(entity);
+  }
+  bookmarks_full_title_reuploaded_ = true;
+}
+
 void SyncedBookmarkTracker::TraverseAndAppend(
     const bookmarks::BookmarkNode* node,
     std::vector<const SyncedBookmarkTracker::Entity*>* ordered_entities) const {

components/sync_bookmarks/synced_bookmark_tracker.h

@@ -275,7 +275,8 @@ class SyncedBookmarkTracker {
     kMaxValue = BOOKMARK_GUID_MISMATCH
   };
 
-  explicit SyncedBookmarkTracker(sync_pb::ModelTypeState model_type_state);
+  SyncedBookmarkTracker(sync_pb::ModelTypeState model_type_state,
+                        bool bookmarks_full_title_reuploaded);
 
   // Add entities to |this| tracker based on the content of |*model| and
   // |model_metadata|. Validates the integrity of |*model| and |model_metadata|
@@ -294,6 +295,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,
@@ -319,6 +330,12 @@ class SyncedBookmarkTracker {
   // The model metadata (progress marker, initial sync done, etc).
   sync_pb::ModelTypeState model_type_state_;
 
+  // This field contains the value of
+  // BookmarksMetadata::bookmarks_full_title_reuploaded.
+  // TODO(crbug.com/1066962): remove this code when most of bookmarks are
+  // reuploaded.
+  bool bookmarks_full_title_reuploaded_ = false;
+
   DISALLOW_COPY_AND_ASSIGN(SyncedBookmarkTracker);
 };