[Sync::USS] Fix missing bookmarks titles created from legacy entities

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

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

@@ -495,6 +495,23 @@ IN_PROC_BROWSER_TEST_P(SingleClientBookmarksSyncTest, DownloadBookmarkFolder) {
   ASSERT_EQ(1, CountFoldersWithTitlesMatching(kSingleProfileIndex, title));
 }
 
+IN_PROC_BROWSER_TEST_P(SingleClientBookmarksSyncTest,
+                       DownloadLegacyBookmarkFolder) {
+  const std::string title = "Seattle Sounders FC";
+  fake_server::EntityBuilderFactory entity_builder_factory;
+  fake_server::BookmarkEntityBuilder bookmark_builder =
+      entity_builder_factory.NewBookmarkEntityBuilder(title);
+  fake_server_->InjectEntity(bookmark_builder.BuildFolder(/*is_legacy=*/true));
+
+  DisableVerifier();
+  ASSERT_TRUE(SetupClients());
+  ASSERT_EQ(0, CountFoldersWithTitlesMatching(kSingleProfileIndex, title));
+
+  ASSERT_TRUE(SetupSync());
+
+  ASSERT_EQ(1, CountFoldersWithTitlesMatching(kSingleProfileIndex, title));
+}
+
 // Legacy bookmark clients append a blank space to empty titles, ".", ".." tiles
 // before committing them because historically they were illegal server titles.
 // This test makes sure that this functionality is implemented for backward

components/sync/engine_impl/model_type_worker.cc

@@ -209,8 +209,8 @@ SyncerError ModelTypeWorker::ProcessGetUpdatesResponse(
     }
 
     auto response_data = std::make_unique<UpdateResponseData>();
