[Sync] Reupload new title field for bookmarks.

When the client receives BookmarkSpecifics with a legacy title field
only it will fill in new field and commit changed data to the server
again in future. Reupload of updated specifics may be done after
receiving of initial merge data or after browser restart.

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

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

@@ -24,6 +24,7 @@
 #include "components/sync/test/fake_server/bookmark_entity_builder.h"
 #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 "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/layout.h"
@@ -98,6 +99,17 @@ void SingleClientBookmarksSyncTest::VerifyBookmarkModelMatchesFakeServer(
   }
 }
 
+class SingleClientBookmarksSyncTestWithEnabledReuploadTitles
+    : public SingleClientBookmarksSyncTest {
+ public:
+  SingleClientBookmarksSyncTestWithEnabledReuploadTitles() {
+    features_.InitAndEnableFeature(switches::kSyncReuploadBookmarkFullTitles);
+  }
+
+ private:
+  base::test::ScopedFeatureList features_;
+};
+
 IN_PROC_BROWSER_TEST_P(SingleClientBookmarksSyncTest, Sanity) {
   ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
 
@@ -1117,7 +1129,87 @@ IN_PROC_BROWSER_TEST_P(SingleClientBookmarksSyncTest,
   EXPECT_EQ(1u, GetBookmarkBarNode(kSingleProfileIndex)->children().size());
 }
 
+IN_PROC_BROWSER_TEST_P(SingleClientBookmarksSyncTestWithEnabledReuploadTitles,
+                       ShouldReuploadFullTitleAfterInitialMerge) {
+  ASSERT_TRUE(SetupClients());
+
+  const std::string title = "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.BuildFolder(/*is_legacy=*/false);
+  const std::string new_guid = remote_folder->GetSpecifics().bookmark().guid();
+  ASSERT_FALSE(remote_folder->GetSpecifics().bookmark().has_full_title());
+  fake_server_->InjectEntity(std::move(remote_folder));
+
+  ASSERT_TRUE(SetupSync());
+  ASSERT_TRUE(
+      UpdatedProgressMarkerChecker(GetSyncService(kSingleProfileIndex)).Wait());
+
+  const std::vector<sync_pb::SyncEntity> server_bookmarks =
+      GetFakeServer()->GetSyncEntitiesByModelType(syncer::BOOKMARKS);
+  ASSERT_EQ(1u, CountFoldersWithTitlesMatching(kSingleProfileIndex, title));
+  ASSERT_EQ(1u, server_bookmarks.size());
+
+  EXPECT_TRUE(server_bookmarks.front().specifics().bookmark().has_full_title());
+}
+
+IN_PROC_BROWSER_TEST_P(
+    SingleClientBookmarksSyncTestWithEnabledReuploadTitles,
+    PRE_ShouldReuploadFullTitleAfterRestartOnIncrementalChange) {
+  ASSERT_TRUE(SetupSync());
+
+  const std::string title = "Title";
+  const GURL url = GURL("http://www.foo.com");
+
+  // Make an incremental remote creation of bookmark.
+  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();
+  ASSERT_FALSE(remote_folder->GetSpecifics().bookmark().has_full_title());
+  fake_server_->InjectEntity(std::move(remote_folder));
+
+  ASSERT_TRUE(BookmarksTitleChecker(kSingleProfileIndex, title,
+                                    /*expected_count=*/1)
+                  .Wait());
+
+  // Check that the full title was not uploaded to the server yet.
+  const std::vector<sync_pb::SyncEntity> server_bookmarks =
+      GetFakeServer()->GetSyncEntitiesByModelType(syncer::BOOKMARKS);
+  ASSERT_EQ(1u, server_bookmarks.size());
+  EXPECT_FALSE(
+      server_bookmarks.front().specifics().bookmark().has_full_title());
+}
+
+IN_PROC_BROWSER_TEST_P(SingleClientBookmarksSyncTestWithEnabledReuploadTitles,
+                       ShouldReuploadFullTitleAfterRestartOnIncrementalChange) {
+  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());
+
+  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());
+}
+
 INSTANTIATE_TEST_SUITE_P(All,
                          SingleClientBookmarksSyncTest,
                          ::testing::Values(false, true));
+INSTANTIATE_TEST_SUITE_P(All,
+                         SingleClientBookmarksSyncTestWithEnabledReuploadTitles,
+                         ::testing::Values(false, true));
 }  // namespace

components/sync_bookmarks/bookmark_model_merger.cc

