Refactor common invalidation framework types

sync/notifier/object_id_invalidation_map.h

Index: sync/notifier/object_id_invalidation_map.h
diff --git a/sync/notifier/object_id_invalidation_map.h b/sync/notifier/object_id_invalidation_map.h
index bb97fb3b1337f63a9ee7b7200c6095d6da53a823..3af93f675a17ff4daa981bbb074e3f8cd9981349 100644
--- a/sync/notifier/object_id_invalidation_map.h
+++ b/sync/notifier/object_id_invalidation_map.h
@@ -1,4 +1,4 @@
-// Copyright 2012 The Chromium Authors. All rights reserved.
+// Copyright 2013 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.
 
@@ -6,40 +6,46 @@
 #define SYNC_NOTIFIER_OBJECT_ID_INVALIDATION_MAP_H_
 
 #include <map>
-#include <string>
+#include <vector>
 
-#include "base/basictypes.h"
-#include "base/memory/scoped_ptr.h"
-#include "google/cacheinvalidation/include/types.h"
 #include "sync/base/sync_export.h"
 #include "sync/internal_api/public/base/invalidation.h"
 #include "sync/notifier/invalidation_util.h"
-
-namespace base {
-class ListValue;
-}  // namespace base
+#include "sync/notifier/ordered_invalidation_list.h"
 
 namespace syncer {
 
-typedef std::map<invalidation::ObjectId,
-                 Invalidation,
-                 ObjectIdLessThan> ObjectIdInvalidationMap;
+class SYNC_EXPORT ObjectIdInvalidationMap {

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

What is this class supposed to do?  Looking at it I have a hard time figuring
out what 'WithObjects' is doing... at first I thought it was a factory function.

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

This class is supposed to be a container for many invalidations which may or may
not have different ObjectIds.

I made it a map because there aren't many interesting things one can do with two
invalidations that have different object IDs.  It's likely that the first thing
most clients will want to do with this object is iterate over the sets of
invalidations that share object IDs.

WithObjects is a helper to select the subset of contents of this map that have
the same ObjectId.  That's often a useful operation, but I guess I didn't do a
very good job of naming it.

Maybe GetInvalidationsWithIdsMatching(...) would be a better name?

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

I see.  So it's returning a subset / projection of the tuples in the full
mapping. I find it pretty informative to include the subset aspect of it in the
name (as opposed to builder/factory functions where you can create an object
from scratch WithObjects(...))

[Get]SubsetWith[Objects|Ids](...)?

I was also hunting for a class comment here :)

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

GetSubsetWithObjectIds() makes sense to me.
I've added one.  Check patch set 12 or later.

+  public:
+   static ObjectIdInvalidationMap InvalidateAll(const ObjectIdSet& ids);
+
+   ObjectIdInvalidationMap();
+   ~ObjectIdInvalidationMap();
+
+   ObjectIdSet GetObjectIds() const;
+   bool Empty() const;
+   bool operator==(const ObjectIdInvalidationMap& other) const;
+
+   void Insert(const Invalidation& invalidation);
+
+   ObjectIdInvalidationMap WithObjects(const ObjectIdSet& ids) const;

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

Are there any basic tests for this filtering?

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

No, there are no tests for any part of this class.  I'll fix that.

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

Great!

+   const OrderedInvalidationList& ForObject(invalidation::ObjectId id) const;
+   void GetAllInvalidations(std::vector<syncer::Invalidation>* out) const;
+
+   scoped_ptr<base::ListValue> ToValue() const;
+   bool ResetFromValue(const base::ListValue& value);
 
-// Converts between ObjectIdInvalidationMaps and ObjectIdSets.
-ObjectIdSet ObjectIdInvalidationMapToSet(
-    const ObjectIdInvalidationMap& invalidation_map);
-SYNC_EXPORT ObjectIdInvalidationMap ObjectIdSetToInvalidationMap(
-    const ObjectIdSet& ids, int64 version, const std::string& payload);
+   std::string ToString() const;
 
-SYNC_EXPORT bool ObjectIdInvalidationMapEquals(
-    const ObjectIdInvalidationMap& invalidation_map1,
-    const ObjectIdInvalidationMap& invalidation_map2);
+  private:
+   typedef std::map<invalidation::ObjectId,
+                    OrderedInvalidationList,
+                    ObjectIdLessThan> IdToListMap;
 
-scoped_ptr<base::ListValue> ObjectIdInvalidationMapToValue(
-    const ObjectIdInvalidationMap& invalidation_map);
+   ObjectIdInvalidationMap(const IdToListMap& map);
 
-bool ObjectIdInvalidationMapFromValue(const base::ListValue& value,
-                               ObjectIdInvalidationMap* out);
+   IdToListMap map_;
+};
 
 }  // namespace syncer