Remove modification timestamp from SyncData

The legacy (pre-USS) implementation of session sync was the only
remaining user of this field and has been deleted recently, so let's
just remove the now unused API.

This is convenient for pseudo-USS (SyncableServiceBasedBridge) because
there is no need for additional plumbing to propagate this information
through (it wasn't actually properly populated prior to this patch).

Bug: 895455
Change-Id: I02a577978b5fd7a84ced4775cd200d0210928985
Reviewed-on: https://chromium-review.googlesource.com/c/1343003
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609715}

chrome/browser/chromeos/preferences_unittest.cc

@@ -60,7 +60,7 @@ CreatePrefSyncData(const std::string& name, const base::Value& value) {
   sync_pb::PreferenceSpecifics* pref = specifics.mutable_preference();
   pref->set_name(name);
   pref->set_value(serialized);
-  return syncer::SyncData::CreateRemoteData(1, specifics, base::Time());
+  return syncer::SyncData::CreateRemoteData(1, specifics);
 }
 
 }  // anonymous namespace

chrome/browser/extensions/extension_disabled_ui_browsertest.cc

@@ -304,7 +304,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionDisabledGlobalErrorTest, RemoteInstall) {
       "http://localhost/autoupdate/updates.xml");
   specifics.mutable_extension()->set_version("2");
   syncer::SyncData sync_data =
