Add version field to syncer::Invalidation.

sync/internal_api/public/base/invalidation.cc

Index: sync/internal_api/public/base/invalidation.cc
diff --git a/sync/internal_api/public/base/invalidation.cc b/sync/internal_api/public/base/invalidation.cc
index 472fad2c3c3f2c1d78f0f1a1d348dbfa6bc60aba..791e30551b81714a0c04c2073dcbf0098f9e76c9 100644
--- a/sync/internal_api/public/base/invalidation.cc
+++ b/sync/internal_api/public/base/invalidation.cc
@@ -64,19 +64,23 @@ AckHandle::AckHandle(const std::string& state, base::Time timestamp)
 AckHandle::~AckHandle() {
 }
 
+const int64 Invalidation::kUnknownVersion = -1;

[rlarocque, 2013/07/16 18:33:16]

I wish this wasn't necessary.  Looking at the protobufs in
googlecacheinvalidation, I get the feeling that the protocol was intended to
allow any int64 to be a valid version.  There should be nothing special about
-1.

I'd prefer it if the API made these invalid versions explicit, like the
googlecacheinvalidation API does.  It has specific callbacks for unknown version
invalidations that don't pass back an Invalidation object.

This approach should be safe for now, since there no one has written an "if
(version == kInvalidVersion)" statement yet.  You can ignore this comment for
now.  I'll try to clean it up in a later CL.

[Steve Condie, 2013/07/16 21:23:06]

Ok. I will be doing that comparison in my code, but my invalidation generating
code doesn't use negative numbers (as I assume is probably the normal case) so
it shouldn't pose a problem.

+
 Invalidation::Invalidation()
-    : ack_handle(AckHandle::InvalidAckHandle()) {
+    : version(kUnknownVersion), ack_handle(AckHandle::InvalidAckHandle()) {
 }
 
 Invalidation::~Invalidation() {
 }
 
 bool Invalidation::Equals(const Invalidation& other) const {
-  return (payload == other.payload) && ack_handle.Equals(other.ack_handle);
+  return (version == other.version) && (payload == other.payload) &&
+      ack_handle.Equals(other.ack_handle);
 }
 
 scoped_ptr<base::DictionaryValue> Invalidation::ToValue() const {
   scoped_ptr<base::DictionaryValue> value(new base::DictionaryValue());
+  value->SetString("version", base::Int64ToString(version));
   value->SetString("payload", payload);
   value->Set("ackHandle", ack_handle.ToValue().release());
   return value.Pass();
@@ -84,6 +88,11 @@ scoped_ptr<base::DictionaryValue> Invalidation::ToValue() const {
 
 bool Invalidation::ResetFromValue(const base::DictionaryValue& value) {
   const base::DictionaryValue* ack_handle_value = NULL;
+  std::string version_as_string;

[rlarocque, 2013/07/16 18:33:16]

nit: This function returns false when the parsing of other values fail. 
Although you can't return false if the version isn't there, maybe you could
return false if the StringToInt64 parsing fails?

[Steve Condie, 2013/07/16 21:23:06]

Done.

+  if (!value.GetString("version", &version_as_string) ||
+      !base::StringToInt64(version_as_string, &version)) {
+    version = kUnknownVersion;
+  }
   return
       value.GetString("payload", &payload) &&
       value.GetDictionary("ackHandle", &ack_handle_value) &&