[Invalidations] Introduce dedicated class for Topic

This CL introduces new class for invalidation Topics, it's temporary
called TopicData, but will be renamed to Topic once legacy Topic alias
usages will be removed.

TopicData contains topic name and a boolean to identify whether topic
is public. It would allow us to simplify external API and clean up
internal usages of Topics - having TopicMetadata and Topics in some
places and Topic names and TopicSet in other places is confusing.

This CL modifies
InvalidatorRegistrarWithMemory::UpdateRegisteredTopics() to use the new
class. Internal usages in InvalidatorRegistrarWithMemory also updated,
but a bunch of methods exposes old Topics, because they are used
outside components/invalidation and it's better to update them on later
stages.

There is one potential behavioral change in case of existing bug in the
code: topics subscriptions were stored in prefs without |is_public|
field, before migration was introduced in
https://cr-rev.appspot.com/c/1570963. This migration assumes that all
previously stored topics are private and likely happened when there was
no public topics in launched projects. If this statement isn't
accurate, then there is a behavioral change:
Before: public topic subscription loaded from legacy prefs, the code
assumes it's a private topic until the same topic is passed into
UpdateInterestedTopics().
After: buggy version of public topic remains in "interesting" state,
once UpdateInterestedTopics() is called with correct TopicData there
are two version of this Topic (one is private (buggy) and one is public
(correct)).
It happens because |is_public| become a part of Topic identifier.

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

components/invalidation/impl/fake_invalidation_service.cc