-    switch (PopulateUpdateResponseData(cryptographer_.get(), *update_entity,
-                                       response_data.get())) {
+    switch (PopulateUpdateResponseData(cryptographer_.get(), type_,
+                                       *update_entity, response_data.get())) {
       case SUCCESS:
         if (!response_data->entity->client_tag_hash.empty()) {
           client_tag_hashes.push_back(response_data->entity->client_tag_hash);
@@ -240,6 +240,7 @@ SyncerError ModelTypeWorker::ProcessGetUpdatesResponse(
 // |response_data| must be not null.
 ModelTypeWorker::DecryptionStatus ModelTypeWorker::PopulateUpdateResponseData(
     const Cryptographer* cryptographer,
+    ModelType model_type,
     const sync_pb::SyncEntity& update_entity,
     UpdateResponseData* response_data) {
   response_data->response_version = update_entity.version();
@@ -342,6 +343,12 @@ ModelTypeWorker::DecryptionStatus ModelTypeWorker::PopulateUpdateResponseData(
   if (!specifics.has_encrypted()) {
     // No encryption.
     data->specifics = specifics;
+    // Legacy clients populates the name field in the SyncEntity instead of the
+    // title field in the BookmarkSpecifics.
+    if (model_type == BOOKMARKS && !specifics.bookmark().has_title() &&
+        !update_entity.name().empty()) {
+      data->specifics.mutable_bookmark()->set_title(update_entity.name());
+    }
     response_data->entity = std::move(data);
     return SUCCESS;
   }

components/sync/engine_impl/model_type_worker.h

@@ -79,6 +79,7 @@ class ModelTypeWorker : public UpdateHandler,
   // |response_data| must be not null.
   static DecryptionStatus PopulateUpdateResponseData(
       const Cryptographer* cryptographer,
+      ModelType model_type,
       const sync_pb::SyncEntity& update_entity,
       UpdateResponseData* response_data);
 

components/sync/engine_impl/model_type_worker_unittest.cc

@@ -1463,8 +1463,8 @@ TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseData) {
   base::HistogramTester histogram_tester;
 
   EXPECT_EQ(ModelTypeWorker::SUCCESS,
-            ModelTypeWorker::PopulateUpdateResponseData(&cryptographer, entity,
-                                                        &response_data));
+            ModelTypeWorker::PopulateUpdateResponseData(
+                &cryptographer, PREFERENCES, entity, &response_data));
   const EntityData& data = *response_data.entity;
   EXPECT_FALSE(data.id.empty());
   EXPECT_FALSE(data.parent_id.empty());
@@ -1499,8 +1499,8 @@ TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseDataWithPositionInParent) {
   base::HistogramTester histogram_tester;
 
   EXPECT_EQ(ModelTypeWorker::SUCCESS,
-            ModelTypeWorker::PopulateUpdateResponseData(&cryptographer, entity,
-                                                        &response_data));
+            ModelTypeWorker::PopulateUpdateResponseData(
+                &cryptographer, PREFERENCES, entity, &response_data));
   const EntityData& data = *response_data.entity;
   EXPECT_TRUE(
       syncer::UniquePosition::FromProto(data.unique_position).IsValid());
@@ -1527,8 +1527,8 @@ TEST_F(ModelTypeWorkerTest, PopulateUpdateResponseDataWithInsertAfterItemId) {
   base::HistogramTester histogram_tester;
 
   EXPECT_EQ(ModelTypeWorker::SUCCESS,
-            ModelTypeWorker::PopulateUpdateResponseData(&cryptographer, entity,
-                                                        &response_data));
+            ModelTypeWorker::PopulateUpdateResponseData(
+                &cryptographer, PREFERENCES, entity, &response_data));
   const EntityData& data = *response_data.entity;
   EXPECT_TRUE(
       syncer::UniquePosition::FromProto(data.unique_position).IsValid());
@@ -1557,8 +1557,8 @@ TEST_F(ModelTypeWorkerTest,
   base::HistogramTester histogram_tester;
 
   EXPECT_EQ(ModelTypeWorker::SUCCESS,
-            ModelTypeWorker::PopulateUpdateResponseData(&cryptographer, entity,
-                                                        &response_data));
+            ModelTypeWorker::PopulateUpdateResponseData(
+                &cryptographer, PREFERENCES, entity, &response_data));
   const EntityData& data = *response_data.entity;
   EXPECT_FALSE(
       syncer::UniquePosition::FromProto(data.unique_position).IsValid());
@@ -1582,8 +1582,8 @@ TEST_F(ModelTypeWorkerTest,
   base::HistogramTester histogram_tester;
 
   EXPECT_EQ(ModelTypeWorker::SUCCESS,
-            ModelTypeWorker::PopulateUpdateResponseData(&cryptographer, entity,
-                                                        &response_data));
+            ModelTypeWorker::PopulateUpdateResponseData(
+                &cryptographer, PREFERENCES, entity, &response_data));
   const EntityData& data = *response_data.entity;
   EXPECT_FALSE(
       syncer::UniquePosition::FromProto(data.unique_position).IsValid());

components/sync/test/fake_server/bookmark_entity_builder.cc

@@ -50,28 +50,33 @@ void BookmarkEntityBuilder::SetIndex(int index) {
 }
 
 std::unique_ptr<LoopbackServerEntity> BookmarkEntityBuilder::BuildBookmark(
-    const GURL& url) {
+    const GURL& url,
+    bool is_legacy) {
   if (!url.is_valid()) {
     return base::WrapUnique<LoopbackServerEntity>(nullptr);
   }
 
-  sync_pb::EntitySpecifics entity_specifics = CreateBaseEntitySpecifics();
+  sync_pb::EntitySpecifics entity_specifics =
+      CreateBaseEntitySpecifics(is_legacy);
   entity_specifics.mutable_bookmark()->set_url(url.spec());
   const bool kIsNotFolder = false;
   return Build(entity_specifics, kIsNotFolder);
 }
 
-std::unique_ptr<LoopbackServerEntity> BookmarkEntityBuilder::BuildFolder() {
+std::unique_ptr<LoopbackServerEntity> BookmarkEntityBuilder::BuildFolder(
+    bool is_legacy) {
   const bool kIsFolder = true;
-  return Build(CreateBaseEntitySpecifics(), kIsFolder);
+  return Build(CreateBaseEntitySpecifics(is_legacy), kIsFolder);
 }
 
-sync_pb::EntitySpecifics BookmarkEntityBuilder::CreateBaseEntitySpecifics()
-    const {
+sync_pb::EntitySpecifics BookmarkEntityBuilder::CreateBaseEntitySpecifics(
+    bool is_legacy) const {
   sync_pb::EntitySpecifics entity_specifics;
   sync_pb::BookmarkSpecifics* bookmark_specifics =
       entity_specifics.mutable_bookmark();
+  if (!is_legacy) {
     bookmark_specifics->set_title(title_);
+  }
 
   return entity_specifics;
 }

components/sync/test/fake_server/bookmark_entity_builder.h

@@ -35,16 +35,21 @@ class BookmarkEntityBuilder {
 
   // Builds and returns a LoopbackServerEntity representing a bookmark. Returns
   // null if the entity could not be built.
-  std::unique_ptr<syncer::LoopbackServerEntity> BuildBookmark(const GURL& url);
+  std::unique_ptr<syncer::LoopbackServerEntity> BuildBookmark(
+      const GURL& url,
+      bool is_legacy = false);
 
   // Builds and returns a LoopbackServerEntity representing a bookmark folder.
   // Returns null if the entity could not be built.
-  std::unique_ptr<syncer::LoopbackServerEntity> BuildFolder();
+  std::unique_ptr<syncer::LoopbackServerEntity> BuildFolder(
+      bool is_legacy = false);
 
  private:
-  // Creates an EntitySpecifics and pre-populates its BookmarkSpecifics with
-  // the entity's title.
-  sync_pb::EntitySpecifics CreateBaseEntitySpecifics() const;
+  // Creates an EntitySpecifics and pre-populates its BookmarkSpecifics with the
+  // entity's title if |is_legacy| is false. Otherwise, it doesn't populate the
+  // title.
+  sync_pb::EntitySpecifics CreateBaseEntitySpecifics(
+      bool is_legacy = false) const;
 
   // Builds the parts of a LoopbackServerEntity common to both normal bookmarks
   // and folders.