@@ -422,10 +422,14 @@ void BookmarkModelMerger::MergeSubtree(
     const bookmarks::BookmarkNode* local_subtree_root,
     const RemoteTreeNode& remote_node) {
   const EntityData& remote_update_entity = remote_node.entity();
-  bookmark_tracker_->Add(
+  const SyncedBookmarkTracker::Entity* entity = bookmark_tracker_->Add(
       local_subtree_root, remote_update_entity.id,
       remote_node.response_version(), remote_update_entity.creation_time,
       remote_update_entity.unique_position, remote_update_entity.specifics);
+  if (!local_subtree_root->is_permanent_node() &&
+      IsFullTitleReuploadNeeded(remote_update_entity.specifics.bookmark())) {
+    bookmark_tracker_->IncrementSequenceNumber(entity);
+  }
 
   // If there are remote child updates, try to match them.
   for (size_t remote_index = 0; remote_index < remote_node.children().size();
@@ -568,10 +572,13 @@ void BookmarkModelMerger::ProcessRemoteCreation(
                                       remote_update_entity.is_folder,
                                       bookmark_model_, favicon_service_);
   DCHECK(bookmark_node);
-  bookmark_tracker_->Add(bookmark_node, remote_update_entity.id,
-                         remote_node.response_version(),
-                         remote_update_entity.creation_time,
-                         remote_update_entity.unique_position, specifics);
+  const SyncedBookmarkTracker::Entity* entity = bookmark_tracker_->Add(
+      bookmark_node, remote_update_entity.id, remote_node.response_version(),
+      remote_update_entity.creation_time, remote_update_entity.unique_position,
+      specifics);
+  if (IsFullTitleReuploadNeeded(specifics.bookmark())) {
+    bookmark_tracker_->IncrementSequenceNumber(entity);
+  }
 
   // Recursively, match by GUID or, if not possible, create local node for all
   // child remote nodes.

components/sync_bookmarks/bookmark_remote_updates_handler.cc

@@ -501,10 +501,12 @@ BookmarkRemoteUpdatesHandler::ProcessCreate(
                                 bookmark_tracker_),
           update_entity.is_folder, bookmark_model_, favicon_service_);
   DCHECK(bookmark_node);
-  return bookmark_tracker_->Add(
+  const SyncedBookmarkTracker::Entity* entity = bookmark_tracker_->Add(
       bookmark_node, update_entity.id, update.response_version,
       update_entity.creation_time, update_entity.unique_position,
       update_entity.specifics);
+  ReuploadEntityIfNeeded(update_entity.specifics.bookmark(), entity);
+  return entity;
 }
 
 void BookmarkRemoteUpdatesHandler::ProcessUpdate(
@@ -560,6 +562,7 @@ void BookmarkRemoteUpdatesHandler::ProcessUpdate(
   }
   ApplyRemoteUpdate(update, tracked_entity, new_parent_entity, bookmark_model_,
                     bookmark_tracker_, favicon_service_);
+  ReuploadEntityIfNeeded(update_entity.specifics.bookmark(), tracked_entity);
 }
 
 void BookmarkRemoteUpdatesHandler::ProcessDelete(
@@ -676,14 +679,14 @@ void BookmarkRemoteUpdatesHandler::ProcessConflict(
 
     // The changes are identical so there isn't a real conflict.
     DLOG(WARNING) << "Conflict: CHANGES_MATCH";
-    return;
+  } else {
+    // Conflict where data don't match and no remote deletion, and hence server
+    // wins. Update the model from server data.
+    DLOG(WARNING) << "Conflict: USE_REMOTE";
+    ApplyRemoteUpdate(update, tracked_entity, new_parent_entity,
+                      bookmark_model_, bookmark_tracker_, favicon_service_);
   }
-
-  // Conflict where data don't match and no remote deletion, and hence server
-  // wins. Update the model from server data.
-  DLOG(WARNING) << "Conflict: USE_REMOTE";
-  ApplyRemoteUpdate(update, tracked_entity, new_parent_entity, bookmark_model_,
-                    bookmark_tracker_, favicon_service_);
+  ReuploadEntityIfNeeded(update_entity.specifics.bookmark(), tracked_entity);
 }
 
 void BookmarkRemoteUpdatesHandler::RemoveEntityAndChildrenFromTracker(
@@ -707,4 +710,13 @@ const bookmarks::BookmarkNode* BookmarkRemoteUpdatesHandler::GetParentNode(
   return parent_entity->bookmark_node();
 }
 
+void BookmarkRemoteUpdatesHandler::ReuploadEntityIfNeeded(
+    const sync_pb::BookmarkSpecifics& specifics,
+    const SyncedBookmarkTracker::Entity* tracked_entity) {
+  if (!IsFullTitleReuploadNeeded(specifics)) {
+    return;
+  }
+  bookmark_tracker_->IncrementSequenceNumber(tracked_entity);
+}
+
 }  // namespace sync_bookmarks

components/sync_bookmarks/bookmark_remote_updates_handler.h

@@ -98,6 +98,10 @@ class BookmarkRemoteUpdatesHandler {
   // from |bookmark_tracker_|.
   void RemoveEntityAndChildrenFromTracker(const bookmarks::BookmarkNode* node);
 
+  void ReuploadEntityIfNeeded(
+      const sync_pb::BookmarkSpecifics& specifics,
+      const SyncedBookmarkTracker::Entity* tracked_entity);
+
   bookmarks::BookmarkModel* const bookmark_model_;
   favicon::FaviconService* const favicon_service_;
   SyncedBookmarkTracker* const bookmark_tracker_;

components/sync_bookmarks/bookmark_remote_updates_handler_unittest.cc

@@ -12,6 +12,7 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/metrics/histogram_tester.h"
+#include "base/test/scoped_feature_list.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/bookmarks/test/test_bookmark_client.h"
 #include "components/favicon/core/test/mock_favicon_service.h"
@@ -22,6 +23,7 @@
 #include "components/sync/model/conflict_resolution.h"
 #include "components/sync/protocol/unique_position.pb.h"
 #include "components/sync_bookmarks/bookmark_model_merger.h"
+#include "components/sync_bookmarks/switches.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -1520,6 +1522,127 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
               Eq(ASCIIToUTF16(kNewRemoteTitle)));
 }
 
+TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
+       ShouldIncrementSequenceNumberOnConflict) {
+  base::test::ScopedFeatureList override_features;
+  override_features.InitAndEnableFeature(
+      switches::kSyncReuploadBookmarkFullTitles);
+
+  const std::string kId = "id";
+  const std::string kGuid = base::GenerateGUID();
+  const std::string kTitle = "title";
+  const std::string kNewTitle = "New title";
+
+  syncer::UpdateResponseDataList updates;
+  updates.push_back(CreateUpdateResponseData(
+      /*server_id=*/kId,
+      /*parent_id=*/kBookmarkBarId,
+      /*guid=*/kGuid,
+      /*title=*/kTitle,
+      /*is_deletion=*/false,
+      /*version=*/0,
+      /*unique_position=*/
+      syncer::UniquePosition::InitialPosition(
+          syncer::UniquePosition::RandomSuffix())));
+  updates.back().entity.specifics.mutable_bookmark()->clear_full_title();
+
+  {
+    BookmarkRemoteUpdatesHandler updates_handler(bookmark_model(),
+                                                 favicon_service(), tracker());
+    updates_handler.Process(updates, /*got_new_encryption_requirements=*/false);
+  }
+
+  const SyncedBookmarkTracker::Entity* entity =
+      tracker()->GetEntityForSyncId(kId);
+  ASSERT_THAT(entity, NotNull());
+  EXPECT_TRUE(entity->IsUnsynced());
+
+  // Check reupload on conflict with new title.
+  updates.back()
+      .entity.specifics.mutable_bookmark()
+      ->set_legacy_canonicalized_title(kNewTitle);
+  updates.back().response_version++;
+  {
+    BookmarkRemoteUpdatesHandler updates_handler(bookmark_model(),
+                                                 favicon_service(), tracker());
+    updates_handler.Process(updates, /*got_new_encryption_requirements=*/false);
+  }
+  const bookmarks::BookmarkNode* bookmark_bar_node =
+      bookmark_model()->bookmark_bar_node();
+  ASSERT_EQ(1u, bookmark_bar_node->children().size());
+  const bookmarks::BookmarkNode* node =
+      bookmark_bar_node->children().front().get();
+  EXPECT_EQ(base::UTF16ToUTF8(node->GetTitle()), kNewTitle);
+  EXPECT_TRUE(entity->IsUnsynced());
+
+  // Check reupload on conflict when specifics matches.
+  updates.back().response_version++;
+  {
+    BookmarkRemoteUpdatesHandler updates_handler(bookmark_model(),
+                                                 favicon_service(), tracker());
+    updates_handler.Process(updates, /*got_new_encryption_requirements=*/false);
+  }
+  ASSERT_EQ(1u, bookmark_bar_node->children().size());
+  EXPECT_EQ(bookmark_bar_node->children().front().get(), node);
+  EXPECT_EQ(base::UTF16ToUTF8(node->GetTitle()), kNewTitle);
+  EXPECT_TRUE(entity->IsUnsynced());
+}
+
+TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
+       ShouldIncrementSequenceNumberOnUpdate) {
+  base::test::ScopedFeatureList override_features;
+  override_features.InitAndEnableFeature(
+      switches::kSyncReuploadBookmarkFullTitles);
+
+  const std::string kId = "id";
+  const std::string kGuid = base::GenerateGUID();
+  const std::string kTitle = "title";
+  const std::string kRemoteTitle = "New title";
+
+  syncer::UpdateResponseDataList updates;
+  updates.push_back(CreateUpdateResponseData(
+      /*server_id=*/kId,
+      /*parent_id=*/kBookmarkBarId,
+      /*guid=*/kGuid,
+      /*title=*/kTitle,
+      /*is_deletion=*/false,
+      /*version=*/0,
+      /*unique_position=*/
+      syncer::UniquePosition::InitialPosition(
+          syncer::UniquePosition::RandomSuffix())));
+  updates.back().entity.specifics.mutable_bookmark()->set_full_title(kTitle);
+
+  {
+    BookmarkRemoteUpdatesHandler updates_handler(bookmark_model(),
+                                                 favicon_service(), tracker());
+    updates_handler.Process(updates, /*got_new_encryption_requirements=*/false);
+  }
+
+  const SyncedBookmarkTracker::Entity* entity =
+      tracker()->GetEntityForSyncId(kId);
+  ASSERT_THAT(entity, NotNull());
+  ASSERT_FALSE(entity->IsUnsynced());
+
+  // Check reupload on conflict.
+  updates.back()
+      .entity.specifics.mutable_bookmark()
+      ->set_legacy_canonicalized_title(kRemoteTitle);
+  updates.back().entity.specifics.mutable_bookmark()->clear_full_title();
+  updates.back().response_version++;
+  {
+    BookmarkRemoteUpdatesHandler updates_handler(bookmark_model(),
+                                                 favicon_service(), tracker());
+    updates_handler.Process(updates, /*got_new_encryption_requirements=*/false);
+  }
+  const bookmarks::BookmarkNode* bookmark_bar_node =
+      bookmark_model()->bookmark_bar_node();
+  ASSERT_EQ(1u, bookmark_bar_node->children().size());
+  const bookmarks::BookmarkNode* node =
+      bookmark_bar_node->children().front().get();
+  EXPECT_EQ(node->GetTitle(), base::UTF8ToUTF16(kRemoteTitle));
+  EXPECT_TRUE(entity->IsUnsynced());
+}
+
 }  // namespace
 
 }  // namespace sync_bookmarks

