[Sync] Enable adding notifier observers from ProfileSyncService

chrome/browser/sync/profile_sync_service_unittest.cc

Index: chrome/browser/sync/profile_sync_service_unittest.cc
diff --git a/chrome/browser/sync/profile_sync_service_unittest.cc b/chrome/browser/sync/profile_sync_service_unittest.cc
index 1d589b34d3ebb2204de9a60f84ff43d4f1018e32..d36eadbe8ea60dba17931870ca21f973693e31cb 100644
--- a/chrome/browser/sync/profile_sync_service_unittest.cc
+++ b/chrome/browser/sync/profile_sync_service_unittest.cc
@@ -21,9 +21,11 @@
 #include "chrome/test/base/testing_profile.h"
 #include "content/public/common/content_client.h"
 #include "content/public/test/test_browser_thread.h"
+#include "google/cacheinvalidation/include/types.h"
 #include "sync/js/js_arg_list.h"
 #include "sync/js/js_event_details.h"
 #include "sync/js/js_test_util.h"
+#include "sync/notifier/mock_sync_notifier_observer.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "webkit/glue/webkit_glue.h"
@@ -40,6 +42,7 @@ using content::BrowserThread;
 using testing::_;
 using testing::AtLeast;
 using testing::AtMost;
+using testing::Mock;
 using testing::Return;
 using testing::StrictMock;
 
