Revert r147801 "Refactor sync-specific parts out of SyncNotifier/Sync…

…NotifierObserver"

This broke sync_integration_tests like it had never been run.

Note that sync_integration_tests is not on GateKeeper but redness is still sad.

TBR=dcheng@chromium.org
BUG=
TEST=

Review URL: https://chromiumcodereview.appspot.com/10806060

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@147817 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/sync/glue/DEPS

@@ -10,10 +10,8 @@ include_rules = [
 
   # Should these live in their own "includes" (e.g) directory(ies)?
   # Bug 19878.
-  "+sync/notifier/invalidation_util.h",
   "+sync/notifier/mock_sync_notifier_observer.h",
   "+sync/notifier/sync_notifier.h",
-  "+sync/notifier/sync_notifier_helper.h",
   "+sync/notifier/sync_notifier_factory.h",
   "+sync/notifier/sync_notifier_observer.h",
 

chrome/browser/sync/glue/bridged_sync_notifier.cc

@@ -18,12 +18,18 @@ BridgedSyncNotifier::BridgedSyncNotifier(
 BridgedSyncNotifier::~BridgedSyncNotifier() {
 }
 
-void BridgedSyncNotifier::UpdateRegisteredIds(
-    syncer::SyncNotifierObserver* handler,
-    const syncer::ObjectIdSet& ids) {
+void BridgedSyncNotifier::AddObserver(
+    syncer::SyncNotifierObserver* observer) {
   if (delegate_.get())
-   delegate_->UpdateRegisteredIds(handler, ids);
-  bridge_->UpdateRegisteredIds(handler, ids);
+   delegate_->AddObserver(observer);
+  bridge_->AddObserver(observer);
+}
+
+void BridgedSyncNotifier::RemoveObserver(
+    syncer::SyncNotifierObserver* observer) {
+  bridge_->RemoveObserver(observer);
+  if (delegate_.get())
+    delegate_->RemoveObserver(observer);
 }
 
 void BridgedSyncNotifier::SetUniqueId(const std::string& unique_id) {
@@ -42,6 +48,12 @@ void BridgedSyncNotifier::UpdateCredentials(
     delegate_->UpdateCredentials(email, token);
 }
 
+void BridgedSyncNotifier::UpdateEnabledTypes(
+    syncer::ModelTypeSet enabled_types) {
+  if (delegate_.get())
+    delegate_->UpdateEnabledTypes(enabled_types);
+}
+
 void BridgedSyncNotifier::SendNotification(
     syncer::ModelTypeSet changed_types) {
   if (delegate_.get())

chrome/browser/sync/glue/bridged_sync_notifier.h

@@ -28,13 +28,18 @@ class BridgedSyncNotifier : public syncer::SyncNotifier {
   virtual ~BridgedSyncNotifier();
 
   // SyncNotifier implementation. Passes through all calls to the delegate.
-  // UpdateRegisteredIds calls will also be forwarded to the bridge.
-  virtual void UpdateRegisteredIds(syncer::SyncNotifierObserver* handler,
-                                   const syncer::ObjectIdSet& ids) OVERRIDE;
+  // AddObserver/RemoveObserver will also register/deregister |observer| with
+  // the bridge.
+  virtual void AddObserver(
+      syncer::SyncNotifierObserver* observer) OVERRIDE;
+  virtual void RemoveObserver(
+      syncer::SyncNotifierObserver* observer) OVERRIDE;
   virtual void SetUniqueId(const std::string& unique_id) OVERRIDE;
   virtual void SetStateDeprecated(const std::string& state) OVERRIDE;
   virtual void UpdateCredentials(
       const std::string& email, const std::string& token) OVERRIDE;
+  virtual void UpdateEnabledTypes(
+      syncer::ModelTypeSet enabled_types) OVERRIDE;
   virtual void SendNotification(
       syncer::ModelTypeSet changed_types) OVERRIDE;
 

chrome/browser/sync/glue/bridged_sync_notifier_unittest.cc

@@ -35,26 +35,27 @@ class MockChromeSyncNotificationBridge : public ChromeSyncNotificationBridge {
       : ChromeSyncNotificationBridge(profile, sync_task_runner) {}
   virtual ~MockChromeSyncNotificationBridge() {}
 
-  MOCK_METHOD2(UpdateRegisteredIds, void(syncer::SyncNotifierObserver*,
-                                         const syncer::ObjectIdSet&));
+  MOCK_METHOD1(AddObserver, void(syncer::SyncNotifierObserver*));
+  MOCK_METHOD1(RemoveObserver, void(syncer::SyncNotifierObserver*));
 };
 
 class MockSyncNotifier : public syncer::SyncNotifier {
  public:
   MockSyncNotifier() {}
   virtual ~MockSyncNotifier() {}
 
-  MOCK_METHOD2(UpdateRegisteredIds,
-               void(syncer::SyncNotifierObserver*, const syncer::ObjectIdSet&));
+  MOCK_METHOD1(AddObserver, void(syncer::SyncNotifierObserver*));
+  MOCK_METHOD1(RemoveObserver, void(syncer::SyncNotifierObserver*));
   MOCK_METHOD1(SetUniqueId, void(const std::string&));
   MOCK_METHOD1(SetStateDeprecated, void(const std::string&));
   MOCK_METHOD2(UpdateCredentials, void(const std::string&, const std::string&));
+  MOCK_METHOD1(UpdateEnabledTypes, void(syncer::ModelTypeSet));
   MOCK_METHOD1(SendNotification, void(syncer::ModelTypeSet));
 };
 
 // All tests just verify that each call is passed through to the delegate, with
-// the exception of UpdateRegisteredIds, which also verifies the call is
-// forwarded to the bridge.
+// the exception of AddObserver/RemoveObserver, which also verify the observer
+// is registered with the bridge.
 class BridgedSyncNotifierTest : public testing::Test {
  public:
   BridgedSyncNotifierTest()
@@ -73,13 +74,18 @@ class BridgedSyncNotifierTest : public testing::Test {
   BridgedSyncNotifier bridged_notifier_;
 };
 
-TEST_F(BridgedSyncNotifierTest, UpdateRegisteredIds) {
+TEST_F(BridgedSyncNotifierTest, AddObserver) {
   syncer::MockSyncNotifierObserver observer;
-  EXPECT_CALL(mock_bridge_, UpdateRegisteredIds(
-      &observer, syncer::ObjectIdSet()));
-  EXPECT_CALL(*mock_delegate_, UpdateRegisteredIds(
-      &observer, syncer::ObjectIdSet()));
-  bridged_notifier_.UpdateRegisteredIds(&observer, syncer::ObjectIdSet());
+  EXPECT_CALL(mock_bridge_, AddObserver(&observer));
+  EXPECT_CALL(*mock_delegate_, AddObserver(&observer));
+  bridged_notifier_.AddObserver(&observer);
+}
+
+TEST_F(BridgedSyncNotifierTest, RemoveObserver) {
+  syncer::MockSyncNotifierObserver observer;
+  EXPECT_CALL(mock_bridge_, RemoveObserver(&observer));
+  EXPECT_CALL(*mock_delegate_, RemoveObserver(&observer));
+  bridged_notifier_.RemoveObserver(&observer);
 }
 
 TEST_F(BridgedSyncNotifierTest, SetUniqueId) {
@@ -101,6 +107,13 @@ TEST_F(BridgedSyncNotifierTest, UpdateCredentials) {
   bridged_notifier_.UpdateCredentials(email, token);
 }
 
+TEST_F(BridgedSyncNotifierTest, UpdateEnabledTypes) {
+  syncer::ModelTypeSet enabled_types(syncer::BOOKMARKS, syncer::PREFERENCES);
+  EXPECT_CALL(*mock_delegate_,
+              UpdateEnabledTypes(HasModelTypes(enabled_types)));
+  bridged_notifier_.UpdateEnabledTypes(enabled_types);
+}
+
 TEST_F(BridgedSyncNotifierTest, SendNotification) {
   syncer::ModelTypeSet changed_types(syncer::SESSIONS, syncer::EXTENSIONS);
   EXPECT_CALL(*mock_delegate_, SendNotification(HasModelTypes(changed_types)));

chrome/browser/sync/glue/chrome_sync_notification_bridge.cc

@@ -6,10 +6,10 @@
 
 #include "base/bind.h"
 #include "base/location.h"
+#include "base/observer_list.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_service.h"
-#include "sync/notifier/sync_notifier_helper.h"
 #include "sync/notifier/sync_notifier_observer.h"
 
 using content::BrowserThread;
@@ -25,11 +25,11 @@ class ChromeSyncNotificationBridge::Core
 
   // All member functions below must be called on the sync task runner.
 
-  void UpdateRegisteredIds(syncer::SyncNotifierObserver* handler,
-                           const syncer::ObjectIdSet& ids);
+  void AddObserver(syncer::SyncNotifierObserver* observer);
+  void RemoveObserver(syncer::SyncNotifierObserver* observer);
 
   void EmitNotification(
-      const syncer::ObjectIdPayloadMap& payload_map,
+      syncer::ModelTypePayloadMap payload_map,
       syncer::IncomingNotificationSource notification_source);
 
  private:
@@ -41,7 +41,8 @@ class ChromeSyncNotificationBridge::Core
   const scoped_refptr<base::SequencedTaskRunner> sync_task_runner_;
 
   // Used only on |sync_task_runner_|.
-  syncer::SyncNotifierHelper helper_;
+  syncer::ModelTypeSet enabled_types_;
+  ObserverList<syncer::SyncNotifierObserver> observers_;
 };
 
 ChromeSyncNotificationBridge::Core::Core(
@@ -56,18 +57,25 @@ ChromeSyncNotificationBridge::Core::~Core() {
          sync_task_runner_->RunsTasksOnCurrentThread());
 }
 
-void ChromeSyncNotificationBridge::Core::UpdateRegisteredIds(
-    syncer::SyncNotifierObserver* handler,
-    const syncer::ObjectIdSet& ids) {
+void ChromeSyncNotificationBridge::Core::AddObserver(
+    syncer::SyncNotifierObserver* observer) {
   DCHECK(sync_task_runner_->RunsTasksOnCurrentThread());
-  helper_.UpdateRegisteredIds(handler, ids);
+  observers_.AddObserver(observer);
+}
+
+void ChromeSyncNotificationBridge::Core::RemoveObserver(
+    syncer::SyncNotifierObserver* observer) {
+  DCHECK(sync_task_runner_->RunsTasksOnCurrentThread());
+  observers_.RemoveObserver(observer);
 }
 
 void ChromeSyncNotificationBridge::Core::EmitNotification(
-    const syncer::ObjectIdPayloadMap& payload_map,
+    syncer::ModelTypePayloadMap payload_map,
     syncer::IncomingNotificationSource notification_source) {
   DCHECK(sync_task_runner_->RunsTasksOnCurrentThread());
-  helper_.DispatchInvalidationsToHandlers(payload_map, notification_source);
+  FOR_EACH_OBSERVER(
+      syncer::SyncNotifierObserver, observers_,
+      OnIncomingNotification(payload_map, notification_source));
 }
 
 ChromeSyncNotificationBridge::ChromeSyncNotificationBridge(
@@ -91,11 +99,16 @@ void ChromeSyncNotificationBridge::UpdateEnabledTypes(
   enabled_types_ = types;
 }
 
-void ChromeSyncNotificationBridge::UpdateRegisteredIds(
-    syncer::SyncNotifierObserver* handler,
-    const syncer::ObjectIdSet& ids) {
+void ChromeSyncNotificationBridge::AddObserver(
+    syncer::SyncNotifierObserver* observer) {
+  DCHECK(sync_task_runner_->RunsTasksOnCurrentThread());
+  core_->AddObserver(observer);
+}
+
+void ChromeSyncNotificationBridge::RemoveObserver(
+    syncer::SyncNotifierObserver* observer) {
   DCHECK(sync_task_runner_->RunsTasksOnCurrentThread());
-  core_->UpdateRegisteredIds(handler, ids);
+  core_->RemoveObserver(observer);
 }
 
 void ChromeSyncNotificationBridge::Observe(
@@ -127,9 +140,7 @@ void ChromeSyncNotificationBridge::Observe(
   sync_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&Core::EmitNotification,
-                 core_,
-                 ModelTypePayloadMapToObjectIdPayloadMap(payload_map),
-                 notification_source));
+                 core_, payload_map, notification_source));
 }
 
 }  // namespace browser_sync

chrome/browser/sync/glue/chrome_sync_notification_bridge.h

@@ -11,7 +11,6 @@
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "sync/internal_api/public/base/model_type.h"
-#include "sync/notifier/invalidation_util.h"
 
 class Profile;
 
@@ -40,8 +39,8 @@ class ChromeSyncNotificationBridge : public content::NotificationObserver {
   void UpdateEnabledTypes(const syncer::ModelTypeSet enabled_types);
 
   // Must be called on the sync task runner.
-  virtual void UpdateRegisteredIds(syncer::SyncNotifierObserver* handler,
-                                   const syncer::ObjectIdSet& ids);
+  virtual void AddObserver(syncer::SyncNotifierObserver* observer);
+  virtual void RemoveObserver(syncer::SyncNotifierObserver* observer);
 
   // NotificationObserver implementation. Called on UI thread.
   virtual void Observe(int type,

chrome/browser/sync/glue/chrome_sync_notification_bridge_unittest.cc

@@ -44,7 +44,7 @@ class FakeSyncNotifierObserver : public syncer::SyncNotifierObserver {
   FakeSyncNotifierObserver(
       const scoped_refptr<base::SequencedTaskRunner>& sync_task_runner,
       ChromeSyncNotificationBridge* bridge,
-      const syncer::ObjectIdPayloadMap& expected_payloads,
+      const syncer::ModelTypePayloadMap& expected_payloads,
       syncer::IncomingNotificationSource expected_source)
       : sync_task_runner_(sync_task_runner),
         bridge_(bridge),
@@ -53,31 +53,25 @@ class FakeSyncNotifierObserver : public syncer::SyncNotifierObserver {
         expected_payloads_(expected_payloads),
         expected_source_(expected_source) {
     DCHECK(sync_task_runner_->RunsTasksOnCurrentThread());
-    // TODO(dcheng): We might want a function to go from ObjectIdPayloadMap ->
-    // ObjectIdSet to avoid this rather long incantation...
-    const syncer::ObjectIdSet& ids = syncer::ModelTypeSetToObjectIdSet(
-        syncer::ModelTypePayloadMapToEnumSet(
-            syncer::ObjectIdPayloadMapToModelTypePayloadMap(
-                expected_payloads)));
-    bridge_->UpdateRegisteredIds(this, ids);
+    bridge_->AddObserver(this);
   }
 
   virtual ~FakeSyncNotifierObserver() {
     DCHECK(sync_task_runner_->RunsTasksOnCurrentThread());
-    bridge_->UpdateRegisteredIds(this, syncer::ObjectIdSet());
+    bridge_->RemoveObserver(this);
   }
 
   // SyncNotifierObserver implementation.
   virtual void OnIncomingNotification(
-      const syncer::ObjectIdPayloadMap& id_payloads,
+      const syncer::ModelTypePayloadMap& type_payloads,
       syncer::IncomingNotificationSource source) OVERRIDE {
     DCHECK(sync_task_runner_->RunsTasksOnCurrentThread());
     notification_count_++;
     if (source != expected_source_) {
       LOG(ERROR) << "Received notification with wrong source";
       received_improper_notification_ = true;
     }
-    if (expected_payloads_ != id_payloads) {
+    if (expected_payloads_ != type_payloads) {
       LOG(ERROR) << "Received wrong payload";
       received_improper_notification_ = true;
     }
@@ -100,7 +94,7 @@ class FakeSyncNotifierObserver : public syncer::SyncNotifierObserver {
   ChromeSyncNotificationBridge* const bridge_;
   bool received_improper_notification_;
   size_t notification_count_;
-  const syncer::ObjectIdPayloadMap expected_payloads_;
+  const syncer::ModelTypePayloadMap expected_payloads_;
   const syncer::IncomingNotificationSource expected_source_;
 };
 
@@ -154,7 +148,7 @@ class ChromeSyncNotificationBridgeTest : public testing::Test {
   }
 
   void CreateObserverOnSyncThread(
-      const syncer::ObjectIdPayloadMap& expected_payloads,
+      syncer::ModelTypePayloadMap expected_payloads,
       syncer::IncomingNotificationSource expected_source) {
     DCHECK(sync_thread_.message_loop_proxy()->RunsTasksOnCurrentThread());
     sync_observer_ = new FakeSyncNotifierObserver(
@@ -165,16 +159,14 @@ class ChromeSyncNotificationBridgeTest : public testing::Test {
   }
 
   void CreateObserverWithExpectations(
-      const syncer::ModelTypePayloadMap& expected_payloads,
+      syncer::ModelTypePayloadMap expected_payloads,
       syncer::IncomingNotificationSource expected_source) {
-    const syncer::ObjectIdPayloadMap& expected_id_payloads =
-        syncer::ModelTypePayloadMapToObjectIdPayloadMap(expected_payloads);
     ASSERT_TRUE(sync_thread_.message_loop_proxy()->PostTask(
         FROM_HERE,
         base::Bind(
             &ChromeSyncNotificationBridgeTest::CreateObserverOnSyncThread,
             base::Unretained(this),
-            expected_id_payloads,
+            expected_payloads,
             expected_source)));
     BlockForSyncThread();
   }

sync/internal_api/sync_manager_impl.cc

@@ -39,7 +39,6 @@
 #include "sync/js/js_event_details.h"
 #include "sync/js/js_event_handler.h"
 #include "sync/js/js_reply_handler.h"
-#include "sync/notifier/invalidation_util.h"
 #include "sync/notifier/notifications_disabled_reason.h"
 #include "sync/notifier/sync_notifier.h"
 #include "sync/protocol/encryption.pb.h"
@@ -482,6 +481,8 @@ bool SyncManagerImpl::Init(
   if (!success)
     return false;
 
+  sync_notifier_->AddObserver(this);
+
   return success;
 }
 
@@ -724,8 +725,7 @@ void SyncManagerImpl::UpdateCredentials(
 void SyncManagerImpl::UpdateEnabledTypes(
     const ModelTypeSet& enabled_types) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  sync_notifier_->UpdateRegisteredIds(this,
-                                      ModelTypeSetToObjectIdSet(enabled_types));
+  sync_notifier_->UpdateEnabledTypes(enabled_types);
 }
 
 void SyncManagerImpl::SetEncryptionPassphrase(
@@ -1195,7 +1195,7 @@ void SyncManagerImpl::ShutdownOnSyncThread() {
   RemoveObserver(&debug_info_event_listener_);
 
   if (sync_notifier_.get()) {
-    sync_notifier_->UpdateRegisteredIds(this, ObjectIdSet());
+    sync_notifier_->RemoveObserver(this);
   }
   sync_notifier_.reset();
 
@@ -1781,11 +1781,9 @@ void SyncManagerImpl::OnNotificationsDisabled(
 }
 
 void SyncManagerImpl::OnIncomingNotification(
-    const ObjectIdPayloadMap& id_payloads,
+    const ModelTypePayloadMap& type_payloads,
     IncomingNotificationSource source) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  const ModelTypePayloadMap& type_payloads =
-      ObjectIdPayloadMapToModelTypePayloadMap(id_payloads);
   if (source == LOCAL_NOTIFICATION) {
     scheduler_->ScheduleNudgeWithPayloadsAsync(
         TimeDelta::FromMilliseconds(kSyncRefreshDelayMsec),