components/sync_bookmarks/bookmark_specifics_conversions.cc

@@ -185,6 +185,14 @@ std::string FullTitleToLegacyCanonicalizedTitle(const std::string& node_title) {
   return specifics_title;
 }
 
+bool IsFullTitleReuploadNeeded(const sync_pb::BookmarkSpecifics& specifics) {
+  if (specifics.has_full_title()) {
+    return false;
+  }
+  return base::FeatureList::IsEnabled(
+      switches::kSyncReuploadBookmarkFullTitles);
+}
+
 sync_pb::EntitySpecifics CreateSpecificsFromBookmarkNode(
     const bookmarks::BookmarkNode* node,
     bookmarks::BookmarkModel* model,

components/sync_bookmarks/bookmark_specifics_conversions.h

@@ -28,6 +28,10 @@ namespace sync_bookmarks {
 // truncating and the appending ' ' in some cases.
 std::string FullTitleToLegacyCanonicalizedTitle(const std::string& node_title);
 
+// Used to decide if entity needs to be reuploaded for each remote change which
+// is true if the proto field for the full title is missing.
+bool IsFullTitleReuploadNeeded(const sync_pb::BookmarkSpecifics& specifics);
+
 // TODO(crbug.com/978430): Remove argument |include_guid| once the client tag
 // hash is required to be populated during sync metadata validation upon
 // startup in SyncedBookmarkTracker::BookmarkModelMatchesMetadata().

components/sync_bookmarks/switches.cc

@@ -18,4 +18,7 @@ const base::Feature kUpdateBookmarkGUIDWithNodeReplacement{
 const base::Feature kMergeBookmarksUsingGUIDs{
     "MergeBookmarksUsingGUIDs", base::FEATURE_DISABLED_BY_DEFAULT};
 
+const base::Feature kSyncReuploadBookmarkFullTitles{
+    "SyncReuploadBookmarkFullTitles", base::FEATURE_DISABLED_BY_DEFAULT};
+
 }  // namespace switches

components/sync_bookmarks/switches.h

@@ -12,6 +12,9 @@ namespace switches {
 extern const base::Feature kSyncDoNotCommitBookmarksWithoutFavicon;
 extern const base::Feature kUpdateBookmarkGUIDWithNodeReplacement;
 extern const base::Feature kMergeBookmarksUsingGUIDs;
+// TODO(crbug.com/1066962): remove this code when most of bookmarks are
+// reuploaded.
+extern const base::Feature kSyncReuploadBookmarkFullTitles;
 
 }  // namespace switches