[Invalidations] Remove ObjectIDInvalidationMap

All usages already replaced with TopicInvalidationMap.

Additionally, this CL adds tests for TopicInvalidationMap (based on the
tests we had for ObjectIDInvalidationMap) and cleans up its interface
from redundant ToString() and ResetFromValue().

There should be no behavioral changes.

Bug: 1029698
Change-Id: I81c9e9aa5e00fd196fe8911b92f5c11b5d1f280a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2078473
Commit-Queue: Maksim Moskvitin <mmoskvitin@google.com>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#746362}

components/invalidation/impl/BUILD.gn

@@ -115,8 +115,8 @@ source_set("unit_tests") {
   if (!is_android) {
     # Non-Android tests.
     sources += [
-      "object_id_invalidation_map_unittest.cc",
       "single_object_invalidation_set_unittest.cc",
+      "topic_invalidation_map_unittest.cc",
       "unacked_invalidation_set_unittest.cc",
     ]
   }

components/invalidation/impl/object_id_invalidation_map_unittest.cc

-// Copyright 2014 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 "components/invalidation/public/object_id_invalidation_map.h"
-
-#include <memory>
-
-#include "google/cacheinvalidation/types.pb.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-namespace syncer {
-
-namespace {
-
-class ObjectIdInvalidationMapTest : public testing::Test {
- public:
-  ObjectIdInvalidationMapTest()
-      : kIdOne(ipc::invalidation::ObjectSource::TEST, "one"),
-        kIdTwo(ipc::invalidation::ObjectSource::TEST, "two"),
-        kInv1(Invalidation::Init(kIdOne, 10, "ten")) {
-    set1.insert(kIdOne);
-    set2.insert(kIdTwo);
-    all_set.insert(kIdOne);
-    all_set.insert(kIdTwo);
-
-    one_invalidation.Insert(kInv1);
-    invalidate_all = ObjectIdInvalidationMap::InvalidateAll(all_set);
-  }
-
- protected:
-  const invalidation::ObjectId kIdOne;
-  const invalidation::ObjectId kIdTwo;
-  const Invalidation kInv1;
-
-  ObjectIdSet set1;
-  ObjectIdSet set2;
-  ObjectIdSet all_set;
-  ObjectIdInvalidationMap empty;
-  ObjectIdInvalidationMap one_invalidation;
-  ObjectIdInvalidationMap invalidate_all;
-};
-
-TEST_F(ObjectIdInvalidationMapTest, Empty) {
-  EXPECT_TRUE(empty.Empty());
-  EXPECT_FALSE(one_invalidation.Empty());
-  EXPECT_FALSE(invalidate_all.Empty());
-}
-
-TEST_F(ObjectIdInvalidationMapTest, Equality) {
-  ObjectIdInvalidationMap empty2;
-  EXPECT_TRUE(empty == empty2);
-
-  ObjectIdInvalidationMap one_invalidation2;
-  one_invalidation2.Insert(kInv1);
-  EXPECT_TRUE(one_invalidation == one_invalidation2);
-
-  EXPECT_FALSE(empty == invalidate_all);
-}
-
-TEST_F(ObjectIdInvalidationMapTest, GetObjectIds) {
-  EXPECT_EQ(ObjectIdSet(), empty.GetObjectIds());
-  EXPECT_EQ(set1, one_invalidation.GetObjectIds());
-  EXPECT_EQ(all_set, invalidate_all.GetObjectIds());
-}
-
-TEST_F(ObjectIdInvalidationMapTest, GetSubsetWithObjectIds) {
-  EXPECT_TRUE(empty.GetSubsetWithObjectIds(set1).Empty());
-
-  EXPECT_TRUE(one_invalidation.GetSubsetWithObjectIds(set1) ==
-              one_invalidation);
-  EXPECT_TRUE(one_invalidation.GetSubsetWithObjectIds(all_set) ==
-              one_invalidation);
-  EXPECT_TRUE(one_invalidation.GetSubsetWithObjectIds(set2).Empty());
-
-  EXPECT_TRUE(invalidate_all.GetSubsetWithObjectIds(ObjectIdSet()).Empty());
-}
-
-TEST_F(ObjectIdInvalidationMapTest, SerializeEmpty) {
-  std::unique_ptr<base::ListValue> value = empty.ToValue();
-  ASSERT_TRUE(value.get());
-  ObjectIdInvalidationMap deserialized;
-  deserialized.ResetFromValue(*value);
-  EXPECT_TRUE(empty == deserialized);
-}
-
-TEST_F(ObjectIdInvalidationMapTest, SerializeOneInvalidation) {
-  std::unique_ptr<base::ListValue> value = one_invalidation.ToValue();
-  ASSERT_TRUE(value.get());
-  ObjectIdInvalidationMap deserialized;
-  deserialized.ResetFromValue(*value);
-  EXPECT_TRUE(one_invalidation == deserialized);
-}
-
-TEST_F(ObjectIdInvalidationMapTest, SerializeInvalidateAll) {
-  std::unique_ptr<base::ListValue> value = invalidate_all.ToValue();
-  ASSERT_TRUE(value.get());
-  ObjectIdInvalidationMap deserialized;
-  deserialized.ResetFromValue(*value);
-  EXPECT_TRUE(invalidate_all == deserialized);
-}
-
-}  // namespace
-
-}  // namespace syncer

