Skip to content

Commit

Permalink
[Invalidations] Remove remaining ObjectID usages
Browse files Browse the repository at this point in the history
This CL removes remaining ObjectID usages (mostly in
components/invalidation) from C++ codebase. Cleaning up
the dependencies on cacheinvalidation outside of
components/invalidation will be done in follow up patches.

Tbr: fukino@chromium.org
Bug: 1029698
Change-Id: Ib6b2aa064be72f35c9ddef191a7471e194090588
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2087338
Commit-Queue: Maksim Moskvitin <mmoskvitin@google.com>
Auto-Submit: Maksim Moskvitin <mmoskvitin@google.com>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#748198}
  • Loading branch information
Maksim Moskvitin authored and Commit Bot committed Mar 9, 2020
1 parent 81fd883 commit 72b79d5
Show file tree
Hide file tree
Showing 25 changed files with 207 additions and 325 deletions.
1 change: 0 additions & 1 deletion components/drive/drive_notification_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,6 @@ void DriveNotificationManager::UpdateRegisteredDriveNotifications() {
return;

syncer::TopicSet topics;
syncer::ObjectIdSet ids;
topics.insert(GetDriveInvalidationTopic());

for (const auto& team_drive_id : team_drive_ids_) {
Expand Down
1 change: 0 additions & 1 deletion components/invalidation/DEPS
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ include_rules = [
"-content",
"-components/invalidation",
"+components/prefs",
"+google/cacheinvalidation",
"+google_apis/gaia",
"+mojo/public",
]
5 changes: 1 addition & 4 deletions components/invalidation/impl/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -137,10 +137,7 @@ static_library("test_support") {
"topic_invalidation_map_test_util.h",
]

public_deps = [
":impl",
"//third_party/cacheinvalidation",
]
public_deps = [ ":impl" ]
deps = [
"//base",
"//components/gcm_driver:test_support",
Expand Down
3 changes: 1 addition & 2 deletions components/invalidation/impl/fake_invalidation_service.cc
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,7 @@ void FakeInvalidationService::EmitInvalidationForTest(
// If no one is listening to this invalidation, do not send it out.
syncer::Topics subscribed_topics =
invalidator_registrar_->GetAllSubscribedTopics();
if (subscribed_topics.find(invalidation.object_id().name()) ==
subscribed_topics.end()) {
if (subscribed_topics.find(invalidation.topic()) == subscribed_topics.end()) {
mock_ack_handler_.RegisterUnsentInvalidation(&invalidation_copy);
return;
}
Expand Down
5 changes: 2 additions & 3 deletions components/invalidation/impl/fcm_invalidation_listener.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
#include "components/invalidation/public/invalidation_util.h"
#include "components/invalidation/public/topic_invalidation_map.h"
#include "components/prefs/pref_service.h"
#include "google/cacheinvalidation/include/types.h"

namespace syncer {

Expand Down Expand Up @@ -79,8 +78,8 @@ void FCMInvalidationListener::InvalidationReceived(
return;
}
TopicInvalidationMap invalidations;
Invalidation inv = Invalidation::Init(
ConvertTopicToId(*expected_public_topic), version, payload);
Invalidation inv =
Invalidation::Init(*expected_public_topic, version, payload);
inv.SetAckHandler(weak_factory_.GetWeakPtr(),
base::ThreadTaskRunnerHandle::Get());
DVLOG(1) << "Received invalidation with version " << inv.version() << " for "
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -337,9 +337,8 @@ TEST_F(FCMInvalidationListenerTest, ManyInvalidations_NoDrop) {
EXPECT_EQ(initial_version + kRepeatCount - 1, GetVersion(topic));
}

// Fire an invalidation for an unregistered object topic with a payload. It
// should still be processed, and both the payload and the version should be
// updated.
// Fire an invalidation for an unregistered topic with a payload. It should
// still be processed, and both the payload and the version should be updated.
TEST_F(FCMInvalidationListenerTest, InvalidateBeforeRegistration_Simple) {
const Topic kUnregisteredId = "unregistered";
const Topic& topic = kUnregisteredId;
Expand All @@ -361,7 +360,7 @@ TEST_F(FCMInvalidationListenerTest, InvalidateBeforeRegistration_Simple) {
EXPECT_EQ(kPayload1, GetPayload(topic));
}

// Fire ten invalidations before an object registers. Some invalidations will
// Fire ten invalidations before an topics registers. Some invalidations will
// be dropped an replaced with an unknown version invalidation.
TEST_F(FCMInvalidationListenerTest, InvalidateBeforeRegistration_Drop) {
const int kRepeatCount =
Expand Down
4 changes: 2 additions & 2 deletions components/invalidation/impl/invalidation_logger.h
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,8 @@ class InvalidationLogger {
syncer::InvalidatorState last_invalidator_state_;
base::Time last_invalidator_state_timestamp_;

// The map that contains every object id that is currently registered
// and its owner.
// The map that contains every topic that is currently registered and its
// owner.
std::map<std::string, syncer::Topics> handler_latest_topics_map_;

// The map that counts how many invalidations per Topic there has been.
Expand Down
6 changes: 3 additions & 3 deletions components/invalidation/impl/invalidation_logger_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -154,10 +154,10 @@ TEST(InvalidationLoggerTest, TestEmitContent) {
log.UnregisterObserver(&observer_test);
}

// Test that the updateId notification actually sends the same ObjectId that
// was sent to the Observer.
// Test that the OnUpdatedTopics() notification actually sends the same Topic
// that was sent to the Observer.
// The ObserverTest rebuilds the map that was sent in pieces by the logger.
TEST(InvalidationLoggerTest, TestUpdateIdsMap) {
TEST(InvalidationLoggerTest, TestUpdatedTopicsMap) {
InvalidationLogger log;
InvalidationLoggerObserverTest observer_test;
std::map<std::string, syncer::Topics> send_test_map;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ TYPED_TEST_P(InvalidationServiceTest, Basic) {
expected_invalidations.Insert(
syncer::Invalidation::Init(this->topic3, 3, "3"));

// Removed object IDs should not be notified, newly-added ones should.
// Removed Topics should not be notified, newly-added ones should.
this->delegate_.TriggerOnIncomingInvalidation(invalidation_map);
EXPECT_EQ(2, handler.GetInvalidationCount());
EXPECT_THAT(expected_invalidations, Eq(handler.GetLastInvalidationMap()));
Expand Down Expand Up @@ -269,8 +269,8 @@ TYPED_TEST_P(InvalidationServiceTest, MultipleHandlers) {
invalidator->UnregisterInvalidationHandler(&handler1);
}

// Multiple registrations by different handlers on the same object ID should
// return false.
// Multiple registrations by different handlers on the same Topic should return
// false.
TYPED_TEST_P(InvalidationServiceTest, MultipleRegistrations) {
invalidation::InvalidationService* const invalidator =
this->CreateAndInitializeInvalidationService();
Expand Down
2 changes: 1 addition & 1 deletion components/invalidation/impl/invalidation_test_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ InvalidationEqMatcher::InvalidationEqMatcher(const Invalidation& expected)
bool InvalidationEqMatcher::MatchAndExplain(
const Invalidation& actual,
MatchResultListener* listener) const {
if (!(expected_.object_id() == actual.object_id()))
if (expected_.topic() != actual.topic())
return false;
if (expected_.is_unknown_version() && actual.is_unknown_version())
return true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,23 +13,30 @@
#include "components/invalidation/public/invalidator_state.h"
#include "components/invalidation/public/topic_invalidation_map.h"
#include "components/prefs/testing_pref_service.h"
#include "google/cacheinvalidation/include/types.h"
#include "google/cacheinvalidation/types.pb.h"
#include "testing/gtest/include/gtest/gtest.h"

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 invalidation::ObjectId id1(ipc::invalidation::ObjectSource::TEST, "a");
const invalidation::ObjectId id2(ipc::invalidation::ObjectSource::TEST, "b");
const invalidation::ObjectId id3(ipc::invalidation::ObjectSource::TEST, "c");
const invalidation::ObjectId id4(ipc::invalidation::ObjectSource::TEST, "d");
const Topic kTopic1 = "a";
const Topic kTopic2 = "b";
const Topic kTopic3 = "c";

TestingPrefServiceSimple pref_service;
InvalidatorRegistrarWithMemory::RegisterProfilePrefs(pref_service.registry());
Expand All @@ -41,35 +48,35 @@ TEST(InvalidatorRegistrarWithMemoryTest, Basic) {
invalidator->RegisterHandler(&handler);

TopicInvalidationMap invalidation_map;
invalidation_map.Insert(Invalidation::Init(id1, 1, "1"));
invalidation_map.Insert(Invalidation::Init(id2, 2, "2"));
invalidation_map.Insert(Invalidation::Init(id3, 3, "3"));
invalidation_map.Insert(Invalidation::Init(kTopic1, 1, "1"));
invalidation_map.Insert(Invalidation::Init(kTopic2, 2, "2"));
invalidation_map.Insert(Invalidation::Init(kTopic3, 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, ConvertIdsToTopics({id1, id2}, &handler)));
&handler, TopicSetToTopics({kTopic1, kTopic2}, &handler)));

invalidator->UpdateInvalidatorState(INVALIDATIONS_ENABLED);
EXPECT_EQ(INVALIDATIONS_ENABLED, handler.GetInvalidatorState());

TopicInvalidationMap expected_invalidations;
expected_invalidations.Insert(Invalidation::Init(id1, 1, "1"));
expected_invalidations.Insert(Invalidation::Init(id2, 2, "2"));
expected_invalidations.Insert(Invalidation::Init(kTopic1, 1, "1"));
expected_invalidations.Insert(Invalidation::Init(kTopic2, 2, "2"));

invalidator->DispatchInvalidationsToHandlers(invalidation_map);
EXPECT_EQ(1, handler.GetInvalidationCount());
EXPECT_EQ(expected_invalidations, handler.GetLastInvalidationMap());

// Remove id1, add id3.
// Remove kTopic1, add kTopic3.
EXPECT_TRUE(invalidator->UpdateRegisteredTopics(
&handler, ConvertIdsToTopics({id2, id3}, &handler)));
&handler, TopicSetToTopics({kTopic2, kTopic3}, &handler)));

expected_invalidations = TopicInvalidationMap();
expected_invalidations.Insert(Invalidation::Init(id2, 2, "2"));
expected_invalidations.Insert(Invalidation::Init(id3, 3, "3"));
expected_invalidations.Insert(Invalidation::Init(kTopic2, 2, "2"));
expected_invalidations.Insert(Invalidation::Init(kTopic3, 3, "3"));

// Removed topic should not be notified, newly-added ones should.
invalidator->DispatchInvalidationsToHandlers(invalidation_map);
Expand All @@ -95,10 +102,10 @@ TEST(InvalidatorRegistrarWithMemoryTest, Basic) {
// invalidations, and the ones that have registered topics should receive
// invalidations for those topics.
TEST(InvalidatorRegistrarWithMemoryTest, MultipleHandlers) {
const invalidation::ObjectId id1(ipc::invalidation::ObjectSource::TEST, "a");
const invalidation::ObjectId id2(ipc::invalidation::ObjectSource::TEST, "b");
const invalidation::ObjectId id3(ipc::invalidation::ObjectSource::TEST, "c");
const invalidation::ObjectId id4(ipc::invalidation::ObjectSource::TEST, "d");
const Topic kTopic1 = "a";
const Topic kTopic2 = "b";
const Topic kTopic3 = "c";
const Topic kTopic4 = "d";

TestingPrefServiceSimple pref_service;
InvalidatorRegistrarWithMemory::RegisterProfilePrefs(pref_service.registry());
Expand All @@ -117,12 +124,12 @@ TEST(InvalidatorRegistrarWithMemoryTest, MultipleHandlers) {
invalidator->RegisterHandler(&handler4);

EXPECT_TRUE(invalidator->UpdateRegisteredTopics(
&handler1, ConvertIdsToTopics({id1, id2}, &handler1)));
&handler1, TopicSetToTopics({kTopic1, kTopic2}, &handler1)));
EXPECT_TRUE(invalidator->UpdateRegisteredTopics(
&handler2, ConvertIdsToTopics({id3}, &handler2)));
&handler2, TopicSetToTopics({kTopic3}, &handler2)));
// Don't register any IDs for handler3.
EXPECT_TRUE(invalidator->UpdateRegisteredTopics(
&handler4, ConvertIdsToTopics({id4}, &handler4)));
&handler4, TopicSetToTopics({kTopic4}, &handler4)));

invalidator->UnregisterHandler(&handler4);

Expand All @@ -133,22 +140,22 @@ TEST(InvalidatorRegistrarWithMemoryTest, MultipleHandlers) {
EXPECT_EQ(TRANSIENT_INVALIDATION_ERROR, handler4.GetInvalidatorState());

TopicInvalidationMap invalidation_map;
invalidation_map.Insert(Invalidation::Init(id1, 1, "1"));
invalidation_map.Insert(Invalidation::Init(id2, 2, "2"));
invalidation_map.Insert(Invalidation::Init(id3, 3, "3"));
invalidation_map.Insert(Invalidation::Init(id4, 4, "4"));
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"));

invalidator->DispatchInvalidationsToHandlers(invalidation_map);

TopicInvalidationMap expected_invalidations1;
expected_invalidations1.Insert(Invalidation::Init(id1, 1, "1"));
expected_invalidations1.Insert(Invalidation::Init(id2, 2, "2"));
expected_invalidations1.Insert(Invalidation::Init(kTopic1, 1, "1"));
expected_invalidations1.Insert(Invalidation::Init(kTopic2, 2, "2"));

EXPECT_EQ(1, handler1.GetInvalidationCount());
EXPECT_EQ(expected_invalidations1, handler1.GetLastInvalidationMap());

TopicInvalidationMap expected_invalidations2;
expected_invalidations2.Insert(Invalidation::Init(id3, 3, "3"));
expected_invalidations2.Insert(Invalidation::Init(kTopic3, 3, "3"));

EXPECT_EQ(1, handler2.GetInvalidationCount());
EXPECT_EQ(expected_invalidations2, handler2.GetLastInvalidationMap());
Expand All @@ -170,7 +177,7 @@ TEST(InvalidatorRegistrarWithMemoryTest, MultipleHandlers) {
// Multiple registrations by different handlers on the same topic should
// return false.
TEST(InvalidatorRegistrarWithMemoryTest, MultipleRegistrations) {
const invalidation::ObjectId id1(ipc::invalidation::ObjectSource::TEST, "a");
const Topic kTopic1 = "a";

TestingPrefServiceSimple pref_service;
InvalidatorRegistrarWithMemory::RegisterProfilePrefs(pref_service.registry());
Expand All @@ -187,9 +194,9 @@ TEST(InvalidatorRegistrarWithMemoryTest, MultipleRegistrations) {
// Registering both handlers for the same topic. First call should succeed,
// second should fail.
EXPECT_TRUE(invalidator->UpdateRegisteredTopics(
&handler1, ConvertIdsToTopics({id1}, &handler1)));
&handler1, TopicSetToTopics({kTopic1}, &handler1)));
EXPECT_FALSE(invalidator->UpdateRegisteredTopics(
&handler2, ConvertIdsToTopics({id1}, &handler2)));
&handler2, TopicSetToTopics({kTopic1}, &handler2)));

// |handler1| should still own subscription to the topic and deregistration
// of its topics should update subscriptions.
Expand All @@ -203,9 +210,9 @@ TEST(InvalidatorRegistrarWithMemoryTest, MultipleRegistrations) {
// Make sure that passing an empty set to UpdateRegisteredTopics clears the
// corresponding entries for the handler.
TEST(InvalidatorRegistrarWithMemoryTest, EmptySetUnregisters) {
const invalidation::ObjectId id1(ipc::invalidation::ObjectSource::TEST, "a");
const invalidation::ObjectId id2(ipc::invalidation::ObjectSource::TEST, "b");
const invalidation::ObjectId id3(ipc::invalidation::ObjectSource::TEST, "c");
const Topic kTopic1 = "a";
const Topic kTopic2 = "b";
const Topic kTopic3 = "c";

TestingPrefServiceSimple pref_service;
InvalidatorRegistrarWithMemory::RegisterProfilePrefs(pref_service.registry());
Expand All @@ -222,9 +229,9 @@ TEST(InvalidatorRegistrarWithMemoryTest, EmptySetUnregisters) {
invalidator->RegisterHandler(&handler2);

EXPECT_TRUE(invalidator->UpdateRegisteredTopics(
&handler1, ConvertIdsToTopics({id1, id2}, &handler1)));
&handler1, TopicSetToTopics({kTopic1, kTopic2}, &handler1)));
EXPECT_TRUE(invalidator->UpdateRegisteredTopics(
&handler2, ConvertIdsToTopics({id3}, &handler2)));
&handler2, TopicSetToTopics({kTopic3}, &handler2)));

// Unregister the topics for the first observer. It should not receive any
// further invalidations.
Expand All @@ -236,9 +243,9 @@ TEST(InvalidatorRegistrarWithMemoryTest, EmptySetUnregisters) {

{
TopicInvalidationMap invalidation_map;
invalidation_map.Insert(Invalidation::Init(id1, 1, "1"));
invalidation_map.Insert(Invalidation::Init(id2, 2, "2"));
invalidation_map.Insert(Invalidation::Init(id3, 3, "3"));
invalidation_map.Insert(Invalidation::Init(kTopic1, 1, "1"));
invalidation_map.Insert(Invalidation::Init(kTopic2, 2, "2"));
invalidation_map.Insert(Invalidation::Init(kTopic3, 3, "3"));
invalidator->DispatchInvalidationsToHandlers(invalidation_map);
EXPECT_EQ(0, handler1.GetInvalidationCount());
EXPECT_EQ(1, handler2.GetInvalidationCount());
Expand Down
Loading

0 comments on commit 72b79d5

Please sign in to comment.