-      syncer::SyncData::CreateRemoteData(1234567, specifics, base::Time::Now());
+      syncer::SyncData::CreateRemoteData(1234567, specifics);
 
   ExtensionSyncService* sync_service = ExtensionSyncService::Get(profile());
   sync_service->MergeDataAndStartSyncing(

chrome/browser/prefs/synced_pref_change_registrar_browsertest.cc

@@ -58,8 +58,8 @@ class SyncedPrefChangeRegistrarTest : public InProcessBrowserTest {
     pref_specifics->set_name(name);
     pref_specifics->set_value(serialized_value);
 
-    syncer::SyncData change_data = syncer::SyncData::CreateRemoteData(
-        ++next_sync_data_id_, specifics, base::Time());
+    syncer::SyncData change_data =
+        syncer::SyncData::CreateRemoteData(++next_sync_data_id_, specifics);
     syncer::SyncChange change(
         FROM_HERE, syncer::SyncChange::ACTION_UPDATE, change_data);
 

chrome/browser/themes/theme_syncable_service_unittest.cc

@@ -456,7 +456,7 @@ TEST_F(ThemeSyncableServiceTest, ProcessSyncThemeChange) {
   syncer::SyncChangeList change_list;
   change_list.push_back(syncer::SyncChange(
       FROM_HERE, syncer::SyncChange::ACTION_UPDATE,
-      syncer::SyncData::CreateRemoteData(1, entity_specifics, base::Time())));
+      syncer::SyncData::CreateRemoteData(1, entity_specifics)));
   error = theme_sync_service_->ProcessSyncChanges(FROM_HERE, change_list);
   EXPECT_FALSE(error.IsSet()) << error.message();
   EXPECT_EQ(fake_theme_service_->theme_extension(), theme_extension_.get());

chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc

@@ -117,7 +117,7 @@ syncer::SyncData CreateAppRemoteData(
     app_list->set_item_pin_ordinal(item_pin_ordinal);
 
   return syncer::SyncData::CreateRemoteData(std::hash<std::string>{}(id),
-                                            specifics, base::Time());
+                                            specifics);
 }
 
 syncer::SyncDataList CreateBadAppRemoteData(const std::string& id) {

chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc

@@ -702,8 +702,7 @@ class ChromeLauncherControllerTest : public BrowserWithTestWindowTest {
     sync_pb::PreferenceSpecifics* pref_one = one.mutable_preference();
     pref_one->set_name(prefs::kPinnedLauncherApps);
     pref_one->set_value(serialized);
-    init_sync_list.push_back(
-        syncer::SyncData::CreateRemoteData(1, one, base::Time()));
+    init_sync_list.push_back(syncer::SyncData::CreateRemoteData(1, one));
     StartPrefSyncService(init_sync_list);
   }
 

components/dom_distiller/core/dom_distiller_store_unittest.cc

@@ -148,8 +148,7 @@ class DomDistillerStoreTest : public testing::Test {
  protected:
   SyncData CreateSyncData(const ArticleEntry& entry) {
     EntitySpecifics specifics = SpecificsFromEntry(entry);
-    return SyncData::CreateRemoteData(next_sync_id_++, specifics,
-                                      Time::UnixEpoch());
+    return SyncData::CreateRemoteData(next_sync_id_++, specifics);
   }
 
   SyncDataList SyncDataFromEntryMap(const EntryMap& model) {

components/history/core/browser/history_service_unittest.cc

@@ -749,8 +749,7 @@ TEST_F(HistoryServiceTest, ProcessGlobalIdDeleteDirective) {
           .ToInternalValue());
   global_id_directive->set_start_time_usec(3);
   global_id_directive->set_end_time_usec(10);
-  directives.push_back(
-      syncer::SyncData::CreateRemoteData(1, entity_specs, base::Time()));
+  directives.push_back(syncer::SyncData::CreateRemoteData(1, entity_specs));
 
   // 2nd directive.
   global_id_directive->Clear();
@@ -759,8 +758,7 @@ TEST_F(HistoryServiceTest, ProcessGlobalIdDeleteDirective) {
           .ToInternalValue());
   global_id_directive->set_start_time_usec(13);
   global_id_directive->set_end_time_usec(19);
-  directives.push_back(
-      syncer::SyncData::CreateRemoteData(2, entity_specs, base::Time()));
+  directives.push_back(syncer::SyncData::CreateRemoteData(2, entity_specs));
 
   syncer::FakeSyncChangeProcessor change_processor;
   EXPECT_FALSE(history_service_
@@ -827,15 +825,13 @@ TEST_F(HistoryServiceTest, ProcessTimeRangeDeleteDirective) {
           ->mutable_time_range_directive();
   time_range_directive->set_start_time_usec(2);
   time_range_directive->set_end_time_usec(5);
-  directives.push_back(
-      syncer::SyncData::CreateRemoteData(1, entity_specs, base::Time()));
+  directives.push_back(syncer::SyncData::CreateRemoteData(1, entity_specs));
 
   // 2nd directive.
   time_range_directive->Clear();
   time_range_directive->set_start_time_usec(8);
   time_range_directive->set_end_time_usec(10);
-  directives.push_back(
-      syncer::SyncData::CreateRemoteData(2, entity_specs, base::Time()));
+  directives.push_back(syncer::SyncData::CreateRemoteData(2, entity_specs));
 
   syncer::FakeSyncChangeProcessor change_processor;
   EXPECT_FALSE(history_service_

components/sync/driver/generic_change_processor.cc

@@ -53,8 +53,7 @@ SyncData BuildRemoteSyncData(int64_t sync_id, const ReadNode& read_node) {
       password_holder.mutable_password()
           ->mutable_client_only_encrypted_data()
           ->CopyFrom(read_node.GetPasswordSpecifics());
-      return SyncData::CreateRemoteData(sync_id, password_holder,
-                                        read_node.GetModificationTime());
+      return SyncData::CreateRemoteData(sync_id, password_holder);
     }
     case SESSIONS:
       // Include tag hashes for sessions data type to allow discarding during
@@ -66,12 +65,11 @@ SyncData BuildRemoteSyncData(int64_t sync_id, const ReadNode& read_node) {
       // another copy of this string around.
       return SyncData::CreateRemoteData(
           sync_id, read_node.GetEntitySpecifics(),
-          read_node.GetModificationTime(),
           read_node.GetEntry()->GetUniqueClientTag());
     default:
       // Use the specifics directly, encryption has already been handled.
-      return SyncData::CreateRemoteData(sync_id, read_node.GetEntitySpecifics(),
-                                        read_node.GetModificationTime());
+      return SyncData::CreateRemoteData(sync_id,
+                                        read_node.GetEntitySpecifics());
   }
 }
 
@@ -114,10 +112,10 @@ void GenericChangeProcessor::ApplyChangesFromSyncModel(
             ->mutable_client_only_encrypted_data()
             ->CopyFrom(it->extra->unencrypted());
       }
-      syncer_changes_.push_back(SyncChange(
-          FROM_HERE, SyncChange::ACTION_DELETE,
-          SyncData::CreateRemoteData(
-              it->id, specifics ? *specifics : it->specifics, base::Time())));
+      syncer_changes_.push_back(
+          SyncChange(FROM_HERE, SyncChange::ACTION_DELETE,
+                     SyncData::CreateRemoteData(
+                         it->id, specifics ? *specifics : it->specifics)));
     } else {
       SyncChange::SyncChangeType action =
           (it->action == ChangeRecord::ACTION_ADD) ? SyncChange::ACTION_ADD

components/sync/model/sync_change_unittest.cc

@@ -84,9 +84,9 @@ TEST_F(SyncChangeTest, SyncerChanges) {
   sync_pb::PreferenceSpecifics* pref_specifics =
       update_specifics.mutable_preference();
   pref_specifics->set_name("update");
-  change_list.push_back(SyncChange(
-      FROM_HERE, SyncChange::ACTION_UPDATE,
-      SyncData::CreateRemoteData(1, update_specifics, base::Time())));
+  change_list.push_back(
+      SyncChange(FROM_HERE, SyncChange::ACTION_UPDATE,
+                 SyncData::CreateRemoteData(1, update_specifics)));
 
   // Create an add.
   sync_pb::EntitySpecifics add_specifics;
@@ -94,15 +94,15 @@ TEST_F(SyncChangeTest, SyncerChanges) {
   pref_specifics->set_name("add");
   change_list.push_back(
       SyncChange(FROM_HERE, SyncChange::ACTION_ADD,
-                 SyncData::CreateRemoteData(2, add_specifics, base::Time())));
+                 SyncData::CreateRemoteData(2, add_specifics)));
 
   // Create a delete.
   sync_pb::EntitySpecifics delete_specifics;
   pref_specifics = delete_specifics.mutable_preference();
   pref_specifics->set_name("add");
-  change_list.push_back(SyncChange(
-      FROM_HERE, SyncChange::ACTION_DELETE,
-      SyncData::CreateRemoteData(3, delete_specifics, base::Time())));
+  change_list.push_back(
+      SyncChange(FROM_HERE, SyncChange::ACTION_DELETE,
+                 SyncData::CreateRemoteData(3, delete_specifics)));
 
   ASSERT_EQ(3U, change_list.size());
 

components/sync/model/sync_data.cc

@@ -42,12 +42,8 @@ void SyncData::ImmutableSyncEntityTraits::Swap(sync_pb::SyncEntity* t1,
 
 SyncData::SyncData() : id_(kInvalidId), is_local_(false), is_valid_(false) {}
 
-SyncData::SyncData(bool is_local,
-                   int64_t id,
-                   sync_pb::SyncEntity* entity,
-                   const base::Time& remote_modification_time)
+SyncData::SyncData(bool is_local, int64_t id, sync_pb::SyncEntity* entity)
     : id_(id),
-      remote_modification_time_(remote_modification_time),
       immutable_entity_(entity),
       is_local_(is_local),
       is_valid_(true) {}
@@ -72,18 +68,17 @@ SyncData SyncData::CreateLocalData(const std::string& sync_tag,
   entity.set_client_defined_unique_tag(sync_tag);
   entity.set_non_unique_name(non_unique_title);
   entity.mutable_specifics()->CopyFrom(specifics);
-  return SyncData(/*is_local=*/true, kInvalidId, &entity, base::Time());
+  return SyncData(/*is_local=*/true, kInvalidId, &entity);
 }
 
 // Static.
 SyncData SyncData::CreateRemoteData(int64_t id,
                                     sync_pb::EntitySpecifics specifics,
-                                    base::Time modification_time,
                                     std::string client_tag_hash) {
   sync_pb::SyncEntity entity;
   *entity.mutable_specifics() = std::move(specifics);
   entity.set_client_defined_unique_tag(std::move(client_tag_hash));
-  return SyncData(/*is_local=*/false, id, &entity, modification_time);
+  return SyncData(/*is_local=*/false, id, &entity);
 }
 
 bool SyncData::IsValid() const {
@@ -152,10 +147,6 @@ SyncDataRemote::SyncDataRemote(const SyncData& sync_data)
 
 SyncDataRemote::~SyncDataRemote() {}
 
-const base::Time& SyncDataRemote::GetModifiedTime() const {
-  return remote_modification_time_;
-}
-
 int64_t SyncDataRemote::GetId() const {
   DCHECK(!IsLocal());
   DCHECK_NE(id_, kInvalidId);

components/sync/model/sync_data.h

@@ -61,7 +61,6 @@ class SyncData {
   // Helper method for creating SyncData objects originating from the syncer.
   static SyncData CreateRemoteData(int64_t id,
                                    sync_pb::EntitySpecifics specifics,
-                                   base::Time last_modified_time,
                                    std::string client_tag_hash = std::string());
 
   // Whether this SyncData holds valid data. The only way to have a SyncData
@@ -111,11 +110,6 @@ class SyncData {
 
   int64_t id_;
 
-  // This may be null if the SyncData represents a deleted item.
-  // TODO(crbug.com/895455): Remove when directory-based sessions sync is
-  // removed, the only remaining reader.
-  base::Time remote_modification_time_;
-
   // The actual shared sync entity being held.
   ImmutableSyncEntity immutable_entity_;
 
@@ -127,10 +121,7 @@ class SyncData {
   bool is_valid_;
 
   // Clears |entity|.
-  SyncData(bool is_local_,
-           int64_t id,
-           sync_pb::SyncEntity* entity,
-           const base::Time& remote_modification_time);
+  SyncData(bool is_local_, int64_t id, sync_pb::SyncEntity* entity);
 };
 
 // A SyncData going to the syncer.
@@ -156,12 +147,6 @@ class SyncDataRemote : public SyncData {
   explicit SyncDataRemote(const SyncData& sync_data);
   ~SyncDataRemote();
 
-  // Return the last motification time according to the server. This may be null
-  // if the SyncData represents a deleted item.
-  // TODO(crbug.com/895455): Remove when directory-based sessions sync is
-  // removed, the only remaining reader.
-  const base::Time& GetModifiedTime() const;
-
   // Returns the tag hash value. May not always be present, in which case an
   // empty string will be returned.
   const std::string& GetClientTagHash() const;

components/sync/model/sync_data_unittest.cc

@@ -23,7 +23,6 @@ const char kSyncTag[] = "3984729834";
 const ModelType kDatatype = PREFERENCES;
 const char kNonUniqueTitle[] = "my preference";
 const int64_t kId = 439829;
-const base::Time kLastModifiedTime = base::Time();
 
 class SyncDataTest : public testing::Test {
  protected:
@@ -59,21 +58,18 @@ TEST_F(SyncDataTest, CreateLocalData) {
 
 TEST_F(SyncDataTest, CreateRemoteData) {
   specifics.mutable_preference();
-  SyncData data = SyncData::CreateRemoteData(kId, specifics, kLastModifiedTime);
+  SyncData data = SyncData::CreateRemoteData(kId, specifics);
   EXPECT_TRUE(data.IsValid());
   EXPECT_FALSE(data.IsLocal());
   EXPECT_EQ(kId, SyncDataRemote(data).GetId());
-  EXPECT_EQ(kLastModifiedTime, SyncDataRemote(data).GetModifiedTime());
   EXPECT_TRUE(data.GetSpecifics().has_preference());
 }
 
 TEST_F(SyncDataTest, CreateRemoteDataWithInvalidId) {
   specifics.mutable_preference();
-  SyncData data =
-      SyncData::CreateRemoteData(kInvalidId, specifics, kLastModifiedTime);
+  SyncData data = SyncData::CreateRemoteData(kInvalidId, specifics);
   EXPECT_TRUE(data.IsValid());
   EXPECT_FALSE(data.IsLocal());
-  EXPECT_EQ(kLastModifiedTime, SyncDataRemote(data).GetModifiedTime());
   EXPECT_TRUE(data.GetSpecifics().has_preference());
 }
 

components/sync/model_impl/syncable_service_based_bridge.cc

@@ -612,7 +612,6 @@ SyncableServiceBasedBridge::MaybeStartSyncableService() {
 
     initial_sync_data.push_back(SyncData::CreateRemoteData(
         /*id=*/kInvalidNodeId, std::move(specifics),
-        /*last_modified_time=*/base::Time(),  // Used by legacy sessions only.
         /*client_tag_hash=*/record.first));
   }
 
@@ -674,7 +673,6 @@ SyncableServiceBasedBridge::StoreAndConvertRemoteChanges(
             FROM_HERE, SyncChange::ACTION_DELETE,
             SyncData::CreateRemoteData(
                 /*id=*/kInvalidNodeId, std::move(specifics),
-                change.data().modification_time,
                 change.data().client_tag_hash));
 
         // For tombstones, there is no actual data, which means no client tag
@@ -723,7 +721,6 @@ SyncableServiceBasedBridge::StoreAndConvertRemoteChanges(
             SyncData::CreateRemoteData(
                 /*id=*/kInvalidNodeId,
                 std::move(*persisted_entity.mutable_specifics()),
-                change.data().modification_time,
                 change.data().client_tag_hash));
 
         break;

components/sync/model_impl/syncable_service_based_bridge_unittest.cc

@@ -719,7 +719,6 @@ TEST(SyncableServiceBasedBridgeLocalChangeProcessorTest,
   change_list.push_back(SyncChange(
       FROM_HERE, SyncChange::ACTION_DELETE,
       SyncData::CreateRemoteData(/*id=*/1, specifics,
-                                 /*last_modified_time=*/base::Time::Now(),
                                  /*client_tag_hash=*/kClientTagHash)));
 
   sync_change_processor->ProcessSyncChanges(FROM_HERE, change_list);
@@ -755,7 +754,6 @@ TEST(SyncableServiceBasedBridgeLocalChangeProcessorTest,
   change_list.push_back(SyncChange(
       FROM_HERE, SyncChange::ACTION_DELETE,
       SyncData::CreateRemoteData(/*id=*/1, specifics,
-                                 /*last_modified_time=*/base::Time::Now(),
                                  /*client_tag_hash=*/kClientTagHash)));
 
   sync_change_processor->ProcessSyncChanges(FROM_HERE, change_list);

components/sync_preferences/pref_service_syncable_unittest.cc

@@ -124,9 +124,8 @@ class PrefServiceSyncableTest : public testing::Test {
     sync_pb::PreferenceSpecifics* pref_one = entity.mutable_preference();
     pref_one->set_name(name);
     pref_one->set_value(serialized);
-    return syncer::SyncChange(
-        FROM_HERE, type,
-        syncer::SyncData::CreateRemoteData(id, entity, base::Time()));
+    return syncer::SyncChange(FROM_HERE, type,
+                              syncer::SyncData::CreateRemoteData(id, entity));
   }
 
   void AddToRemoteDataList(const std::string& name,
@@ -139,8 +138,8 @@ class PrefServiceSyncableTest : public testing::Test {
     sync_pb::PreferenceSpecifics* pref_one = one.mutable_preference();
     pref_one->set_name(name);
     pref_one->set_value(serialized);
-    out->push_back(SyncData::CreateRemoteData(++next_pref_remote_sync_node_id_,
-                                              one, base::Time()));
+    out->push_back(
+        SyncData::CreateRemoteData(++next_pref_remote_sync_node_id_, one));
   }
 
   void InitWithSyncDataTakeOutput(const syncer::SyncDataList& initial_data,
@@ -372,9 +371,8 @@ class PrefServiceSyncableMergeTest : public testing::Test {
     sync_pb::PreferenceSpecifics* pref_one = entity.mutable_preference();
     pref_one->set_name(name);
     pref_one->set_value(serialized);
-    return syncer::SyncChange(
-        FROM_HERE, type,
-        syncer::SyncData::CreateRemoteData(id, entity, base::Time()));
+    return syncer::SyncChange(FROM_HERE, type,
+                              syncer::SyncData::CreateRemoteData(id, entity));
   }
 
   void AddToRemoteDataList(const std::string& name,
@@ -387,8 +385,8 @@ class PrefServiceSyncableMergeTest : public testing::Test {
     sync_pb::PreferenceSpecifics* pref_one = one.mutable_preference();
     pref_one->set_name(name);
     pref_one->set_value(serialized);
-    out->push_back(SyncData::CreateRemoteData(++next_pref_remote_sync_node_id_,
-                                              one, base::Time()));
+    out->push_back(
+        SyncData::CreateRemoteData(++next_pref_remote_sync_node_id_, one));
   }
 
   void InitWithSyncDataTakeOutput(const syncer::SyncDataList& initial_data,

components/sync_wifi/wifi_credential_syncable_service_unittest.cc

@@ -132,8 +132,7 @@ class WifiCredentialSyncableServiceTest : public testing::Test {
         FROM_HERE, syncer::SyncChange::ACTION_ADD,
         syncer::SyncData::CreateRemoteData(
             sync_item_id,
-            MakeWifiCredentialSpecifics(ssid, security_class, passphrase),
-            base::Time()));
+            MakeWifiCredentialSpecifics(ssid, security_class, passphrase)));
   }
 
   void StartSyncing() {