Refactor common invalidation framework types

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 b503d07c4afa99905a688d70eeb85c8d6d80a425..fe64ccd6670ae42785fcb8dab5211936d14c63fd 100644
--- a/sync/internal_api/public/base/invalidation.cc
+++ b/sync/internal_api/public/base/invalidation.cc
@@ -5,100 +5,132 @@
 #include "sync/internal_api/public/base/invalidation.h"
 
 #include <cstddef>
+
+#include "base/json/json_string_value_serializer.h"
 #include "base/rand_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/values.h"
+#include "sync/notifier/invalidation_util.h"
 
 namespace syncer {
 
 namespace {
-// Hopefully enough bytes for uniqueness.
-const size_t kBytesInHandle = 16;
-}  // namespace
-
-AckHandle AckHandle::CreateUnique() {
-  // This isn't a valid UUID, so we don't attempt to format it like one.
-  uint8 random_bytes[kBytesInHandle];
-  base::RandBytes(random_bytes, sizeof(random_bytes));
-  return AckHandle(base::HexEncode(random_bytes, sizeof(random_bytes)),
-                   base::Time::Now());
+const char kObjectIdKey[] = "objectId";
+const char kIsUnknownVersionKey[] = "isUnknownVersion";
+const char kVersionKey[] = "version";
+const char kPayloadKey[] = "payload";
 }
 
-AckHandle AckHandle::InvalidAckHandle() {
-  return AckHandle(std::string(), base::Time());
+Invalidation Invalidation::Init(
+    const invalidation::ObjectId& id,
+    int64 version,
+    const std::string& payload) {
+  return Invalidation(id, false, version, payload, AckHandle::CreateUnique());
 }
 
-bool AckHandle::Equals(const AckHandle& other) const {
-  return state_ == other.state_ && timestamp_ == other.timestamp_;
+Invalidation Invalidation::InitUnknownVersion(
+    const invalidation::ObjectId& id) {
+  return Invalidation(id, true, -1, "", AckHandle::CreateUnique());

[tim (not reviewing), 2013/09/20 21:53:46]

std::string vs ""

[rlarocque, 2013/09/23 18:38:19]

Done.

 }
 
-scoped_ptr<base::DictionaryValue> AckHandle::ToValue() const {
-  scoped_ptr<base::DictionaryValue> value(new base::DictionaryValue());
-  value->SetString("state", state_);
-  value->SetString("timestamp",
-                   base::Int64ToString(timestamp_.ToInternalValue()));
-  return value.Pass();
-}
+Invalidation::Invalidation()
+  : ack_handle_(AckHandle::InvalidAckHandle()) {}
 
-bool AckHandle::ResetFromValue(const base::DictionaryValue& value) {
-  if (!value.GetString("state", &state_))
-    return false;
-  std::string timestamp_as_string;
-  if (!value.GetString("timestamp", &timestamp_as_string))
-    return false;
-  int64 timestamp_value;
-  if (!base::StringToInt64(timestamp_as_string, &timestamp_value))
-    return false;
-  timestamp_ = base::Time::FromInternalValue(timestamp_value);
-  return true;
-}
+Invalidation::~Invalidation() {}
 
-bool AckHandle::IsValid() const {
-  return !state_.empty();
+invalidation::ObjectId Invalidation::GetObjectId() const {

[tim (not reviewing), 2013/09/20 21:53:46]

style guide would have this called id().  Or object_id() if you change the
variable name.

Same for is_unknown_version_, payload, ack_handle...

[rlarocque, 2013/09/23 18:38:19]

True.  Lately, I've been reluctant to follow that guideline, because it means
that I'll have to change a lot of code if I ever decide to change the
implementation of a trivial member function into something less trivial.  

But that's a discussion for chromium-dev, not this CL.  I'll bring this code
into conformance with the guidelines.

+  return id_;
 }
 
-AckHandle::AckHandle(const std::string& state, base::Time timestamp)
-    : state_(state), timestamp_(timestamp) {
+bool Invalidation::IsUnknownVersion() const {
+  return is_unknown_version_;
 }
 
-AckHandle::~AckHandle() {
+int64 Invalidation::GetVersion() const {
+  DCHECK(!is_unknown_version_);

[tim (not reviewing), 2013/09/20 21:53:46]

should be named version()... this is logically just an accessor, and the DCHECK
doesn't impact performance (which would be the only real reason to deviate and
use mixed-case function naming).

[rlarocque, 2013/09/23 18:38:19]

Done.

+  return version_;
 }
 
-const int64 Invalidation::kUnknownVersion = -1;
+const std::string& Invalidation::GetPayload() const {
+  DCHECK(!is_unknown_version_);
+  return payload_;
+}
 
-Invalidation::Invalidation()
-    : version(kUnknownVersion), ack_handle(AckHandle::InvalidAckHandle()) {
+const AckHandle& Invalidation::GetAckHandle() const {
+  return ack_handle_;
 }
 
-Invalidation::~Invalidation() {
+void Invalidation::SetAckHandle(const AckHandle& ack_handle) {
+  ack_handle_ = ack_handle;
 }
 
-bool Invalidation::Equals(const Invalidation& other) const {
-  return (version == other.version) && (payload == other.payload) &&
-      ack_handle.Equals(other.ack_handle);
+bool Invalidation::operator==(const Invalidation& other) const {
+  return id_ == other.id_
+      && is_unknown_version_ == other.is_unknown_version_
+      && version_ == other.version_
+      && payload_ == other.payload_;

[tim (not reviewing), 2013/09/20 21:53:46]

You're not including ack_handle_, was that intentional?  Also, tests with
appropriate use of a factory function could help protect a method like this from
breaking down in the future.

[rlarocque, 2013/09/23 18:38:19]

It was more or less intentional.  The decision will make more sense in the next
patch.

My plan is to replace these IDs with WeakHandles that point back at the
SyncInvalidationListener, so clients can call invalidation.Acknowledge() from
any thread.  It's also a lot safer this way.  We won't have to worry about
sending acknowledgements that were generated by one SyncInvalidationListener to
a newer one, which can happen today if the invalidator is torn down and
restarted.

Once that change is made, it won't make sense to include the ack_handle_ in the
comparison, because there won't be any ack_handle_.

[tim (not reviewing), 2013/09/24 21:16:54]

That makes sense.  However in general, and especially in cases where it's not a
straight POD object and == works in the most trivial of ways, you should provide
your own Equals method (as the previous code had) instead of overloading
operators so that you need to be explicitly aware of what's happening.  See also
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Operat...

[rlarocque, 2013/09/25 00:40:00]

Fixed.  I had to add some boilerplate to get some STL comparisons to work, but
it wasn't as hard as I thought it would be.

 }
 
 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());
+  value->Set(kObjectIdKey, ObjectIdToValue(id_).release());
+  if (is_unknown_version_) {
+    value->SetBoolean(kIsUnknownVersionKey, true);
+  } else {
+    value->SetBoolean(kIsUnknownVersionKey, false);
+    value->SetString(kVersionKey, base::Int64ToString(version_));
+    value->SetString(kPayloadKey, payload_);
+  }
   return value.Pass();
 }
 
 bool Invalidation::ResetFromValue(const base::DictionaryValue& value) {
-  const base::DictionaryValue* ack_handle_value = NULL;
-  std::string version_as_string;
-  if (value.GetString("version", &version_as_string)) {
-    if (!base::StringToInt64(version_as_string, &version))
-      return false;
+  const base::DictionaryValue* object_id_dict;
+  if (!value.GetDictionary(kObjectIdKey, &object_id_dict)
+      || !ObjectIdFromValue(*object_id_dict, &id_)) {
+    DLOG(WARNING) << "Failed to parse id";
+    return false;
+  }
+  if (!value.GetBoolean(kIsUnknownVersionKey, &is_unknown_version_)) {
+    DLOG(WARNING) << "Failed to parse is_unknown_version flag";
+    return false;
+  }
+  if (is_unknown_version_) {
+    version_ = -1;
+    payload_ = "";

[tim (not reviewing), 2013/09/20 21:53:46]

std::string

[rlarocque, 2013/09/23 18:38:19]

Done.

   } else {
-    version = kUnknownVersion;
+    std::string version_as_string;
+    if (!value.GetString(kVersionKey, &version_as_string)
+        || !base::StringToInt64(version_as_string, &version_)) {
+      DLOG(WARNING) << "Failed to parse version";
+      return false;
+    }
+    if (!value.GetString(kPayloadKey, &payload_)) {
+      DLOG(WARNING) << "Failed to parse payload";
+      return false;
+    }
   }
-  return
-      value.GetString("payload", &payload) &&
-      value.GetDictionary("ackHandle", &ack_handle_value) &&
-      ack_handle.ResetFromValue(*ack_handle_value);
+  return true;
 }
 
+std::string Invalidation::ToString() const {
+  std::string output;
+  JSONStringValueSerializer serializer(&output);
+  serializer.set_pretty_print(true);
+  serializer.Serialize(*ToValue().get());
+  return output;
+}
+
+Invalidation::Invalidation(
+    const invalidation::ObjectId& id,
+    bool is_unknown_version,
+    int64 version,
+    const std::string& payload,
+    AckHandle ack_handle)
+  : id_(id),
+    is_unknown_version_(is_unknown_version),
+    version_(version),
+    payload_(payload),
+    ack_handle_(ack_handle) {}
+
 }  // namespace syncer