[Sync] Avoid unregistering object IDs on shutdown

sync/notifier/sync_notifier_helper_unittest.cc

Index: sync/notifier/sync_notifier_helper_unittest.cc
diff --git a/sync/notifier/sync_notifier_helper_unittest.cc b/sync/notifier/sync_notifier_helper_unittest.cc
index 24d88716cb89d412a208fd8d39e82b89201158fc..c919d777ddef0774000bf75d7a578d81b4602238 100644
--- a/sync/notifier/sync_notifier_helper_unittest.cc
+++ b/sync/notifier/sync_notifier_helper_unittest.cc
@@ -3,13 +3,19 @@
 // found in the LICENSE file.
 
 #include "google/cacheinvalidation/types.pb.h"
-#include "sync/notifier/sync_notifier_helper.h"
 #include "sync/notifier/mock_sync_notifier_observer.h"
+#include "sync/notifier/sync_notifier_helper.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+namespace syncer {
+
+namespace {
+
+using testing::InSequence;
 using testing::StrictMock;
 
-namespace syncer {
+const char kHandlerName1[] = "handler_name1";
+const char kHandlerName2[] = "handler_name2";
 
 class SyncNotifierHelperTest : public testing::Test {
  protected:
@@ -26,12 +32,30 @@ class SyncNotifierHelperTest : public testing::Test {
 
 // Basic check that registrations are correctly updated for one handler.
 TEST_F(SyncNotifierHelperTest, Basic) {
+  InSequence dummy;
+
   SyncNotifierHelper helper;
+
   StrictMock<MockSyncNotifierObserver> observer;
+
+  ObjectIdPayloadMap expected_payload1;
+  expected_payload1[kObjectId1] = "1";
+  expected_payload1[kObjectId2] = "2";
+  EXPECT_CALL(observer, OnIncomingNotification(expected_payload1,
+                                               REMOTE_NOTIFICATION));
+
+  ObjectIdPayloadMap expected_payload2;
+  expected_payload2[kObjectId2] = "2";
+  expected_payload2[kObjectId3] = "3";
+  EXPECT_CALL(observer, OnIncomingNotification(expected_payload2,
+                                               REMOTE_NOTIFICATION));
+
+  helper.SetHandler(kHandlerName1, &observer);
+
   ObjectIdSet ids;
   ids.insert(kObjectId1);
   ids.insert(kObjectId2);
-  helper.UpdateRegisteredIds(&observer, ids);
+  helper.UpdateRegisteredIds(kHandlerName1, ids);
 
   ObjectIdPayloadMap dispatched_payloads;
   dispatched_payloads[kObjectId1] = "1";
@@ -39,51 +63,75 @@ TEST_F(SyncNotifierHelperTest, Basic) {
   dispatched_payloads[kObjectId3] = "3";
 
   // A object ID with no registration should be ignored.
-  ObjectIdPayloadMap expected_payload1;
-  expected_payload1[kObjectId1] = "1";
-  expected_payload1[kObjectId2] = "2";
-  EXPECT_CALL(observer, OnIncomingNotification(expected_payload1,
-                                               REMOTE_NOTIFICATION));
   helper.DispatchInvalidationsToHandlers(dispatched_payloads,
                                          REMOTE_NOTIFICATION);
 
   // Removed object IDs should not be notified, newly-added ones should.
   ids.erase(kObjectId1);
   ids.insert(kObjectId3);
-  helper.UpdateRegisteredIds(&observer, ids);
+  helper.UpdateRegisteredIds(kHandlerName1, ids);
 
-  ObjectIdPayloadMap expected_payload2;
-  expected_payload2[kObjectId2] = "2";
-  expected_payload2[kObjectId3] = "3";
-  EXPECT_CALL(observer, OnIncomingNotification(expected_payload2,
-                                               REMOTE_NOTIFICATION));
   helper.DispatchInvalidationsToHandlers(dispatched_payloads,
                                          REMOTE_NOTIFICATION);
 }
 
-// Tests that we correctly bucket and dispatch invalidations on multiple objects
-// to the corresponding handlers.
-TEST_F(SyncNotifierHelperTest, MultipleHandlers) {
+// Register some IDs and fire an invalidation.  Since there are no set
+// handlers when the invalidation is fired, nothing should happen.
+TEST_F(SyncNotifierHelperTest, NullHandler) {
   SyncNotifierHelper helper;
+
   StrictMock<MockSyncNotifierObserver> observer;
+  helper.SetHandler(kHandlerName1, &observer);
+
   ObjectIdSet ids;
   ids.insert(kObjectId1);
   ids.insert(kObjectId2);
-  helper.UpdateRegisteredIds(&observer, ids);
-  StrictMock<MockSyncNotifierObserver> observer2;
-  ObjectIdSet ids2;
-  ids2.insert(kObjectId3);
-  helper.UpdateRegisteredIds(&observer2, ids2);
+  helper.UpdateRegisteredIds(kHandlerName1, ids);
+
+  helper.SetHandler(kHandlerName1, NULL);
+
+  ObjectIdPayloadMap dispatched_payloads;
+  dispatched_payloads[kObjectId1] = "1";
+  dispatched_payloads[kObjectId2] = "2";
+  helper.DispatchInvalidationsToHandlers(dispatched_payloads,
+                                         REMOTE_NOTIFICATION);
+}
+
+// Tests that we correctly bucket and dispatch invalidations on
+// multiple objects to the corresponding handlers.
+TEST_F(SyncNotifierHelperTest, MultipleHandlers) {
+  SyncNotifierHelper helper;
 
+  StrictMock<MockSyncNotifierObserver> observer1;
   ObjectIdPayloadMap expected_payload1;
   expected_payload1[kObjectId1] = "1";
   expected_payload1[kObjectId2] = "2";
-  EXPECT_CALL(observer, OnIncomingNotification(expected_payload1,
-                                               REMOTE_NOTIFICATION));
+  EXPECT_CALL(observer1, OnIncomingNotification(expected_payload1,
+                                                REMOTE_NOTIFICATION));
+  EXPECT_CALL(observer1, OnNotificationsEnabled());
+  EXPECT_CALL(observer1,
+              OnNotificationsDisabled(TRANSIENT_NOTIFICATION_ERROR));
+
+  StrictMock<MockSyncNotifierObserver> observer2;
   ObjectIdPayloadMap expected_payload2;
   expected_payload2[kObjectId3] = "3";
   EXPECT_CALL(observer2, OnIncomingNotification(expected_payload2,
                                                 REMOTE_NOTIFICATION));
+  EXPECT_CALL(observer2, OnNotificationsEnabled());
+  EXPECT_CALL(observer2,
+              OnNotificationsDisabled(TRANSIENT_NOTIFICATION_ERROR));
+
+  helper.SetHandler(kHandlerName1, &observer1);
+  helper.SetHandler(kHandlerName2, &observer2);
+
+  ObjectIdSet ids;
+  ids.insert(kObjectId1);
+  ids.insert(kObjectId2);
+  helper.UpdateRegisteredIds(kHandlerName1, ids);
+
+  ObjectIdSet ids2;
+  ids2.insert(kObjectId3);
+  helper.UpdateRegisteredIds(kHandlerName2, ids2);
 
   ObjectIdPayloadMap dispatched_payloads;
   dispatched_payloads[kObjectId1] = "1";
@@ -92,13 +140,6 @@ TEST_F(SyncNotifierHelperTest, MultipleHandlers) {
   helper.DispatchInvalidationsToHandlers(dispatched_payloads,
                                          REMOTE_NOTIFICATION);
 
-  // Also verify that the callbacks for OnNotificationsEnabled/Disabled work.
-  EXPECT_CALL(observer, OnNotificationsEnabled());
-  EXPECT_CALL(observer2, OnNotificationsEnabled());
-  EXPECT_CALL(observer,
-              OnNotificationsDisabled(TRANSIENT_NOTIFICATION_ERROR));
-  EXPECT_CALL(observer2,
-              OnNotificationsDisabled(TRANSIENT_NOTIFICATION_ERROR));
   helper.EmitOnNotificationsEnabled();
   helper.EmitOnNotificationsDisabled(TRANSIENT_NOTIFICATION_ERROR);
 }
@@ -107,34 +148,51 @@ TEST_F(SyncNotifierHelperTest, MultipleHandlers) {
 // cause a CHECK.
 TEST_F(SyncNotifierHelperTest, MultipleRegistration) {
   SyncNotifierHelper helper;
+
   StrictMock<MockSyncNotifierObserver> observer;
+  helper.SetHandler(kHandlerName1, &observer);
+
   ObjectIdSet ids;
   ids.insert(kObjectId1);
   ids.insert(kObjectId2);
-  helper.UpdateRegisteredIds(&observer, ids);
+  helper.UpdateRegisteredIds(kHandlerName1, ids);
 
-  StrictMock<MockSyncNotifierObserver> observer2;
-  EXPECT_DEATH({ helper.UpdateRegisteredIds(&observer2, ids); },
-               "Duplicate registration for .*");
+  helper.DetachFromThreadForTest();
+  EXPECT_DEATH({ helper.UpdateRegisteredIds(kHandlerName2, ids); },
+               "Duplicate registration: .*");
+}
+
+// Setting the same handler for different names should cause a CHECK.
+TEST_F(SyncNotifierHelperTest, SameHandlerDifferentName) {
+  SyncNotifierHelper helper;
+
+  StrictMock<MockSyncNotifierObserver> observer;
+  helper.SetHandler(kHandlerName1, &observer);
+
+  helper.DetachFromThreadForTest();
+  EXPECT_DEATH({ helper.SetHandler(kHandlerName2, &observer); },
+               "Observers can only be added once!");
 }
 
 // Make sure that passing an empty set to UpdateRegisteredIds clears the
 // corresponding entries for the handler.
 TEST_F(SyncNotifierHelperTest, EmptySetUnregisters) {
   SyncNotifierHelper helper;
-  StrictMock<MockSyncNotifierObserver> observer;
+
+  StrictMock<MockSyncNotifierObserver> observer1;
+  EXPECT_CALL(observer1, OnNotificationsEnabled());
+  EXPECT_CALL(observer1,
+              OnNotificationsDisabled(TRANSIENT_NOTIFICATION_ERROR));
+
+  helper.SetHandler(kHandlerName1, &observer1);
+
   ObjectIdSet ids;
   ids.insert(kObjectId1);
   ids.insert(kObjectId2);
-  helper.UpdateRegisteredIds(&observer, ids);
+  helper.UpdateRegisteredIds(kHandlerName1, ids);
+
   // Control observer.
   StrictMock<MockSyncNotifierObserver> observer2;
-  ObjectIdSet ids2;
-  ids2.insert(kObjectId3);
-  helper.UpdateRegisteredIds(&observer2, ids2);
-  // Unregister the first observer. It should not receive any further callbacks.
-  helper.UpdateRegisteredIds(&observer, ObjectIdSet());
-
   ObjectIdPayloadMap expected_payload2;
   expected_payload2[kObjectId3] = "3";
   EXPECT_CALL(observer2, OnIncomingNotification(expected_payload2,
@@ -143,6 +201,16 @@ TEST_F(SyncNotifierHelperTest, EmptySetUnregisters) {
   EXPECT_CALL(observer2,
               OnNotificationsDisabled(TRANSIENT_NOTIFICATION_ERROR));
 
+  helper.SetHandler(kHandlerName2, &observer2);
+
+  ObjectIdSet ids2;
+  ids2.insert(kObjectId3);
+  helper.UpdateRegisteredIds(kHandlerName2, ids2);
+
+  // Unregister the first observer. It should not receive any further
+  // invalidations.
+  helper.UpdateRegisteredIds(kHandlerName1, ObjectIdSet());

[msw, 2012/08/07 20:09:30]

Should this be SetHandler(kHandlerName1, NULL), or should the comment explain
that this is unregistering the handler and all its object ids?

[akalin, 2012/08/07 23:38:05]

It should be the latter.  But I think I'll have to rewrite this test anyway for
the new API.

+
   ObjectIdPayloadMap dispatched_payloads;
   dispatched_payloads[kObjectId1] = "1";
   dispatched_payloads[kObjectId2] = "2";
@@ -153,4 +221,6 @@ TEST_F(SyncNotifierHelperTest, EmptySetUnregisters) {
   helper.EmitOnNotificationsDisabled(TRANSIENT_NOTIFICATION_ERROR);
 }
 
+}  // namespace
+
 }  // namespace syncer