Refactor common invalidation framework types

sync/internal_api/public/base/invalidation.h

Index: sync/internal_api/public/base/invalidation.h
diff --git a/sync/internal_api/public/base/invalidation.h b/sync/internal_api/public/base/invalidation.h
index 851dbed747338433be6db6ba745318c87040b3bc..0e1d7a606c91f9c024826e6f8634da1f09a624dd 100644
--- a/sync/internal_api/public/base/invalidation.h
+++ b/sync/internal_api/public/base/invalidation.h
@@ -9,59 +9,68 @@
 
 #include "base/basictypes.h"
 #include "base/memory/scoped_ptr.h"
-#include "base/time/time.h"
+#include "base/values.h"
+#include "google/cacheinvalidation/include/types.h"
 #include "sync/base/sync_export.h"
-
-namespace base {
-class DictionaryValue;
-}  // namespace
+#include "sync/internal_api/public/base/ack_handle.h"
 
 namespace syncer {
 
-// Opaque class that represents a local ack handle. We don't reuse the
-// invalidation ack handles to avoid unnecessary dependencies.
-class SYNC_EXPORT AckHandle {
- public:
-  static AckHandle CreateUnique();
-  static AckHandle InvalidAckHandle();
-
-  bool Equals(const AckHandle& other) const;
-
-  scoped_ptr<base::DictionaryValue> ToValue() const;
-  bool ResetFromValue(const base::DictionaryValue& value);
+class DroppedInvalidationTracker;
+class AckHandler;
 
-  bool IsValid() const;
+// Represents a local invalidation, and is roughly analogous to
+// invalidation::Invalidation.

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

This comment raises the question of why we need it... can you distinguish it
from invalidation::Invalidation in some way here?

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

Added some comments.

The main difference is support for local ack-tracking.  This requires it to
include ack handles and serialization functions.  There are also some difference
in the API, but I intend to minimize those in the next patch. 

+class SYNC_EXPORT Invalidation {
+ public:
+  // Factory functions.
+  static Invalidation Init(
+      const invalidation::ObjectId& id,
+      int64 version,
+      const std::string& payload);
+  static Invalidation InitWithAckHandle(
+      const invalidation::ObjectId& id,
+      int64 version,
+      const std::string& payload);
+  static Invalidation InitUnknownVersion(const invalidation::ObjectId& id);
+  static Invalidation InitUnknownVersionWithAckHandle(
+      const invalidation::ObjectId& id);
 
-  ~AckHandle();
+  Invalidation();

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

Remove this?

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

I can't remove support for the zero-argument constructor, because we need it for
statements of the form

Invalidation i = Invalidation::Init(...);

I think it's also useful to explicitly define this constructor, since I'd like
to provide some reasonable values for its members in this case.

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

You don't need a default constructor for statements like that, you need a copy
constructor. If you omit it altogether you don't have to worry about providing
reasonable values, because no one could ever invoke it (because you have a
private: constructor below).

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

Right.  That was a bad example.

I might have remembered a real reason for it, though.  The function
Invalidation::ResetFromValue() must be able to fail.  I can see a few ways to
implement this:

- Have it return an Invalidation*, with NULL representing failure.  That gets a
bit weird, since it brings up a bunch of memory ownership issues that I'd rather
avoid.

- Make it a factory method that returns an Invalidation.  Then we would have to
maintain and check some flag to see if the invalidation it returned was actually
valid.

- The current solution, which is to have it operate on an existing Invalidation
and return a bool.  We can check the return value to see whether or not it
failed.

I don't think there's any solution that doesn't require either the use of an
Invalidation* or the concept of an "invalid" Invalidation.  This constructor
allows us to create an uninitialized Invalidation that can be initialized later
by ResetFromValue().

+  ~Invalidation();
 
- private:
-  // Explicitly copyable and assignable for STL containers.
-  AckHandle(const std::string& state, base::Time timestamp);
+  // Compares two invalidations.  The comparison ignores ack-tracking state.
+  bool operator==(const Invalidation& other) const;
 
-  std::string state_;
-  base::Time timestamp_;
-};
+  invalidation::ObjectId GetObjectId() const;
+  bool IsUnknownVersion() const;
 
-// Represents a local invalidation, and is roughly analogous to
-// invalidation::Invalidation. It contains a version (which may be
-// kUnknownVersion), a payload (which may be empty) and an
-// associated ack handle that an InvalidationHandler implementation can use to
-// acknowledge receipt of the invalidation. It does not embed the object ID,
-// since it is typically associated with it through ObjectIdInvalidationMap.
-struct SYNC_EXPORT Invalidation {
-  static const int64 kUnknownVersion;
+  // Safe to call only if IsUnknownVersion() returns false.
+  int64 GetVersion() const;
 
-  Invalidation();
-  ~Invalidation();
+  // Safe to call only if IsUnknownVersion() returns false.
+  const std::string& GetPayload() const;
 
-  bool Equals(const Invalidation& other) const;
+  const AckHandle& GetAckHandle() const;
+  void SetAckHandle(const AckHandle& ack_handle);
 
   scoped_ptr<base::DictionaryValue> ToValue() const;
   bool ResetFromValue(const base::DictionaryValue& value);
+  std::string ToString() const;
 
-  int64 version;
-  std::string payload;
-  AckHandle ack_handle;
+ private:
+  Invalidation(const invalidation::ObjectId& id,
+               bool is_unknown_version,
+               int64 version,
+               const std::string& payload,
+               AckHandle ack_handle);
+
+  invalidation::ObjectId id_;
+  bool is_unknown_version_;
+  int64 version_;
+  std::string payload_;
+
+  AckHandle ack_handle_;

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

It's worth commenting these fields (some, at least).  e.g payload may be empty,
what is ack_handle for, ...

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

Done.

 };
 
 }  // namespace syncer