Refactor sync-specific parts out of SyncNotifier/SyncNotifierObserver

chrome/browser/sync/glue/chrome_sync_notification_bridge.cc

 // Copyright (c) 2012 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 "chrome/browser/sync/glue/chrome_sync_notification_bridge.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_observer.h"
 
 using content::BrowserThread;
 
 namespace browser_sync {
 
 ChromeSyncNotificationBridge::ChromeSyncNotificationBridge(
-    const Profile* profile)
-    : observers_(
-        new ObserverListThreadSafe<syncer::SyncNotifierObserver>()) {
+    const Profile* profile) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(profile);
   registrar_.Add(this, chrome::NOTIFICATION_SYNC_REFRESH_LOCAL,
                  content::Source<Profile>(profile));
   registrar_.Add(this, chrome::NOTIFICATION_SYNC_REFRESH_REMOTE,
                  content::Source<Profile>(profile));
 }
 
 ChromeSyncNotificationBridge::~ChromeSyncNotificationBridge() {}
 
 void ChromeSyncNotificationBridge::UpdateEnabledTypes(
     const syncer::ModelTypeSet types) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   enabled_types_ = types;
 }
 
-void ChromeSyncNotificationBridge::AddObserver(
-    syncer::SyncNotifierObserver* observer) {
-  observers_->AddObserver(observer);
-}
-
-void ChromeSyncNotificationBridge::RemoveObserver(
-    syncer::SyncNotifierObserver* observer) {
-  observers_->RemoveObserver(observer);
+void ChromeSyncNotificationBridge::UpdateRegisteredIds(
+    syncer::SyncNotifierObserver* handler,
+    const syncer::ObjectIdSet& ids) {
+  base::AutoLock locker_(helper_lock_);
+  helper_.UpdateRegisteredIds(handler, ids);
 }
 
 void ChromeSyncNotificationBridge::Observe(
     int type,
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   syncer::IncomingNotificationSource notification_source;
   if (type == chrome::NOTIFICATION_SYNC_REFRESH_LOCAL) {
     notification_source = syncer::LOCAL_NOTIFICATION;
   } else if (type == chrome::NOTIFICATION_SYNC_REFRESH_REMOTE) {
     notification_source = syncer::REMOTE_NOTIFICATION;
   } else {
     NOTREACHED() << "Unexpected notification type:" << type;
     return;
   }
 
   content::Details<const syncer::ModelTypePayloadMap>
       payload_details(details);
   syncer::ModelTypePayloadMap payload_map = *(payload_details.ptr());
 
   if (payload_map.empty()) {
     // No model types to invalidate, invalidating all enabled types.
     payload_map =
         syncer::ModelTypePayloadMapFromEnumSet(enabled_types_, std::string());
   }
 
-  observers_->Notify(
-      &syncer::SyncNotifierObserver::OnIncomingNotification,
-      payload_map, notification_source);
+  base::AutoLock locker_(helper_lock_);
+  helper_.DispatchInvalidationsToHandlers(

[akalin, 2012/07/19 00:42:06]

this won't work.  the invalidations must happen on the same thread as called by
UpdateRegisteredIds.  Ugh, this sucks, I forgot about this. :( Let me try to
think of a solution.

I may try to fix this in a separate patch in parallel.

[dcheng, 2012/07/19 18:31:05]

I don't quite understand the issue. Why isn't it a problem in the current code?

[akalin, 2012/07/19 23:07:26]

ObserverListThreadSafe guarantees that notifications are sent only on the same
thread as AddObserver, and only if RemoveObserver (which has to happen on the
same thread) hasn't been called yet.  Without using OLTS, you'd need to
guarantee that yourself (which is non-trivial).

FYI, my patch to fix this is https://chromiumcodereview.appspot.com/10808040/

[dcheng, 2012/07/20 00:57:04]

Ah. My fault for not looking at the implementation of OLTS. I assumed it was
pretty much just a bunch of locks =)

I'll wait for that patch to land then.

+      ModelTypePayloadMapToObjectIdPayloadMap(payload_map),
+      notification_source);
 }
 
 }  // namespace browser_sync