@@ -32,13 +32,12 @@ void FakeInvalidationService::RegisterInvalidationHandler(
 
 bool FakeInvalidationService::UpdateInterestedTopics(
     syncer::InvalidationHandler* handler,
-    const syncer::TopicSet& topics) {
-  syncer::Topics topic_map;
-  for (const auto& topic : topics) {
-    topic_map.emplace(topic,
-                      syncer::TopicMetadata{handler->IsPublicTopic(topic)});
+    const syncer::TopicSet& legacy_topic_set) {
+  std::set<TopicData> topic_set;
+  for (const auto& topic_name : legacy_topic_set) {
+    topic_set.insert(TopicData(topic_name, handler->IsPublicTopic(topic_name)));
   }
-  return invalidator_registrar_->UpdateRegisteredTopics(handler, topic_map);
+  return invalidator_registrar_->UpdateRegisteredTopics(handler, topic_set);
 }
 
 void FakeInvalidationService::UnregisterInvalidationHandler(

components/invalidation/impl/fcm_invalidation_service_base.cc

@@ -14,6 +14,7 @@
 #include "components/invalidation/impl/fcm_network_handler.h"
 #include "components/invalidation/impl/invalidation_prefs.h"
 #include "components/invalidation/public/invalidator_state.h"
+#include "components/invalidation/public/topic_data.h"
 #include "components/invalidation/public/topic_invalidation_map.h"
 #include "components/prefs/scoped_user_pref_update.h"
 
@@ -84,24 +85,20 @@ void FCMInvalidationServiceBase::RegisterInvalidationHandler(
 
 bool FCMInvalidationServiceBase::UpdateInterestedTopics(
     syncer::InvalidationHandler* handler,
-    const syncer::TopicSet& topic_set) {
+    const syncer::TopicSet& legacy_topic_set) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   update_was_requested_ = true;
-  DVLOG(2) << "Subscribing to topics: " << topic_set.size();
-  // TODO(crbug.com/1029698): |is_public| should be a part of dedicated Topic
-  // type, that would allow to avoid the conversion below and removal of
-  // syncer::Topics, syncer::TopicMetadata and
-  // InvalidationHandler::IsPublicTopic().
-  syncer::Topics topic_map;
-  for (const auto& topic : topic_set) {
-    topic_map.emplace(topic,
-                      syncer::TopicMetadata{handler->IsPublicTopic(topic)});
+  DVLOG(2) << "Subscribing to topics: " << legacy_topic_set.size();
+  std::set<TopicData> topic_set;
+  for (const auto& topic_name : legacy_topic_set) {
+    topic_set.insert(TopicData(topic_name, handler->IsPublicTopic(topic_name)));
   }
   // TODO(crbug.com/1054404): UpdateRegisteredTopics() should be renamed to
   // clarify that it actually updates whether topics need subscription (aka
   // interested).
-  if (!invalidator_registrar_.UpdateRegisteredTopics(handler, topic_map))
+  if (!invalidator_registrar_.UpdateRegisteredTopics(handler, topic_set)) {
     return false;
+  }
   DoUpdateSubscribedTopicsIfNeeded();
   logger_.OnUpdatedTopics(invalidator_registrar_.GetHandlerNameToTopicsMap());
   return true;

components/invalidation/impl/invalidator_registrar_with_memory.cc

@@ -8,8 +8,11 @@
 #include <iterator>
 #include <string>
 #include <utility>
+#include <vector>
 
 #include "base/logging.h"
+#include "base/optional.h"
+#include "base/stl_util.h"
 #include "base/values.h"
 #include "components/invalidation/public/topic_invalidation_map.h"
 #include "components/prefs/pref_registry_simple.h"
@@ -39,6 +42,17 @@ void MigratePrefs(PrefService* prefs, const std::string& sender_id) {
   prefs->ClearPref(kTopicsToHandlerDeprecated);
 }
 
+base::Optional<invalidation::TopicData> FindAnyDuplicatedTopic(
+    const std::set<invalidation::TopicData>& lhs,
+    const std::set<invalidation::TopicData>& rhs) {
+  auto intersection =
+      base::STLSetIntersection<std::vector<invalidation::TopicData>>(lhs, rhs);
+  if (!intersection.empty()) {
+    return intersection[0];
+  }
+  return base::nullopt;
+}
+
 }  // namespace
 
 // static
@@ -74,20 +88,20 @@ InvalidatorRegistrarWithMemory::InvalidatorRegistrarWithMemory(
   }
   // Restore |handler_name_to_subscribed_topics_map_| from prefs.
   for (const auto& it : pref_data->DictItems()) {
-    Topic topic = it.first;
+    const std::string& topic_name = it.first;
     if (it.second.is_dict()) {
       const base::Value* handler = it.second.FindDictKey(kHandler);
       const base::Value* is_public = it.second.FindDictKey(kIsPublic);
       if (!handler || !is_public) {
         continue;
       }
-      handler_name_to_subscribed_topics_map_[handler->GetString()].emplace(
-          topic, TopicMetadata{is_public->GetBool()});
+      handler_name_to_subscribed_topics_map_[handler->GetString()].insert(
+          invalidation::TopicData(topic_name, is_public->GetBool()));
     } else if (it.second.is_string()) {
       std::string handler_name;
       it.second.GetAsString(&handler_name);
-      handler_name_to_subscribed_topics_map_[handler_name].emplace(
-          topic, TopicMetadata{false});
+      handler_name_to_subscribed_topics_map_[handler_name].insert(
+          invalidation::TopicData(topic_name, false));
     }
   }
 }
@@ -120,7 +134,7 @@ void InvalidatorRegistrarWithMemory::UnregisterHandler(
 
 bool InvalidatorRegistrarWithMemory::UpdateRegisteredTopics(
     InvalidationHandler* handler,
-    const Topics& topics) {
+    const std::set<invalidation::TopicData>& topics) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   CHECK(handler);
   CHECK(handlers_.HasObserver(handler));
@@ -129,7 +143,8 @@ bool InvalidatorRegistrarWithMemory::UpdateRegisteredTopics(
     return false;
   }
 
-  Topics old_topics = GetRegisteredTopics(handler);
+  std::set<invalidation::TopicData> old_topics =
+      registered_handler_to_topics_map_[handler];
   if (topics.empty()) {
     registered_handler_to_topics_map_.erase(handler);
   } else {
@@ -142,9 +157,12 @@ bool InvalidatorRegistrarWithMemory::UpdateRegisteredTopics(
   // topics which are not registered, but it almost certainly should. It
   // requires GetOwnerName() to return unique value for each handler, which is
   // currently not the case for CloudPolicyInvalidator (see crbug.com/1049591).
-  auto to_unregister = FindRemovedTopics(old_topics, topics);
+  auto to_unregister =
+      base::STLSetDifference<std::set<invalidation::TopicData>>(old_topics,
+                                                                topics);
+  ;
   for (const auto& topic : to_unregister) {
-    pref_data->RemoveKey(topic);
+    pref_data->RemoveKey(topic.name);
     handler_name_to_subscribed_topics_map_[handler->GetOwnerName()].erase(
         topic);
   }
@@ -154,8 +172,8 @@ bool InvalidatorRegistrarWithMemory::UpdateRegisteredTopics(
         topic);
     base::DictionaryValue handler_pref;
     handler_pref.SetStringKey(kHandler, handler->GetOwnerName());
-    handler_pref.SetBoolKey(kIsPublic, topic.second.is_public);
-    pref_data->SetKey(topic.first, std::move(handler_pref));
+    handler_pref.SetBoolKey(kIsPublic, topic.is_public);
+    pref_data->SetKey(topic.name, std::move(handler_pref));
   }
   return true;
 }
@@ -164,17 +182,18 @@ Topics InvalidatorRegistrarWithMemory::GetRegisteredTopics(
     InvalidationHandler* handler) const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   auto lookup = registered_handler_to_topics_map_.find(handler);
-  return lookup != registered_handler_to_topics_map_.end() ? lookup->second
-                                                           : Topics();
+  return lookup != registered_handler_to_topics_map_.end()
+             ? invalidation::ConvertTopicSetToLegacyTopicMap(lookup->second)
+             : Topics();
 }
 
 Topics InvalidatorRegistrarWithMemory::GetAllSubscribedTopics() const {
-  Topics subscribed_topics;
+  std::set<invalidation::TopicData> subscribed_topics;
   for (const auto& handler_to_topic : handler_name_to_subscribed_topics_map_) {
     subscribed_topics.insert(handler_to_topic.second.begin(),
                              handler_to_topic.second.end());
   }
-  return subscribed_topics;
+  return invalidation::ConvertTopicSetToLegacyTopicMap(subscribed_topics);
 }
 
 void InvalidatorRegistrarWithMemory::DispatchInvalidationsToHandlers(
@@ -186,8 +205,8 @@ void InvalidatorRegistrarWithMemory::DispatchInvalidationsToHandlers(
   }
 
   for (const auto& handler_and_topics : registered_handler_to_topics_map_) {
-    TopicInvalidationMap topics_to_emit =
-        invalidation_map.GetSubsetWithTopics(handler_and_topics.second);
+    TopicInvalidationMap topics_to_emit = invalidation_map.GetSubsetWithTopics(
+        ConvertTopicSetToLegacyTopicMap(handler_and_topics.second));
     if (topics_to_emit.Empty()) {
       continue;
     }
@@ -222,7 +241,8 @@ InvalidatorRegistrarWithMemory::GetHandlerNameToTopicsMap() {
   std::map<std::string, Topics> names_to_topics;
   for (const auto& handler_and_topics : registered_handler_to_topics_map_) {
     names_to_topics[handler_and_topics.first->GetOwnerName()] =
-        handler_and_topics.second;
+        invalidation::ConvertTopicSetToLegacyTopicMap(
+            handler_and_topics.second);
   }
   return names_to_topics;
 }
@@ -235,16 +255,17 @@ void InvalidatorRegistrarWithMemory::RequestDetailedStatus(
 
 bool InvalidatorRegistrarWithMemory::HasDuplicateTopicRegistration(
     InvalidationHandler* handler,
-    const Topics& topics) const {
+    const std::set<invalidation::TopicData>& topics) const {
   for (const auto& handler_and_topics : registered_handler_to_topics_map_) {
     if (handler_and_topics.first == handler) {
       continue;
     }
 
-    if (auto* duplicate =
-            FindMatchingTopic(topics, handler_and_topics.second)) {
-      DVLOG(1) << "Duplicate registration: trying to register " << *duplicate
-               << " for " << handler << " when it's already registered for "
+    if (base::Optional<invalidation::TopicData> duplicate =
+            FindAnyDuplicatedTopic(topics, handler_and_topics.second)) {
+      DVLOG(1) << "Duplicate registration: trying to register "
+               << duplicate->name << " for " << handler
+               << " when it's already registered for "
                << handler_and_topics.first;
       return true;
     }
@@ -259,7 +280,7 @@ base::DictionaryValue InvalidatorRegistrarWithMemory::CollectDebugData() const {
   for (const auto& handler_to_topics : handler_name_to_subscribed_topics_map_) {
     const std::string& handler = handler_to_topics.first;
     for (const auto& topic : handler_to_topics.second) {
-      return_value.SetString("InvalidatorRegistrarWithMemory." + topic.first,
+      return_value.SetString("InvalidatorRegistrarWithMemory." + topic.name,
                              handler);
     }
   }

components/invalidation/impl/invalidator_registrar_with_memory.h

@@ -6,6 +6,7 @@
 #define COMPONENTS_INVALIDATION_IMPL_INVALIDATOR_REGISTRAR_WITH_MEMORY_H_
 
 #include <map>
+#include <set>
 #include <string>
 
 #include "base/macros.h"
@@ -13,7 +14,7 @@
 #include "base/sequence_checker.h"
 #include "components/invalidation/public/invalidation_export.h"
 #include "components/invalidation/public/invalidation_handler.h"
-#include "components/invalidation/public/invalidation_util.h"
+#include "components/invalidation/public/topic_data.h"
 #include "components/invalidation/public/topic_invalidation_map.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
@@ -59,7 +60,8 @@ class INVALIDATION_EXPORT InvalidatorRegistrarWithMemory {
   // Note that this also updates the *subscribed* topics - assuming that whoever
   // called this will also send (un)subscription requests to the server.
   bool UpdateRegisteredTopics(InvalidationHandler* handler,
-                              const Topics& topics) WARN_UNUSED_RESULT;
+                              const std::set<invalidation::TopicData>& topics)
+      WARN_UNUSED_RESULT;
 
   // Returns all topics currently registered to |handler|.
   Topics GetRegisteredTopics(InvalidationHandler* handler) const;
@@ -104,8 +106,9 @@ class INVALIDATION_EXPORT InvalidatorRegistrarWithMemory {
  private:
   // Checks if any of the |topics| is already registered for a *different*
   // handler than the given one.
-  bool HasDuplicateTopicRegistration(InvalidationHandler* handler,
-                                     const Topics& topics) const;
+  bool HasDuplicateTopicRegistration(
+      InvalidationHandler* handler,
+      const std::set<invalidation::TopicData>& topics) const;
 
   // Generate a Dictionary with all the debugging information.
   base::DictionaryValue CollectDebugData() const;
@@ -116,8 +119,10 @@ class INVALIDATION_EXPORT InvalidatorRegistrarWithMemory {
   // Note: When a handler is unregistered, its entry is removed from
   // |registered_handler_to_topics_map_| but NOT from
   // |handler_name_to_subscribed_topics_map_|.
-  std::map<InvalidationHandler*, Topics> registered_handler_to_topics_map_;
-  std::map<std::string, Topics> handler_name_to_subscribed_topics_map_;
+  std::map<InvalidationHandler*, std::set<invalidation::TopicData>>
+      registered_handler_to_topics_map_;
+  std::map<std::string, std::set<invalidation::TopicData>>
+      handler_name_to_subscribed_topics_map_;
 
   InvalidatorState state_;
 

components/invalidation/impl/invalidator_registrar_with_memory_unittest.cc

@@ -19,24 +19,14 @@ namespace syncer {
 
 namespace {
 
-Topics TopicSetToTopics(const TopicSet& topic_set,
-                        InvalidationHandler* handler) {
-  DCHECK(handler);
-  Topics result;
-  for (const Topic& topic : topic_set) {
-    result.emplace(topic, TopicMetadata{handler->IsPublicTopic(topic)});
-  }
-  return result;
-}
-
 // Initialize the invalidator, register a handler, register some topics for that
 // handler, and then unregister the handler, dispatching invalidations in
 // between. The handler should only see invalidations when it's registered and
 // its topics are registered.
 TEST(InvalidatorRegistrarWithMemoryTest, Basic) {
-  const Topic kTopic1 = "a";
-  const Topic kTopic2 = "b";
-  const Topic kTopic3 = "c";
+  const invalidation::TopicData kTopic1(/*name=*/"a", /*is_public=*/false);
+  const invalidation::TopicData kTopic2(/*name=*/"b", /*is_public=*/false);
+  const invalidation::TopicData kTopic3(/*name=*/"c", /*is_public=*/false);
 
   TestingPrefServiceSimple pref_service;
   InvalidatorRegistrarWithMemory::RegisterProfilePrefs(pref_service.registry());
@@ -48,35 +38,35 @@ TEST(InvalidatorRegistrarWithMemoryTest, Basic) {
   invalidator->RegisterHandler(&handler);
 
   TopicInvalidationMap invalidation_map;
-  invalidation_map.Insert(Invalidation::Init(kTopic1, 1, "1"));
-  invalidation_map.Insert(Invalidation::Init(kTopic2, 2, "2"));
-  invalidation_map.Insert(Invalidation::Init(kTopic3, 3, "3"));
+  invalidation_map.Insert(Invalidation::Init(kTopic1.name, 1, "1"));
+  invalidation_map.Insert(Invalidation::Init(kTopic2.name, 2, "2"));
+  invalidation_map.Insert(Invalidation::Init(kTopic3.name, 3, "3"));
 
   // Should be ignored since no topics are registered to |handler|.
   invalidator->DispatchInvalidationsToHandlers(invalidation_map);
   EXPECT_EQ(0, handler.GetInvalidationCount());
 
-  EXPECT_TRUE(invalidator->UpdateRegisteredTopics(
-      &handler, TopicSetToTopics({kTopic1, kTopic2}, &handler)));
+  EXPECT_TRUE(
+      invalidator->UpdateRegisteredTopics(&handler, {kTopic1, kTopic2}));
 
   invalidator->UpdateInvalidatorState(INVALIDATIONS_ENABLED);
   EXPECT_EQ(INVALIDATIONS_ENABLED, handler.GetInvalidatorState());
 
   TopicInvalidationMap expected_invalidations;
-  expected_invalidations.Insert(Invalidation::Init(kTopic1, 1, "1"));
-  expected_invalidations.Insert(Invalidation::Init(kTopic2, 2, "2"));
+  expected_invalidations.Insert(Invalidation::Init(kTopic1.name, 1, "1"));
+  expected_invalidations.Insert(Invalidation::Init(kTopic2.name, 2, "2"));
 
   invalidator->DispatchInvalidationsToHandlers(invalidation_map);
   EXPECT_EQ(1, handler.GetInvalidationCount());
   EXPECT_EQ(expected_invalidations, handler.GetLastInvalidationMap());
 
   // Remove kTopic1, add kTopic3.
-  EXPECT_TRUE(invalidator->UpdateRegisteredTopics(
-      &handler, TopicSetToTopics({kTopic2, kTopic3}, &handler)));
+  EXPECT_TRUE(
+      invalidator->UpdateRegisteredTopics(&handler, {kTopic2, kTopic3}));
 
   expected_invalidations = TopicInvalidationMap();
-  expected_invalidations.Insert(Invalidation::Init(kTopic2, 2, "2"));
-  expected_invalidations.Insert(Invalidation::Init(kTopic3, 3, "3"));
+  expected_invalidations.Insert(Invalidation::Init(kTopic2.name, 2, "2"));
+  expected_invalidations.Insert(Invalidation::Init(kTopic3.name, 3, "3"));
 
   // Removed topic should not be notified, newly-added ones should.
   invalidator->DispatchInvalidationsToHandlers(invalidation_map);
@@ -102,10 +92,10 @@ TEST(InvalidatorRegistrarWithMemoryTest, Basic) {
 // invalidations, and the ones that have registered topics should receive
 // invalidations for those topics.
 TEST(InvalidatorRegistrarWithMemoryTest, MultipleHandlers) {
-  const Topic kTopic1 = "a";
-  const Topic kTopic2 = "b";
-  const Topic kTopic3 = "c";
-  const Topic kTopic4 = "d";
+  const invalidation::TopicData kTopic1(/*name=*/"a", /*is_public=*/false);
+  const invalidation::TopicData kTopic2(/*name=*/"b", /*is_public=*/false);
+  const invalidation::TopicData kTopic3(/*name=*/"c", /*is_public=*/false);
+  const invalidation::TopicData kTopic4(/*name=*/"d", /*is_public=*/false);
 
   TestingPrefServiceSimple pref_service;
   InvalidatorRegistrarWithMemory::RegisterProfilePrefs(pref_service.registry());
@@ -123,13 +113,11 @@ TEST(InvalidatorRegistrarWithMemoryTest, MultipleHandlers) {
   invalidator->RegisterHandler(&handler3);
   invalidator->RegisterHandler(&handler4);
 
-  EXPECT_TRUE(invalidator->UpdateRegisteredTopics(
-      &handler1, TopicSetToTopics({kTopic1, kTopic2}, &handler1)));
-  EXPECT_TRUE(invalidator->UpdateRegisteredTopics(
-      &handler2, TopicSetToTopics({kTopic3}, &handler2)));
+  EXPECT_TRUE(
+      invalidator->UpdateRegisteredTopics(&handler1, {kTopic1, kTopic2}));
+  EXPECT_TRUE(invalidator->UpdateRegisteredTopics(&handler2, {kTopic3}));
   // Don't register any IDs for handler3.
-  EXPECT_TRUE(invalidator->UpdateRegisteredTopics(
-      &handler4, TopicSetToTopics({kTopic4}, &handler4)));
+  EXPECT_TRUE(invalidator->UpdateRegisteredTopics(&handler4, {kTopic4}));
 
   invalidator->UnregisterHandler(&handler4);
 
@@ -140,22 +128,22 @@ TEST(InvalidatorRegistrarWithMemoryTest, MultipleHandlers) {
   EXPECT_EQ(TRANSIENT_INVALIDATION_ERROR, handler4.GetInvalidatorState());
 
   TopicInvalidationMap invalidation_map;
-  invalidation_map.Insert(Invalidation::Init(kTopic1, 1, "1"));
-  invalidation_map.Insert(Invalidation::Init(kTopic2, 2, "2"));
-  invalidation_map.Insert(Invalidation::Init(kTopic3, 3, "3"));
-  invalidation_map.Insert(Invalidation::Init(kTopic4, 4, "4"));
+  invalidation_map.Insert(Invalidation::Init(kTopic1.name, 1, "1"));
+  invalidation_map.Insert(Invalidation::Init(kTopic2.name, 2, "2"));
+  invalidation_map.Insert(Invalidation::Init(kTopic3.name, 3, "3"));
+  invalidation_map.Insert(Invalidation::Init(kTopic4.name, 4, "4"));
 
   invalidator->DispatchInvalidationsToHandlers(invalidation_map);
 
   TopicInvalidationMap expected_invalidations1;
-  expected_invalidations1.Insert(Invalidation::Init(kTopic1, 1, "1"));
-  expected_invalidations1.Insert(Invalidation::Init(kTopic2, 2, "2"));
+  expected_invalidations1.Insert(Invalidation::Init(kTopic1.name, 1, "1"));
+  expected_invalidations1.Insert(Invalidation::Init(kTopic2.name, 2, "2"));
 
   EXPECT_EQ(1, handler1.GetInvalidationCount());
   EXPECT_EQ(expected_invalidations1, handler1.GetLastInvalidationMap());
 
   TopicInvalidationMap expected_invalidations2;
-  expected_invalidations2.Insert(Invalidation::Init(kTopic3, 3, "3"));
+  expected_invalidations2.Insert(Invalidation::Init(kTopic3.name, 3, "3"));
 
   EXPECT_EQ(1, handler2.GetInvalidationCount());
   EXPECT_EQ(expected_invalidations2, handler2.GetLastInvalidationMap());
@@ -177,7 +165,7 @@ TEST(InvalidatorRegistrarWithMemoryTest, MultipleHandlers) {
 // Multiple registrations by different handlers on the same topic should
 // return false.
 TEST(InvalidatorRegistrarWithMemoryTest, MultipleRegistrations) {
-  const Topic kTopic1 = "a";
+  const invalidation::TopicData kTopic1(/*name=*/"a", /*is_public=*/false);
 
   TestingPrefServiceSimple pref_service;
   InvalidatorRegistrarWithMemory::RegisterProfilePrefs(pref_service.registry());
@@ -193,10 +181,8 @@ TEST(InvalidatorRegistrarWithMemoryTest, MultipleRegistrations) {
 
   // Registering both handlers for the same topic. First call should succeed,
   // second should fail.
-  EXPECT_TRUE(invalidator->UpdateRegisteredTopics(
-      &handler1, TopicSetToTopics({kTopic1}, &handler1)));
-  EXPECT_FALSE(invalidator->UpdateRegisteredTopics(
-      &handler2, TopicSetToTopics({kTopic1}, &handler2)));
+  EXPECT_TRUE(invalidator->UpdateRegisteredTopics(&handler1, {kTopic1}));
+  EXPECT_FALSE(invalidator->UpdateRegisteredTopics(&handler2, {kTopic1}));
 
   // |handler1| should still own subscription to the topic and deregistration
   // of its topics should update subscriptions.
@@ -210,9 +196,9 @@ TEST(InvalidatorRegistrarWithMemoryTest, MultipleRegistrations) {
 // Make sure that passing an empty set to UpdateRegisteredTopics clears the
 // corresponding entries for the handler.
 TEST(InvalidatorRegistrarWithMemoryTest, EmptySetUnregisters) {
-  const Topic kTopic1 = "a";
-  const Topic kTopic2 = "b";
-  const Topic kTopic3 = "c";
+  const invalidation::TopicData kTopic1(/*name=*/"a", /*is_public=*/false);
+  const invalidation::TopicData kTopic2(/*name=*/"b", /*is_public=*/false);
+  const invalidation::TopicData kTopic3(/*name=*/"c", /*is_public=*/false);
 
   TestingPrefServiceSimple pref_service;
   InvalidatorRegistrarWithMemory::RegisterProfilePrefs(pref_service.registry());
@@ -228,10 +214,9 @@ TEST(InvalidatorRegistrarWithMemoryTest, EmptySetUnregisters) {
   invalidator->RegisterHandler(&handler1);
   invalidator->RegisterHandler(&handler2);
 
-  EXPECT_TRUE(invalidator->UpdateRegisteredTopics(
-      &handler1, TopicSetToTopics({kTopic1, kTopic2}, &handler1)));
-  EXPECT_TRUE(invalidator->UpdateRegisteredTopics(
-      &handler2, TopicSetToTopics({kTopic3}, &handler2)));
+  EXPECT_TRUE(
+      invalidator->UpdateRegisteredTopics(&handler1, {kTopic1, kTopic2}));
+  EXPECT_TRUE(invalidator->UpdateRegisteredTopics(&handler2, {kTopic3}));
 
   // Unregister the topics for the first observer. It should not receive any
   // further invalidations.
@@ -243,9 +228,9 @@ TEST(InvalidatorRegistrarWithMemoryTest, EmptySetUnregisters) {
 
   {
     TopicInvalidationMap invalidation_map;
-    invalidation_map.Insert(Invalidation::Init(kTopic1, 1, "1"));
-    invalidation_map.Insert(Invalidation::Init(kTopic2, 2, "2"));
-    invalidation_map.Insert(Invalidation::Init(kTopic3, 3, "3"));
+    invalidation_map.Insert(Invalidation::Init(kTopic1.name, 1, "1"));
+    invalidation_map.Insert(Invalidation::Init(kTopic2.name, 2, "2"));
+    invalidation_map.Insert(Invalidation::Init(kTopic3.name, 3, "3"));
     invalidator->DispatchInvalidationsToHandlers(invalidation_map);
     EXPECT_EQ(0, handler1.GetInvalidationCount());
     EXPECT_EQ(1, handler2.GetInvalidationCount());

components/invalidation/public/BUILD.gn

@@ -22,6 +22,8 @@ static_library("public") {
     "invalidator_state.h",
     "single_object_invalidation_set.cc",
     "single_object_invalidation_set.h",
+    "topic_data.cc",
+    "topic_data.h",
     "topic_invalidation_map.cc",
     "topic_invalidation_map.h",
   ]

components/invalidation/public/invalidation_util.cc

@@ -61,34 +61,4 @@ HandlerOwnerType OwnerNameToHandlerType(const std::string& owner_name) {
   return HandlerOwnerType::kUnknown;
 }
 
-const Topic* FindMatchingTopic(const Topics& lhs, const Topics& rhs) {
-  for (auto lhs_it = lhs.begin(), rhs_it = rhs.begin();
-       lhs_it != lhs.end() && rhs_it != rhs.end();) {
-    if (lhs_it->first == rhs_it->first) {
-      return &lhs_it->first;
-    } else if (lhs_it->first < rhs_it->first) {
-      ++lhs_it;
-    } else {
-      ++rhs_it;
-    }
-  }
-  return nullptr;
-}
-
-std::vector<Topic> FindRemovedTopics(const Topics& lhs, const Topics& rhs) {
-  std::vector<Topic> result;
-  for (auto lhs_it = lhs.begin(), rhs_it = rhs.begin(); lhs_it != lhs.end();) {
-    if (rhs_it == rhs.end() || lhs_it->first < rhs_it->first) {
-      result.push_back(lhs_it->first);
-      ++lhs_it;
-    } else if (lhs_it->first == rhs_it->first) {
-      ++lhs_it;
-      ++rhs_it;
-    } else {
-      ++rhs_it;
-    }
-  }
-  return result;
-}
-
 }  // namespace syncer

components/invalidation/public/invalidation_util.h

@@ -58,13 +58,6 @@ using Topics = std::map<std::string, TopicMetadata>;
 
 HandlerOwnerType OwnerNameToHandlerType(const std::string& owner_name);
 
-// Returns a |Topic| contained within both |lhs| and |rhs| or null if |lhs| and
-// |rhs| are disjoint.
-const Topic* FindMatchingTopic(const Topics& lhs, const Topics& rhs);
-
-// Returns a vector of Topics in |lhs| but not |rhs|.
-std::vector<Topic> FindRemovedTopics(const Topics& lhs, const Topics& rhs);
-
 }  // namespace syncer
 
 #endif  // COMPONENTS_INVALIDATION_PUBLIC_INVALIDATION_UTIL_H_

components/invalidation/public/topic_data.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_data.h"
+
+namespace invalidation {
+
+TopicData::TopicData(const std::string& name, bool is_public)
+    : name(name), is_public(is_public) {}
+
+TopicData::TopicData(const TopicData& other) = default;
+
+TopicData::TopicData(TopicData&& other) = default;
+
+TopicData& TopicData::operator=(const TopicData& other) = default;
+TopicData& TopicData::operator=(TopicData&& other) = default;
+
+bool operator==(const TopicData& lhs, const TopicData& rhs) {
+  return lhs.name == rhs.name && lhs.is_public == rhs.is_public;
+}
+
+bool operator!=(const TopicData& lhs, const TopicData& rhs) {
+  return !(lhs == rhs);
+}
+
+bool operator<(const TopicData& lhs, const TopicData& rhs) {
+  if (lhs.name != rhs.name) {
+    return lhs.name < rhs.name;
+  }
+  return lhs.is_public < rhs.is_public;
+}
+
+syncer::Topics ConvertTopicSetToLegacyTopicMap(
+    const std::set<TopicData>& topics) {
+  syncer::Topics result;
+  for (const TopicData& topic : topics) {
+    result.emplace(topic.name,
+                   syncer::TopicMetadata{/*is_public=*/topic.is_public});
+  }
+  return result;
+}
+
+}  // namespace invalidation
\ No newline at end of file

components/invalidation/public/topic_data.h

+// 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.
+
+#ifndef COMPONENTS_INVALIDATION_PUBLIC_TOPIC_DATA_H_
+#define COMPONENTS_INVALIDATION_PUBLIC_TOPIC_DATA_H_
+
+#include <string>
+
+#include "components/invalidation/public/invalidation_export.h"
+#include "components/invalidation/public/invalidation_util.h"
+
+namespace invalidation {
+
+// Represents invalidation topic concept: A "namespace" or "channel" of
+// messages that clients can subscribe to. For Sync, they correspond to data
+// types.
+// TODO(crbug.com/1029698): rename to Topic.
+class INVALIDATION_EXPORT TopicData {
+ public:
+  TopicData(const std::string& name, bool is_public);
+  TopicData(const TopicData& other);
+  TopicData(TopicData&& other);
+
+  TopicData& operator=(const TopicData& other);
+  TopicData& operator=(TopicData&& other);
+
+  // A public name of the topic (i.e. it's always not GAIA-keyed).
+  std::string name;
+
+  // A topic can be either private (i.e. GAIA-keyed) or public. For private
+  // topics, a unique ID derived from the user's GAIA ID is appended to the
+  // topic name to make it unique (though this is an implementation detail
+  // which is hidden from clients).
+  bool is_public;
+};
+
+INVALIDATION_EXPORT bool operator==(const TopicData& lhs, const TopicData& rhs);
+INVALIDATION_EXPORT bool operator!=(const TopicData& lhs, const TopicData& rhs);
+INVALIDATION_EXPORT bool operator<(const TopicData& lhs, const TopicData& rhs);
+
+INVALIDATION_EXPORT std::string ToString(const TopicData& topic_data);
+
+// TDOO(crbug.com/1029698): delete this function together with legacy topic
+// datatypes.
+syncer::Topics ConvertTopicSetToLegacyTopicMap(
+    const std::set<TopicData>& topics);
+
+}  // namespace invalidation
+
+#endif  // COMPONENTS_INVALIDATION_PUBLIC_TOPIC_DATA_H_
\ No newline at end of file