Add version field to syncer::Invalidation.

sync/internal_api/public/base/invalidation.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "sync/internal_api/public/base/invalidation.h"
 
 #include <cstddef>
 #include "base/rand_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/values.h"
@@ skipping 46 matching lines @@
   return !state_.empty();
 }
 
 AckHandle::AckHandle(const std::string& state, base::Time timestamp)
     : state_(state), timestamp_(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();
 }
 
 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) &&
       ack_handle.ResetFromValue(*ack_handle_value);
 }
 
 }  // namespace syncer