[Sync] Replace uses of ObserverListThreadSafe with WeakHandles

chrome/browser/sync/notifier/non_blocking_invalidation_notifier.cc

Index: chrome/browser/sync/notifier/non_blocking_invalidation_notifier.cc
diff --git a/chrome/browser/sync/notifier/non_blocking_invalidation_notifier.cc b/chrome/browser/sync/notifier/non_blocking_invalidation_notifier.cc
index 40a1f91c9eb8edc6912e93d753e0b25eaa80f6a9..674c223d48824f76c21181da1c964f05a43e1217 100644
--- a/chrome/browser/sync/notifier/non_blocking_invalidation_notifier.cc
+++ b/chrome/browser/sync/notifier/non_blocking_invalidation_notifier.cc
@@ -7,10 +7,8 @@
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop.h"
-#include "base/observer_list_threadsafe.h"
 #include "base/threading/thread.h"
 #include "chrome/browser/sync/notifier/invalidation_notifier.h"
-#include "chrome/browser/sync/notifier/sync_notifier_observer.h"
 
 namespace sync_notifier {
 
@@ -18,12 +16,11 @@ class NonBlockingInvalidationNotifier::Core
     : public base::RefCountedThreadSafe<NonBlockingInvalidationNotifier::Core>,
       public SyncNotifierObserver {
  public:
-  // Called on parent thread.
-  Core();
-
-  // Called on parent thread.
-  void AddObserver(SyncNotifierObserver* observer);
-  void RemoveObserver(SyncNotifierObserver* observer);
+  // Called on parent thread.  |delegate_observer| should be
+  // initialized.
+  explicit Core(
+      const browser_sync::WeakHandle<SyncNotifierObserver>&
+          delegate_observer);
 
   // Helpers called on I/O thread.
   void Initialize(
@@ -50,14 +47,19 @@ class NonBlockingInvalidationNotifier::Core
   // Called on parent or I/O thread.
   ~Core();
 
+  // The variables below should be used only on the I/O thread.
+  const browser_sync::WeakHandle<SyncNotifierObserver> delegate_observer_;
   scoped_ptr<InvalidationNotifier> invalidation_notifier_;
   scoped_refptr<base::MessageLoopProxy> io_message_loop_proxy_;
-  scoped_refptr<ObserverListThreadSafe<SyncNotifierObserver> > observers_;
+
   DISALLOW_COPY_AND_ASSIGN(Core);
 };
 
-NonBlockingInvalidationNotifier::Core::Core()
-    : observers_(new ObserverListThreadSafe<SyncNotifierObserver>()) {
+NonBlockingInvalidationNotifier::Core::Core(
+    const browser_sync::WeakHandle<SyncNotifierObserver>&
+        delegate_observer)
+    : delegate_observer_(delegate_observer) {
+  DCHECK(delegate_observer_.IsInitialized());
 }
 
 NonBlockingInvalidationNotifier::Core::~Core() {
@@ -92,16 +94,6 @@ void NonBlockingInvalidationNotifier::Core::Teardown() {
   io_message_loop_proxy_ = NULL;
 }
 
-void NonBlockingInvalidationNotifier::Core::AddObserver(
-    SyncNotifierObserver* observer) {
-  observers_->AddObserver(observer);
-}
-
-void NonBlockingInvalidationNotifier::Core::RemoveObserver(
-    SyncNotifierObserver* observer) {
-  observers_->RemoveObserver(observer);
-}
-
 void NonBlockingInvalidationNotifier::Core::SetUniqueId(
     const std::string& unique_id) {
   DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
@@ -129,21 +121,24 @@ void NonBlockingInvalidationNotifier::Core::UpdateEnabledTypes(
 void NonBlockingInvalidationNotifier::Core::OnIncomingNotification(
         const syncable::ModelTypePayloadMap& type_payloads) {
   DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
-  observers_->Notify(&SyncNotifierObserver::OnIncomingNotification,
-                     type_payloads);
+  delegate_observer_.Call(FROM_HERE,

[Nicolas Zea, 2011/11/17 21:35:45]

I wonder, would it be possible to keep the observer_list in the core, have each
observer actually be a weakhandle and then have something like:

FOR_EACH_OBSERVER(SyncNotifierObserver, observers_, Call(FROM_HERE, ..., ...)).

Might need to make a special weak handle observerlist macro. It's seems
unnecessary to have to do this extra plumbing of core -> notifier -> observer
just because you might have multiple observers.

WDYT?

[akalin, 2011/11/18 00:04:32]

I looked into this and basically it's much more complicated than the current
solution.

The first problem is that ObserverLists store pointers, so it would have to
store WeakHandle<T>*, which is weird.

However, it's technically possible to have a WeakHandle to the ObserverList
itself, since it implements SupportsWeakPtr.  I'd have to make a new function,
say, ObserverList<T>::Notify(), or NotifyObservers(..., ObserverList<T>*) and
add support for binding to functions/callbacks to WeakHandle.  However, the
major weakness of this is that the first argument to NotifyObservers (the
function to apply to each observer) would be a callback, but then it would have
to be a zero-arg member function, since there's no support for binding just a
member function arguments (as opposed to the object variable).  Even if we had
that, it seems less clear, IMO.

Another possibility would be a special WeakHandleObserverList<T> which would
store WeakHandle<T>s.  The major problem with that, though, is that it's in
general impossible to remove an observer even if you're on the same thread
(i.e., you're able to get the WeakPtr), since the WeakPtr may already be
invalid, and thus the pointer is NULL, so you don't know which one to remove. 
Although then I guess it doesn't really matter.  But in this case it would
require the observer type itself to expose WeakPtrs, since the public interface
still takes a pointer.

So yeah, I think this is the best solution. :/

+                          &SyncNotifierObserver::OnIncomingNotification,
+                          type_payloads);
 }
 
 void NonBlockingInvalidationNotifier::Core::OnNotificationStateChange(
         bool notifications_enabled) {
   DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
-  observers_->Notify(&SyncNotifierObserver::OnNotificationStateChange,
-                     notifications_enabled);
+  delegate_observer_.Call(FROM_HERE,
+                          &SyncNotifierObserver::OnNotificationStateChange,
+                          notifications_enabled);
 }
 
 void NonBlockingInvalidationNotifier::Core::StoreState(
     const std::string& state) {
   DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
-  observers_->Notify(&SyncNotifierObserver::StoreState, state);
+  delegate_observer_.Call(FROM_HERE,
+                          &SyncNotifierObserver::StoreState, state);
 }
 
 NonBlockingInvalidationNotifier::NonBlockingInvalidationNotifier(
@@ -152,7 +147,10 @@ NonBlockingInvalidationNotifier::NonBlockingInvalidationNotifier(
     const browser_sync::WeakHandle<InvalidationVersionTracker>&
         invalidation_version_tracker,
     const std::string& client_info)
-        : core_(new Core),
+        : weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
+          core_(
+              new Core(browser_sync::MakeWeakHandle(
+                  weak_ptr_factory_.GetWeakPtr()))),
           parent_message_loop_proxy_(
               base::MessageLoopProxy::current()),
           io_message_loop_proxy_(notifier_options.request_context_getter->
@@ -183,13 +181,13 @@ NonBlockingInvalidationNotifier::~NonBlockingInvalidationNotifier() {
 void NonBlockingInvalidationNotifier::AddObserver(
     SyncNotifierObserver* observer) {
   DCHECK(parent_message_loop_proxy_->BelongsToCurrentThread());
-  core_->AddObserver(observer);
+  observers_.AddObserver(observer);
 }
 
 void NonBlockingInvalidationNotifier::RemoveObserver(
     SyncNotifierObserver* observer) {
   DCHECK(parent_message_loop_proxy_->BelongsToCurrentThread());
-  core_->RemoveObserver(observer);
+  observers_.RemoveObserver(observer);
 }
 
 void NonBlockingInvalidationNotifier::SetUniqueId(
@@ -242,4 +240,25 @@ void NonBlockingInvalidationNotifier::SendNotification(
   // need to forward on the call.
 }
 
+void NonBlockingInvalidationNotifier::OnIncomingNotification(
+        const syncable::ModelTypePayloadMap& type_payloads) {
+  DCHECK(parent_message_loop_proxy_->BelongsToCurrentThread());
+  FOR_EACH_OBSERVER(SyncNotifierObserver, observers_,
+                    OnIncomingNotification(type_payloads));
+}
+
+void NonBlockingInvalidationNotifier::OnNotificationStateChange(
+        bool notifications_enabled) {
+  DCHECK(parent_message_loop_proxy_->BelongsToCurrentThread());
+  FOR_EACH_OBSERVER(SyncNotifierObserver, observers_,
+                    OnNotificationStateChange(notifications_enabled));
+}
+
+void NonBlockingInvalidationNotifier::StoreState(
+    const std::string& state) {
+  DCHECK(parent_message_loop_proxy_->BelongsToCurrentThread());
+  FOR_EACH_OBSERVER(SyncNotifierObserver, observers_,
+                    StoreState(state));
+}
+
 }  // namespace sync_notifier