components/invalidation/impl/topic_invalidation_map_unittest.cc

+// Copyright 2020 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 "components/invalidation/public/topic_invalidation_map.h"
+
+#include <memory>
+
+#include "google/cacheinvalidation/types.pb.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace syncer {
+
+namespace {
+
+// TODO(crbug.com/1056651): some methods aren't covered by tests, it's likely
+// worth adding them (especially for ToValue(), which is passed to js code).
+class TopicInvalidationMapTest : public testing::Test {
+ public:
+  TopicInvalidationMapTest()
+      : kTopicOne("one"),
+        kTopicTwo("two"),
+        kInv1(Invalidation::Init(kTopicOne, 10, "ten")) {
+    set1_.insert(kTopicOne);
+    set2_.insert(kTopicTwo);
+    all_set_.insert(kTopicOne);
+    all_set_.insert(kTopicTwo);
+
+    one_invalidation_.Insert(kInv1);
+    invalidate_all_.Insert(Invalidation::InitUnknownVersion(kTopicOne));
+    invalidate_all_.Insert(Invalidation::InitUnknownVersion(kTopicTwo));
+  }
+
+ protected:
+  const Topic kTopicOne;
+  const Topic kTopicTwo;
+  const Invalidation kInv1;
+
+  TopicSet set1_;
+  TopicSet set2_;
+  TopicSet all_set_;
+  TopicInvalidationMap empty_;
+  TopicInvalidationMap one_invalidation_;
+  TopicInvalidationMap invalidate_all_;
+};
+
+TEST_F(TopicInvalidationMapTest, Empty) {
+  EXPECT_TRUE(empty_.Empty());
+  EXPECT_FALSE(one_invalidation_.Empty());
+  EXPECT_FALSE(invalidate_all_.Empty());
+}
+
+TEST_F(TopicInvalidationMapTest, Equality) {
+  // TODO(crbug.com/1056651): equality operator is only used in tests, so maybe
+  // factor it away from the TopicInvalidationMap.
+  TopicInvalidationMap empty2;
+  EXPECT_EQ(empty_, empty2);
+
+  TopicInvalidationMap one_invalidation_2;
+  one_invalidation_2.Insert(kInv1);
+  EXPECT_EQ(one_invalidation_, one_invalidation_2);
+
+  EXPECT_FALSE(empty_ == invalidate_all_);
+}
+
+TEST_F(TopicInvalidationMapTest, GetTopics) {
+  EXPECT_EQ(TopicSet(), empty_.GetTopics());
+  EXPECT_EQ(set1_, one_invalidation_.GetTopics());
+  EXPECT_EQ(all_set_, invalidate_all_.GetTopics());
+}
+
+TEST_F(TopicInvalidationMapTest, GetSubsetWithTopics) {
+  EXPECT_TRUE(empty_.GetSubsetWithTopics(set1_).Empty());
+
+  EXPECT_EQ(one_invalidation_.GetSubsetWithTopics(set1_), one_invalidation_);
+  EXPECT_EQ(one_invalidation_.GetSubsetWithTopics(all_set_), one_invalidation_);
+  EXPECT_TRUE(one_invalidation_.GetSubsetWithTopics(set2_).Empty());
+
+  EXPECT_TRUE(invalidate_all_.GetSubsetWithTopics(TopicSet()).Empty());
+}
+
+}  // namespace
+
+}  // namespace syncer