@@ -82,7 +85,7 @@ class ProfileSyncServiceTest : public testing::Test {
 
   void StartSyncService() {
     StartSyncServiceAndSetInitialSyncEnded(
-        true, true, false, true, true, syncer::STORAGE_IN_MEMORY);
+        true, true, false, true, 1, syncer::STORAGE_IN_MEMORY);
   }
 
   void StartSyncServiceAndSetInitialSyncEnded(
@@ -90,7 +93,7 @@ class ProfileSyncServiceTest : public testing::Test {
       bool issue_auth_token,
       bool synchronous_sync_configuration,
       bool sync_setup_completed,
-      bool expect_create_dtm,
+      int expected_create_dtm_calls,

[tim (not reviewing), 2012/08/02 17:43:23]

Oh boy. 1 and 0 :| An enum CREATE_DTM, DONT_CREATE_DTM would be far more clear,
but frankly, we probably don't care much beyond the fact that the DTM is created
if needed (it's arguably brittle to expect more, imo?). 

Instead of the WillOnce / Times(0) call, I think we should mimic what a "fake"
would do, and say 

   ON_CALL(*factory, CreateDataTypeManager()).
        WillByDefault(ReturnNewDataTypeManager());

... and, wait for it, remove the param to Start...!!!

[akalin, 2012/08/02 22:06:00]

Done.

       syncer::StorageOption storage_option) {
     if (!service_.get()) {
       SigninManager* signin =
@@ -113,14 +116,10 @@ class ProfileSyncServiceTest : public testing::Test {
       if (!sync_setup_completed)
         profile_->GetPrefs()->SetBoolean(prefs::kSyncHasSetupCompleted, false);
 
-      if (expect_create_dtm) {
-        // Register the bookmark data type.
-        EXPECT_CALL(*factory, CreateDataTypeManager(_, _)).
-            WillOnce(ReturnNewDataTypeManager());
-      } else {
-        EXPECT_CALL(*factory, CreateDataTypeManager(_, _)).
-            Times(0);
-      }
+      // Register the bookmark data type.
+      EXPECT_CALL(*factory, CreateDataTypeManager(_, _)).
+          Times(expected_create_dtm_calls).
+          WillRepeatedly(ReturnNewDataTypeManager());
 
       if (issue_auth_token) {
         IssueTestTokens();
@@ -252,7 +251,7 @@ TEST_F(ProfileSyncServiceTest, JsControllerHandlersBasic) {
 
 TEST_F(ProfileSyncServiceTest,
        JsControllerHandlersDelayedBackendInitialization) {
-  StartSyncServiceAndSetInitialSyncEnded(true, false, false, true, true,
+  StartSyncServiceAndSetInitialSyncEnded(true, false, false, true, 1,
                                          syncer::STORAGE_IN_MEMORY);
 
   StrictMock<syncer::MockJsEventHandler> event_handler;
@@ -294,7 +293,7 @@ TEST_F(ProfileSyncServiceTest, JsControllerProcessJsMessageBasic) {
 
 TEST_F(ProfileSyncServiceTest,
        JsControllerProcessJsMessageBasicDelayedBackendInitialization) {
-  StartSyncServiceAndSetInitialSyncEnded(true, false, false, true, true,
+  StartSyncServiceAndSetInitialSyncEnded(true, false, false, true, 1,
                                          syncer::STORAGE_IN_MEMORY);
 
   StrictMock<syncer::MockJsReplyHandler> reply_handler;
@@ -337,7 +336,7 @@ TEST_F(ProfileSyncServiceTest, TestStartupWithOldSyncData) {
   ASSERT_NE(-1,
             file_util::WriteFile(sync_file3, nonsense3, strlen(nonsense3)));
 
-  StartSyncServiceAndSetInitialSyncEnded(false, false, true, false, true,
+  StartSyncServiceAndSetInitialSyncEnded(false, false, true, false, 1,
                                          syncer::STORAGE_ON_DISK);
   EXPECT_FALSE(service_->HasSyncSetupCompleted());
   EXPECT_FALSE(service_->sync_initialized());
@@ -365,12 +364,70 @@ TEST_F(ProfileSyncServiceTest, TestStartupWithOldSyncData) {
 // recreate it.  This test is useful mainly when it is run under valgrind.  Its
 // expectations are not very interesting.
 TEST_F(ProfileSyncServiceTest, FailToOpenDatabase) {
-  StartSyncServiceAndSetInitialSyncEnded(false, true, true, true, false,
+  StartSyncServiceAndSetInitialSyncEnded(false, true, true, true, 0,
                                          syncer::STORAGE_INVALID);
 
   // The backend is not ready.  Ensure the PSS knows this.
   EXPECT_FALSE(service_->sync_initialized());
 }
 
+// Register for some IDs with the ProfileSyncService and trigger some
+// invalidation messages.  They should be received by the observer.
+// Then unregister and trigger the invalidation messages again.  Those
+// shouldn't be received by the observer.
+TEST_F(ProfileSyncServiceTest, UpdateRegisteredInvalidationIds) {
+  StartSyncService();
+
+  syncer::ObjectIdSet ids;
+  ids.insert(invalidation::ObjectId(1, "id1"));
+  ids.insert(invalidation::ObjectId(2, "id2"));
+  const syncer::ObjectIdPayloadMap& payloads =
+      syncer::ObjectIdSetToPayloadMap(ids, "payload");
+
+  StrictMock<syncer::MockSyncNotifierObserver> observer;
+  EXPECT_CALL(observer, OnNotificationsEnabled());
+  EXPECT_CALL(observer, OnNotificationsDisabled(
+      syncer::TRANSIENT_NOTIFICATION_ERROR));
+  EXPECT_CALL(observer, OnIncomingNotification(
+      payloads, syncer::REMOTE_NOTIFICATION));
+
+  service_->UpdateRegisteredInvalidationIds(&observer, ids);
+
+  SyncBackendHostForProfileSyncTest* const backend =
+      service_->GetBackendForTest();
+
+  backend->EmitOnNotificationsEnabled();
+  backend->EmitOnNotificationsDisabled(syncer::TRANSIENT_NOTIFICATION_ERROR);
+  backend->EmitOnIncomingNotification(payloads, syncer::REMOTE_NOTIFICATION);
+
+  Mock::VerifyAndClearExpectations(&observer);
+
+  service_->UpdateRegisteredInvalidationIds(&observer, syncer::ObjectIdSet());
+
+  backend->EmitOnNotificationsEnabled();
+  backend->EmitOnNotificationsDisabled(syncer::TRANSIENT_NOTIFICATION_ERROR);
+  backend->EmitOnIncomingNotification(payloads, syncer::REMOTE_NOTIFICATION);
+}
+
+// Register for some IDs with the ProfileSyncService, restart sync,
+// and trigger some invalidation messages.  They should still be
+// received by the observer.
+TEST_F(ProfileSyncServiceTest, UpdateRegisteredInvalidationIdsPersistence) {
+  StartSyncServiceAndSetInitialSyncEnded(true, true, false, true, 2,
+                                         syncer::STORAGE_IN_MEMORY);
+
+  StrictMock<syncer::MockSyncNotifierObserver> observer;
+  EXPECT_CALL(observer, OnNotificationsEnabled());
+
+  syncer::ObjectIdSet ids;
+  ids.insert(invalidation::ObjectId(3, "id3"));
+  service_->UpdateRegisteredInvalidationIds(&observer, ids);
+
+  service_->StopAndSuppress();
+  service_->UnsuppressAndStart();
+
+  service_->GetBackendForTest()->EmitOnNotificationsEnabled();
+}
+
 }  // namespace
 }  // namespace browser_sync