sync: Deprecate old UniquePosition format

Drop support for legacy UniquePosition encoding.

Some old clients (M28 and M29) only understand the uncompressed and gzip
encodings of UniquePositions.  To maintain compatibility with them,
clients that supported the newer and better custom compression algorithm
(M30 and above) wrote protobufs that contained both the old and new
encodings.

Maintaining backwards compatibility in this way is expensive.  It's not
a big deal for clients that have a reasonable number of bookmarks, but
for the few users that have thousands of them this could be a real
problem.

Since those old clients are now much rarer, it should be safe to no
longer write the extra encoding just for them.  If any of those older
clients do come across protobufs that are newer than what they support,
they will fail to apply the position updates.  In other words, bookmarks
modified on newer clients will have randomly assigned positions on older
clients.  Those older clients will also run over a NOTREACHED(), but I
don't think this will do any harm in practice.

BUG=260394

Review URL: https://codereview.chromium.org/68533007

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@235380 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 40 insertions, 49 deletions

diff --git a/sync/internal_api/public/base/unique_position.cc b/sync/internal_api/public/base/unique_position.cc
index 80c7187..40bab6e 100644
--- a/sync/internal_api/public/base/unique_position.cc
+++ b/sync/internal_api/public/base/unique_position.cc
@@ -154,38 +154,10 @@ void UniquePosition::ToProto(sync_pb::UniquePosition* proto) const {
   // This is the current preferred foramt.
   proto->set_custom_compressed_v1(compressed_);
 
-  // Some older clients (M28) don't know how to read that format.  We don't want
-  // to break them until they're obsolete.  We'll serialize to the old-style in
-  // addition to the new so they won't be confused.
-  std::string bytes = Uncompress(compressed_);
-  if (bytes.size() < kCompressBytesThreshold) {
-    // If it's small, then just write it.  This is the common case.
-    proto->set_value(bytes);
-  } else {
-    // We've got a large one.  Compress it.
-    proto->set_uncompressed_length(bytes.size());
-    std::string* compressed = proto->mutable_compressed_value();
-
-    uLongf compressed_len = compressBound(bytes.size());
-    compressed->resize(compressed_len);
-    int result = compress(reinterpret_cast<Bytef*>(string_as_array(compressed)),
-             &compressed_len,
-             reinterpret_cast<const Bytef*>(bytes.data()),
-             bytes.size());
-    if (result != Z_OK) {
-      NOTREACHED() << "Failed to compress position: " << result;
-      // Maybe we can write an uncompressed version?
-      proto->Clear();
-      proto->set_value(bytes);
-    } else if (compressed_len >= bytes.size()) {
-      // Oops, we made it bigger.  Just write the uncompressed version instead.
-      proto->Clear();
-      proto->set_value(bytes);
-    } else {
-      // Success!  Don't forget to adjust the string's length.
-      compressed->resize(compressed_len);
-    }
-  }
+  // Older clients used to write other formats.  We don't bother doing that
+  // anymore because that form of backwards compatibility is expensive.  We no
+  // longer want to pay that price just too support clients that have been
+  // obsolete for a long time.  See the proto definition for details.
 }
 
 void UniquePosition::SerializeToString(std::string* blob) const {
diff --git a/sync/internal_api/public/base/unique_position_unittest.cc b/sync/internal_api/public/base/unique_position_unittest.cc
index 5007a4b..371874a 100644
--- a/sync/internal_api/public/base/unique_position_unittest.cc
+++ b/sync/internal_api/public/base/unique_position_unittest.cc
@@ -655,25 +655,16 @@ TEST_F(CompressedPositionTest, SerializeAndDeserialize) {
     UniquePosition deserialized = UniquePosition::FromProto(proto);
 
     EXPECT_PRED_FORMAT2(Equals, *it, deserialized);
-
   }
 }
 
-// Test that redundant serialization for legacy clients is correct, too.
-TEST_F(CompressedPositionTest, SerializeAndLegacyDeserialize) {
-  for (std::vector<UniquePosition>::const_iterator it = positions_.begin();
-       it != positions_.end(); ++it) {
-    SCOPED_TRACE("iteration: " + it->ToDebugString());
-    sync_pb::UniquePosition proto;
-
-    it->ToProto(&proto);
-
-    // Clear default encoding to force it to use legacy as fallback.
-    proto.clear_custom_compressed_v1();
-    UniquePosition deserialized = UniquePosition::FromProto(proto);
-
-    EXPECT_PRED_FORMAT2(Equals, *it, deserialized);
-  }
+// Test that deserialization failures of protobufs where we know none of its
+// fields is not catastrophic.  This may happen if all the fields currently
+// known to this client become deprecated in the future.
+TEST_F(CompressedPositionTest, DeserializeProtobufFromTheFuture) {
+  sync_pb::UniquePosition proto;
+  UniquePosition deserialized = UniquePosition::FromProto(proto);
+  EXPECT_FALSE(deserialized.IsValid());
 }
 
 // Make sure the comparison functions are working correctly.
diff --git a/sync/protocol/unique_position.proto b/sync/protocol/unique_position.proto
index 992c134..4864f27 100644
--- a/sync/protocol/unique_position.proto
+++ b/sync/protocol/unique_position.proto
@@ -24,7 +24,33 @@ package sync_pb;
 // Items under the same parent are positioned relative to each other by a
 // lexicographic comparison of their UniquePosition values.
 message UniquePosition {
+  // History:
+  //
+  // Unique positions were first introduced in M28.  This change was rolled out
+  // in such a way that it would try to maintain backwards compatibilty with
+  // clients that understood only the old int64-based positions.
+  //
+  // At first, clients supported only the 'value' field.  This version never
+  // made it to stable.  We later added support for the 'compressed_value'
+  // field, and clients would populate either one or the other.
+  //
+  // In M30, we added the custom_compressed_v1 representation.  This
+  // representation was better than the previous implementations in almost every
+  // way.  However, we could not use it right away, since older clients would
+  // not understand it.  We decided to write both the old-style ('value' or
+  // 'custom_compressed') representation and the 'custom_compressed_v1'
+  // repersentations to every protobuf during the transition period.  Protobufs
+  // written during this transition period would be readable by clients who
+  // understand at least one of the two formats.
+  //
+  // In M33, we dropped support for writing the backwards-compatibility fields.
+  // Protobufs written by this version or later are not be intelligible by
+  // clients with version M29 or older.  Those clients will end up making use of
+  // the old int64 position fallback mechanism.
+
   // The uncompressed string of bytes representing the position.
+  //
+  // Deprecated.  See history note above.
   optional bytes value = 1;
 
   // The client may choose to write a compressed position to this field instead
@@ -33,6 +59,8 @@ message UniquePosition {
   // with gzip and stored in the compressed_value field.  The position's
   // uncompressed length must be specified and written to the
   // uncompressed_length field.
+  //
+  // Deprecated.  See history note above.
   optional bytes compressed_value = 2;
   optional uint64 uncompressed_length = 3;
 
@@ -44,7 +72,7 @@ message UniquePosition {
   //
   // The compression scheme is implemented and documented in
   // sync/internal_api/base/unique_position.cc.
-  // 
+  //
   // As of M30, this is the preferred encoding.  Newer clients may continue to
   // populate the 'value' and 'compressed_value' fields to ensure backwards
   // compatibility, but they will always try to read from this field first.