Refactor common invalidation framework types

sync/notifier/sync_invalidation_listener.cc

Index: sync/notifier/sync_invalidation_listener.cc
diff --git a/sync/notifier/sync_invalidation_listener.cc b/sync/notifier/sync_invalidation_listener.cc
index ed07f2070b7fdc4398758deacd2565c5d69f27ad..1528be5b05b2a32af0341173b4a796ed3d86b3c7 100644
--- a/sync/notifier/sync_invalidation_listener.cc
+++ b/sync/notifier/sync_invalidation_listener.cc
@@ -22,6 +22,8 @@ namespace {
 
 const char kApplicationName[] = "chrome-sync";
 
+static const int64 kUnknownVersion = -1;
+
 }  // namespace
 
 namespace syncer {
@@ -220,7 +222,7 @@ void SyncInvalidationListener::InvalidateUnknownVersion(
   ids.insert(object_id);
   PrepareInvalidation(
       ids,
-      Invalidation::kUnknownVersion,
+      kUnknownVersion,
       std::string(),
       client,
       ack_handle);
@@ -237,7 +239,7 @@ void SyncInvalidationListener::InvalidateAll(
 
   PrepareInvalidation(
       registered_ids_,
-      Invalidation::kUnknownVersion,
+      kUnknownVersion,
       std::string(),
       client,
       ack_handle);
@@ -275,13 +277,22 @@ void SyncInvalidationListener::EmitInvalidation(
     const invalidation::AckHandle& ack_handle,
     const AckHandleMap& local_ack_handles) {
   DCHECK(CalledOnValidThread());
-  ObjectIdInvalidationMap invalidation_map =
-      ObjectIdSetToInvalidationMap(ids, version, payload);
+
+  ObjectIdInvalidationMap invalidation_map;
   for (AckHandleMap::const_iterator it = local_ack_handles.begin();
        it != local_ack_handles.end(); ++it) {
     // Update in-memory copy of the invalidation state.
     invalidation_state_map_[it->first].expected = it->second;
-    invalidation_map[it->first].ack_handle = it->second;
+
+    if (version == kUnknownVersion) {

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

So is the contract with clients such that they decide whether they want to use X
as an "unknown version", but the underlying Invalidation object supports
whatever? Add a comment to this effect where you define kUnknownVersion.

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

No, I've rewritten the interface to the Invalidation class to force clients to
explicitly check the IsUnknownVersion() flag.  The TICL API does not specify a
version or payload on unknown version invalidations, so there is no valid value
for those fields on an unknown version invalidation.

The only reason kUnknownVersion still exists is that I haven't refactored the
"PrepareInvalidation" function yet.  That will be part of the next patch.

+      Invalidation inv = Invalidation::InitUnknownVersion(it->first);
+      inv.SetAckHandle(it->second);
+      invalidation_map.Insert(inv);
+    } else {
+      Invalidation inv = Invalidation::Init(it->first, version, payload);
+      inv.SetAckHandle(it->second);
+      invalidation_map.Insert(inv);
+    }
   }
   ack_tracker_.Track(ids);
   delegate_->OnInvalidate(invalidation_map);
@@ -291,13 +302,19 @@ void SyncInvalidationListener::EmitInvalidation(
 void SyncInvalidationListener::OnTimeout(const ObjectIdSet& ids) {
   ObjectIdInvalidationMap invalidation_map;
   for (ObjectIdSet::const_iterator it = ids.begin(); it != ids.end(); ++it) {
-    Invalidation invalidation;
-    invalidation.ack_handle = invalidation_state_map_[*it].expected;
-    invalidation.version = invalidation_state_map_[*it].version;
-    invalidation.payload = invalidation_state_map_[*it].payload;
-    invalidation_map.insert(std::make_pair(*it, invalidation));
+    Invalidation inv;
+    if (invalidation_state_map_[*it].version == kUnknownVersion) {
+      inv = Invalidation::InitUnknownVersion(*it);
+      inv.SetAckHandle(invalidation_state_map_[*it].expected);
+    } else {
+      inv = Invalidation::Init(
+          *it,
+          invalidation_state_map_[*it].version,
+          invalidation_state_map_[*it].payload);
+      inv.SetAckHandle(invalidation_state_map_[*it].expected);
+    }
+    invalidation_map.Insert(inv);
   }
-
   delegate_->OnInvalidate(invalidation_map);
 }