components/invalidation/public/BUILD.gn

@@ -20,8 +20,6 @@ static_library("public") {
     "invalidation_util.h",
     "invalidator_state.cc",
     "invalidator_state.h",
-    "object_id_invalidation_map.cc",
-    "object_id_invalidation_map.h",
     "single_object_invalidation_set.cc",
     "single_object_invalidation_set.h",
     "topic_invalidation_map.cc",

components/invalidation/public/object_id_invalidation_map.cc

-// Copyright 2014 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 "components/invalidation/public/object_id_invalidation_map.h"
-
-#include <stddef.h>
-
-#include "base/json/json_string_value_serializer.h"
-
-namespace syncer {
-
-// static
-ObjectIdInvalidationMap ObjectIdInvalidationMap::InvalidateAll(
-    const ObjectIdSet& ids) {
-  ObjectIdInvalidationMap invalidate_all;
-  for (auto it = ids.begin(); it != ids.end(); ++it) {
-    invalidate_all.Insert(Invalidation::InitUnknownVersion(*it));
-  }
-  return invalidate_all;
-}
-
-ObjectIdInvalidationMap::ObjectIdInvalidationMap() {}
-
-ObjectIdInvalidationMap::ObjectIdInvalidationMap(
-    const ObjectIdInvalidationMap& other) = default;
-
-ObjectIdInvalidationMap::~ObjectIdInvalidationMap() {}
-
-ObjectIdSet ObjectIdInvalidationMap::GetObjectIds() const {
-  ObjectIdSet ret;
-  for (auto it = map_.begin(); it != map_.end(); ++it) {
-    ret.insert(it->first);
-  }
-  return ret;
-}
-
-bool ObjectIdInvalidationMap::Empty() const {
-  return map_.empty();
-}
-
-void ObjectIdInvalidationMap::Insert(const Invalidation& invalidation) {
-  map_[invalidation.object_id()].Insert(invalidation);
-}
-
-ObjectIdInvalidationMap ObjectIdInvalidationMap::GetSubsetWithObjectIds(
-    const ObjectIdSet& ids) const {
-  IdToListMap new_map;
-  for (auto it = ids.begin(); it != ids.end(); ++it) {
-    auto lookup = map_.find(*it);
-    if (lookup != map_.end()) {
-      new_map[*it] = lookup->second;
-    }
-  }
-  return ObjectIdInvalidationMap(new_map);
-}
-
-const SingleObjectInvalidationSet& ObjectIdInvalidationMap::ForObject(
-    invalidation::ObjectId id) const {
-  auto lookup = map_.find(id);
-  DCHECK(lookup != map_.end());
-  DCHECK(!lookup->second.IsEmpty());
-  return lookup->second;
-}
-
-void ObjectIdInvalidationMap::GetAllInvalidations(
-    std::vector<syncer::Invalidation>* out) const {
-  for (auto it = map_.begin(); it != map_.end(); ++it) {
-    out->insert(out->begin(), it->second.begin(), it->second.end());
-  }
-}
-void ObjectIdInvalidationMap::AcknowledgeAll() const {
-  for (auto it1 = map_.begin(); it1 != map_.end(); ++it1) {
-    for (auto it2 = it1->second.begin(); it2 != it1->second.end(); ++it2) {
-      it2->Acknowledge();
-    }
-  }
-}
-
-bool ObjectIdInvalidationMap::operator==(
-    const ObjectIdInvalidationMap& other) const {
-  return map_ == other.map_;
-}
-
-std::unique_ptr<base::ListValue> ObjectIdInvalidationMap::ToValue() const {
-  std::unique_ptr<base::ListValue> value(new base::ListValue());
-  for (auto it1 = map_.begin(); it1 != map_.end(); ++it1) {
-    for (auto it2 = it1->second.begin(); it2 != it1->second.end(); ++it2) {
-      value->Append(it2->ToValue());
-    }
-  }
-  return value;
-}
-
-bool ObjectIdInvalidationMap::ResetFromValue(const base::ListValue& value) {
-  map_.clear();
-  for (size_t i = 0; i < value.GetSize(); ++i) {
-    const base::DictionaryValue* dict;
-    if (!value.GetDictionary(i, &dict)) {
-      return false;
-    }
-    std::unique_ptr<Invalidation> invalidation =
-        Invalidation::InitFromValue(*dict);
-    if (!invalidation) {
-      return false;
-    }
-    Insert(*invalidation);
-  }
-  return true;
-}
-
-std::string ObjectIdInvalidationMap::ToString() const {
-  std::string output;
-  JSONStringValueSerializer serializer(&output);
-  serializer.set_pretty_print(true);
-  serializer.Serialize(*ToValue());
-  return output;
-}
-
-ObjectIdInvalidationMap::ObjectIdInvalidationMap(const IdToListMap& map)
-  : map_(map) {}
-
-}  // namespace syncer

components/invalidation/public/object_id_invalidation_map.h

-// Copyright 2014 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.
-
-#ifndef COMPONENTS_INVALIDATION_PUBLIC_OBJECT_ID_INVALIDATION_MAP_H_
-#define COMPONENTS_INVALIDATION_PUBLIC_OBJECT_ID_INVALIDATION_MAP_H_
-
-#include <map>
-#include <memory>
-#include <vector>
-
-#include "components/invalidation/public/invalidation.h"
-#include "components/invalidation/public/invalidation_export.h"
-#include "components/invalidation/public/invalidation_util.h"
-#include "components/invalidation/public/single_object_invalidation_set.h"
-
-namespace syncer {
-
-// A set of notifications with some helper methods to organize them by object ID
-// and version number.
-class INVALIDATION_EXPORT ObjectIdInvalidationMap {
-  public:
-   // Creates an invalidation map that includes an 'unknown version'
-   // invalidation for each specified ID in |ids|.
-   static ObjectIdInvalidationMap InvalidateAll(const ObjectIdSet& ids);
-
-   ObjectIdInvalidationMap();
-   ObjectIdInvalidationMap(const ObjectIdInvalidationMap& other);
-   ~ObjectIdInvalidationMap();
-
-   // Returns set of ObjectIds for which at least one invalidation is present.
-   ObjectIdSet GetObjectIds() const;
-
-   // Returns true if this map contains no invalidations.
-   bool Empty() const;
-
-   // Returns true if both maps contain the same set of invalidations.
-   bool operator==(const ObjectIdInvalidationMap& other) const;
-
-   // Inserts a new invalidation into this map.
-   void Insert(const Invalidation& invalidation);
-
-   // Returns a new map containing the subset of invaliations from this map
-   // whose IDs were in the specified |ids| set.
-   ObjectIdInvalidationMap GetSubsetWithObjectIds(const ObjectIdSet& ids) const;
-
-   // Returns the subset of invalidations with IDs matching |id|.
-   const SingleObjectInvalidationSet& ForObject(
-       invalidation::ObjectId id) const;
-
-   // Returns the contents of this map in a single vector.
-   void GetAllInvalidations(std::vector<syncer::Invalidation>* out) const;
-
-   // Call Acknowledge() on all contained Invalidations.
-   void AcknowledgeAll() const;
-
-   // Serialize this map to a value.
-   std::unique_ptr<base::ListValue> ToValue() const;
-
-   // Deserialize the value into a map and use it to re-initialize this object.
-   bool ResetFromValue(const base::ListValue& value);
-
-   // Prints the contentes of this map as a human-readable string.
-   std::string ToString() const;
-
-  private:
-   typedef std::map<invalidation::ObjectId,
-                    SingleObjectInvalidationSet,
-                    ObjectIdLessThan> IdToListMap;
-
-   ObjectIdInvalidationMap(const IdToListMap& map);
-
-   IdToListMap map_;
-};
-
-}  // namespace syncer
-
-#endif  // COMPONENTS_INVALIDATION_PUBLIC_OBJECT_ID_INVALIDATION_MAP_H_

components/invalidation/public/topic_invalidation_map.cc

@@ -6,8 +6,7 @@
 
 #include <stddef.h>
 
-#include "base/json/json_string_value_serializer.h"
-#include "components/invalidation/public/object_id_invalidation_map.h"
+#include "base/values.h"
 
 namespace syncer {
 
@@ -35,7 +34,7 @@ void TopicInvalidationMap::Insert(const Invalidation& invalidation) {
 
 TopicInvalidationMap TopicInvalidationMap::GetSubsetWithTopics(
     const Topics& topics) const {
-  TopicToListMap new_map;
+  std::map<Topic, SingleObjectInvalidationSet> new_map;
   for (const auto& topic : topics) {
     auto lookup = map_.find(topic.first);
     if (lookup != map_.end()) {
@@ -45,6 +44,18 @@ TopicInvalidationMap TopicInvalidationMap::GetSubsetWithTopics(
   return TopicInvalidationMap(new_map);
 }
 
+TopicInvalidationMap TopicInvalidationMap::GetSubsetWithTopics(
+    const TopicSet& topics) const {
+  std::map<Topic, SingleObjectInvalidationSet> new_map;
+  for (const auto& topic : topics) {
+    auto lookup = map_.find(topic);
+    if (lookup != map_.end()) {
+      new_map[topic] = lookup->second;
+    }
+  }
+  return TopicInvalidationMap(new_map);
+}
+
 const SingleObjectInvalidationSet& TopicInvalidationMap::ForTopic(
     Topic topic) const {
   auto lookup = map_.find(topic);
@@ -82,54 +93,8 @@ std::unique_ptr<base::ListValue> TopicInvalidationMap::ToValue() const {
   return value;
 }
 
-bool TopicInvalidationMap::ResetFromValue(const base::ListValue& value) {
-  map_.clear();
-  for (size_t i = 0; i < value.GetSize(); ++i) {
-    const base::DictionaryValue* dict;
-    if (!value.GetDictionary(i, &dict)) {
-      return false;
-    }
-    std::unique_ptr<Invalidation> invalidation =
-        Invalidation::InitFromValue(*dict);
-    if (!invalidation) {
-      return false;
-    }
-    Insert(*invalidation);
-  }
-  return true;
-}
-
-std::string TopicInvalidationMap::ToString() const {
-  std::string output;
-  JSONStringValueSerializer serializer(&output);
-  serializer.set_pretty_print(true);
-  serializer.Serialize(*ToValue());
-  return output;
-}
-
-TopicInvalidationMap::TopicInvalidationMap(const TopicToListMap& map)
+TopicInvalidationMap::TopicInvalidationMap(
+    const std::map<Topic, SingleObjectInvalidationSet>& map)
     : map_(map) {}
 
-TopicInvalidationMap ConvertObjectIdInvalidationMapToTopicInvalidationMap(
-    ObjectIdInvalidationMap object_ids_map) {
-  TopicInvalidationMap topics_map;
-  std::vector<Invalidation> invalidations;
-  object_ids_map.GetAllInvalidations(&invalidations);
-  for (const auto& invalidation : invalidations) {
-    topics_map.Insert(invalidation);
-  }
-  return topics_map;
-}
-
-ObjectIdInvalidationMap ConvertTopicInvalidationMapToObjectIdInvalidationMap(
-    const TopicInvalidationMap& topics_map) {
-  ObjectIdInvalidationMap object_ids_map;
-  std::vector<Invalidation> invalidations;
-  topics_map.GetAllInvalidations(&invalidations);
-  for (const auto& invalidation : invalidations) {
-    object_ids_map.Insert(invalidation);
-  }
-  return object_ids_map;
-}
-
 }  // namespace syncer

components/invalidation/public/topic_invalidation_map.h

@@ -14,8 +14,11 @@
 #include "components/invalidation/public/invalidation_util.h"
 #include "components/invalidation/public/single_object_invalidation_set.h"
 
+namespace base {
+class ListValue;
+}  // namespace base
+
 namespace syncer {
-class ObjectIdInvalidationMap;
 
 // A set of notifications with some helper methods to organize them by object ID
 // and version number.
@@ -38,11 +41,17 @@ class INVALIDATION_EXPORT TopicInvalidationMap {
   void Insert(const Invalidation& invalidation);
 
   // Returns a new map containing the subset of invaliations from this map
-  // whose IDs were in the specified |ids| set.
-  TopicInvalidationMap GetSubsetWithTopics(const Topics& ids) const;
+  // whose topic were in the specified |topics|.
+  // TODO(crbug.com/1029698): replace all usages with the version below and
+  // remove this method.
+  TopicInvalidationMap GetSubsetWithTopics(const Topics& topics) const;
+
+  // Returns a new map containing the subset of invaliations from this map
+  // whose topic were in the specified |topics|.
+  TopicInvalidationMap GetSubsetWithTopics(const TopicSet& topics) const;
 
-  // Returns the subset of invalidations with IDs matching |id|.
-  const SingleObjectInvalidationSet& ForTopic(Topic id) const;
+  // Returns the subset of invalidations with Topic matching |topic|.
+  const SingleObjectInvalidationSet& ForTopic(Topic topic) const;
 
   // Returns the contents of this map in a single vector.
   void GetAllInvalidations(std::vector<syncer::Invalidation>* out) const;
@@ -50,29 +59,16 @@ class INVALIDATION_EXPORT TopicInvalidationMap {
   // Call Acknowledge() on all contained Invalidations.
   void AcknowledgeAll() const;
 
-  // Serialize this map to a value.
+  // Serialize this map to a value. Used to expose value on
+  // chrome://invalidations page.
   std::unique_ptr<base::ListValue> ToValue() const;
 
-  // Deserialize the value into a map and use it to re-initialize this object.
-  bool ResetFromValue(const base::ListValue& value);
-
-  // Prints the contentes of this map as a human-readable string.
-  std::string ToString() const;
-
  private:
-  typedef std::map<Topic, SingleObjectInvalidationSet> TopicToListMap;
-
-  TopicInvalidationMap(const TopicToListMap& map);
+  TopicInvalidationMap(const std::map<Topic, SingleObjectInvalidationSet>& map);
 
-  TopicToListMap map_;
+  std::map<Topic, SingleObjectInvalidationSet> map_;
 };
 
-TopicInvalidationMap ConvertObjectIdInvalidationMapToTopicInvalidationMap(
-    ObjectIdInvalidationMap object_ids_map);
-
-ObjectIdInvalidationMap ConvertTopicInvalidationMapToObjectIdInvalidationMap(
-    const TopicInvalidationMap& topics_map);
-
 }  // namespace syncer
 
 #endif  // COMPONENTS_INVALIDATION_PUBLIC_TOPIC_INVALIDATION_MAP_H_