[Sync] Replace uses of ObserverListThreadSafe with WeakHandles

chrome/browser/sync/notifier/non_blocking_invalidation_notifier.cc

 // Copyright (c) 2011 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/notifier/non_blocking_invalidation_notifier.h"
 
 #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 {
 
 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(
       const notifier::NotifierOptions& notifier_options,
       const InvalidationVersionMap& initial_max_invalidation_versions,
       const browser_sync::WeakHandle<InvalidationVersionTracker>&
           invalidation_version_tracker,
       const std::string& client_info);
   void Teardown();
   void SetUniqueId(const std::string& unique_id);
   void SetState(const std::string& state);
   void UpdateCredentials(const std::string& email, const std::string& token);
   void UpdateEnabledTypes(const syncable::ModelTypeSet& enabled_types);
 
   // SyncNotifierObserver implementation (all called on I/O thread).
   virtual void OnIncomingNotification(
       const syncable::ModelTypePayloadMap& type_payloads);
   virtual void OnNotificationStateChange(bool notifications_enabled);
   virtual void StoreState(const std::string& state);
 
  private:
   friend class
       base::RefCountedThreadSafe<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() {
 }
 
 void NonBlockingInvalidationNotifier::Core::Initialize(
     const notifier::NotifierOptions& notifier_options,
     const InvalidationVersionMap& initial_max_invalidation_versions,
     const browser_sync::WeakHandle<InvalidationVersionTracker>&
         invalidation_version_tracker,
@@ skipping 14 matching lines @@
 }
 
 
 void NonBlockingInvalidationNotifier::Core::Teardown() {
   DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
   invalidation_notifier_->RemoveObserver(this);
   invalidation_notifier_.reset();
   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());
   invalidation_notifier_->SetUniqueId(unique_id);
 }
 
 void NonBlockingInvalidationNotifier::Core::SetState(
     const std::string& state) {
   DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
   invalidation_notifier_->SetState(state);
 }
 
 void NonBlockingInvalidationNotifier::Core::UpdateCredentials(
     const std::string& email, const std::string& token) {
   DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
   invalidation_notifier_->UpdateCredentials(email, token);
 }
 
 void NonBlockingInvalidationNotifier::Core::UpdateEnabledTypes(
     const syncable::ModelTypeSet& enabled_types) {
   DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
   invalidation_notifier_->UpdateEnabledTypes(enabled_types);
 }
 
 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(
     const notifier::NotifierOptions& notifier_options,
     const InvalidationVersionMap& initial_max_invalidation_versions,
     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->
               GetIOMessageLoopProxy()) {
   if (!io_message_loop_proxy_->PostTask(
           FROM_HERE,
           base::Bind(
               &NonBlockingInvalidationNotifier::Core::Initialize,
               core_.get(),
               notifier_options,
@@ skipping 10 matching lines @@
           FROM_HERE,
           base::Bind(&NonBlockingInvalidationNotifier::Core::Teardown,
                      core_.get()))) {
     NOTREACHED();
   }
 }
 
 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(
     const std::string& unique_id) {
   DCHECK(parent_message_loop_proxy_->BelongsToCurrentThread());
   if (!io_message_loop_proxy_->PostTask(
           FROM_HERE,
           base::Bind(&NonBlockingInvalidationNotifier::Core::SetUniqueId,
                      core_.get(), unique_id))) {
     NOTREACHED();
@@ skipping 32 matching lines @@
   }
 }
 
 void NonBlockingInvalidationNotifier::SendNotification(
     const syncable::ModelTypeSet& changed_types) {
   DCHECK(parent_message_loop_proxy_->BelongsToCurrentThread());
   // InvalidationClient doesn't implement SendNotification(), so no
   // 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