Sync protocol: Remove and deprecate more fields

deprecated = true

This replaces new_timestamp, from_timestamp, and sync_timestamp by
"reserved" entries, and marks a number of other fields as
[deprecated = true].

Bug: 950904
Change-Id: Ic4b26ec368ac94e38ff330024698cd13c30417a5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1821790
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Auto-Submit: Marc Treib <treib@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#699664}

components/sync/engine_impl/syncer_proto_util.cc

@@ -420,7 +420,6 @@ SyncerError SyncerProtoUtil::PostClientToServerMessage(
   DCHECK(msg.has_api_key());
   DCHECK(msg.has_client_status());
   DCHECK(msg.has_invalidator_client_id());
-  DCHECK(!msg.get_updates().has_from_timestamp());  // Deprecated.
 
   LogClientToServerMessage(msg);
   if (!PostAndProcessHeaders(cycle->context()->connection_manager(), cycle, msg,
@@ -613,12 +612,12 @@ std::string SyncerProtoUtil::SyncEntityDebugString(
       "d (%s), "
       "ctime: %" PRId64
       "d (%s), "
-      "name: %s, sync_timestamp: %" PRId64
+      "name: %s, "
       "d, "
       "%s ",
       entry.id_string().c_str(), entry.parent_id_string().c_str(),
       entry.version(), entry.mtime(), mtime_str.c_str(), entry.ctime(),
-      ctime_str.c_str(), entry.name().c_str(), entry.sync_timestamp(),
+      ctime_str.c_str(), entry.name().c_str(),
       entry.deleted() ? "deleted, " : "");
 }
 

components/sync/protocol/proto_visitors.h

@@ -851,7 +851,6 @@ VISIT_PROTO_FIELDS(const sync_pb::SyncEntity& proto) {
   VISIT(ctime);
   VISIT(name);
   VISIT(non_unique_name);
-  VISIT(sync_timestamp);
   VISIT(server_defined_unique_tag);
   VISIT(position_in_parent);
   VISIT(unique_position);

components/sync/protocol/sync.proto

@@ -225,11 +225,8 @@ message SyncEntity {
   // Present in both GetUpdatesResponse and CommitMessage.
   optional string non_unique_name = 8;
 
-  // A value from a monotonically increasing sequence that indicates when
-  // this item was last updated on the server. This is now equivalent
-  // to version. This is now deprecated in favor of version.
-  // Present only in GetUpdatesResponse.
-  optional int64 sync_timestamp = 9;
+  reserved 9;
+  reserved "sync_timestamp";
 
   // If present, this tag identifies this item as being a uniquely
   // instanced item.  The server ensures that there is never more
@@ -296,7 +293,7 @@ message SyncEntity {
   // Conversion from UniquePosition to int64 reverses this process: the first 8
   // bytes of the position are to be interpreted as a big endian int64 value
   // with its sign bit flipped.
-  optional int64 position_in_parent = 15;
+  optional int64 position_in_parent = 15 [deprecated = true];
 
   // Contains the ID of the element (under the same parent) after which this
   // element resides. An empty string indicates that the element is the first
@@ -309,7 +306,7 @@ message SyncEntity {
   //
   // This is deprecated.  Clients are allowed to omit this as long as they
   // include |position_in_parent| instead.
-  optional string insert_after_item_id = 16;
+  optional string insert_after_item_id = 16 [deprecated = true];
 
   reserved 17;
   reserved "extended_attributes";
@@ -380,7 +377,7 @@ message SyncEntity {
   // attempt to support it.  In fact, no client ever read or set this field.
   //
   // Deprecated in M26.
-  optional bytes ordinal_in_parent = 24;
+  optional bytes ordinal_in_parent = 24 [deprecated = true];
 
   // This is the fourth attempt at positioning.
   //
@@ -603,7 +600,7 @@ message DataTypeProgressMarker {
   // token mechanism can set this field in a GetUpdatesMessage.  The server
   // will perform a get updates operation as normal from the indicated
   // timestamp, and return only an opaque progress token.
-  optional int64 timestamp_token_for_migration = 3;
+  optional int64 timestamp_token_for_migration = 3 [deprecated = true];
 
   // An opaque-to-the-client string of bytes, received through a notification,
   // that the server may interpret as a hint about the location of the latest
@@ -611,7 +608,7 @@ message DataTypeProgressMarker {
   //
   // Deprecated in M29.  We should use the repeated field version in the
   // PerClientTypeState instead.
-  optional string notification_hint = 4;
+  optional string notification_hint = 4 [deprecated = true];
 
   // This field will be included only in GetUpdates with origin GU_TRIGGER.
   optional GetUpdateTriggers get_update_triggers = 5;
@@ -624,14 +621,8 @@ message DataTypeProgressMarker {
 }
 
 message GetUpdatesMessage {
-  // Indicates the client's current progress in downloading updates.  A
-  // from_timestamp value of zero means that the client is requesting a first-
-  // time sync.  After that point, clients should fill in this value with the
-  // value returned in the last-seen GetUpdatesResponse.new_timestamp.
-  //
-  // from_timestamp has been deprecated; clients should use
-  // |from_progress_marker| instead, which allows more flexibility.
-  optional int64 from_timestamp = 1;
+  reserved 1;
+  reserved "from_timestamp";
 
   // Indicates the reason for the GetUpdatesMessage.
   // This was *mostly* deprecated in M29.  GetUpdatesOrigin is the new way to
@@ -690,8 +681,6 @@ message GetUpdatesMessage {
   // Set of optional per-client datatype contexts.
   repeated DataTypeContext client_contexts = 11;
 
-  // requested_types is a deprecated predecessor to using progress markers.
-  // See https://crbug.com/687426.
   reserved 4;
   reserved "requested_types";
 };
@@ -881,15 +870,8 @@ message GetUpdatesResponse {
   // New sync entries that the client should apply.
   repeated SyncEntity entries = 1;
 
-  // If there are more changes on the server that weren't processed during this
-  // GetUpdates request, the client should send another GetUpdates request and
-  // use new_timestamp as the from_timestamp value within GetUpdatesMessage.
-  //
-  // This field has been deprecated and will be returned only to clients
-  // that set the also-deprecated |from_timestamp| field in the update request.
-  // Clients should use |from_progress_marker| and |new_progress_marker|
-  // instead.
-  optional int64 new_timestamp = 2;
+  reserved 2;
+  reserved "new_timestamp";
 
   reserved 3;
   reserved "newest_timestamp";
@@ -899,11 +881,9 @@ message GetUpdatesResponse {
   // after the current batch.
   optional int64 changes_remaining = 4;
 
-  // Opaque, per-datatype timestamp-like tokens.  A client should use this
-  // field in lieu of new_timestamp, which is deprecated in newer versions
-  // of the protocol.  Clients should retain and persist the values returned
-  // in this field, and present them back to the server to indicate the
-  // starting point for future update requests.
+  // Opaque, per-datatype timestamp-like tokens.  Clients should retain and
+  // persist the values returned in this field, and present them back to the
+  // server to indicate the starting point for future update requests.
   //
   // This will be sent only if the client provided |from_progress_marker|
   // in the update request.

components/sync/test/engine/mock_connection_manager.cc

@@ -313,7 +313,6 @@ sync_pb::SyncEntity* MockConnectionManager::SetNigori(
   ent->set_name("Nigori");
   ent->set_non_unique_name("Nigori");
   ent->set_version(version);
-  ent->set_sync_timestamp(sync_ts);
   ent->set_mtime(sync_ts);
   ent->set_ctime(1);
   ent->set_position_in_parent(0);
@@ -365,7 +364,6 @@ sync_pb::SyncEntity* MockConnectionManager::AddUpdateMeta(
   ent->set_non_unique_name(name);
   ent->set_name(name);
   ent->set_version(version);
-  ent->set_sync_timestamp(sync_ts);
   ent->set_mtime(sync_ts);
   ent->set_ctime(1);
   ent->set_position_in_parent(GeneratePositionInParent());
@@ -523,7 +521,6 @@ bool MockConnectionManager::ProcessGetUpdates(
   }
   const GetUpdatesMessage& gu = csm->get_updates();
   num_get_updates_requests_++;
-  EXPECT_FALSE(gu.has_from_timestamp());
 
   if (fail_non_periodic_get_updates_) {
     EXPECT_EQ(sync_pb::SyncEnums::PERIODIC, gu.get_updates_origin());