[Android WebView] AwContentsClient.shouldCreate window callback part 2.

android_webview/native/aw_contents_io_thread_client_impl.cc

Index: android_webview/native/aw_contents_io_thread_client_impl.cc
diff --git a/android_webview/native/aw_contents_io_thread_client_impl.cc b/android_webview/native/aw_contents_io_thread_client_impl.cc
index d302608fb5022a1f636aa19ad5c355520235d5bc..85aed815390e99d9717a97b65e1d176bcf435f01 100644
--- a/android_webview/native/aw_contents_io_thread_client_impl.cc
+++ b/android_webview/native/aw_contents_io_thread_client_impl.cc
@@ -13,6 +13,7 @@
 #include "base/lazy_instance.h"
 #include "base/memory/linked_ptr.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/observer_list_threadsafe.h"
 #include "base/synchronization/lock.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/render_process_host.h"
@@ -49,15 +50,23 @@ static pair<int, int> GetRenderViewHostIdPair(RenderViewHost* rvh) {
 // RvhToIoThreadClientMap -----------------------------------------------------
 class RvhToIoThreadClientMap {
  public:
+  RvhToIoThreadClientMap()
+      : ready_observers_(NULL) {
+  }
   static RvhToIoThreadClientMap* GetInstance();
   void Insert(pair<int, int> rvh_id, JavaObjectWeakGlobalRef jdelegate);
   ScopedJavaLocalRef<jobject> Get(pair<int, int> rvh_id);
   void Erase(pair<int, int> rvh_id);
 
+  void AddObserver(AwContentsIoThreadClient::ReadyObserver* o);
+  void RemoveObserver(AwContentsIoThreadClient::ReadyObserver* o);
+
  private:
   static LazyInstance<RvhToIoThreadClientMap> g_instance_;
   base::Lock map_lock_;
   RenderViewHostToWeakDelegateMapType rvh_to_weak_delegate_map_;
+  scoped_refptr<ObserverListThreadSafe<
+      AwContentsIoThreadClient::ReadyObserver> > ready_observers_;
 };
 
 // static
@@ -73,6 +82,41 @@ void RvhToIoThreadClientMap::Insert(pair<int, int> rvh_id,
                                   JavaObjectWeakGlobalRef jdelegate) {
   base::AutoLock lock(map_lock_);
   rvh_to_weak_delegate_map_[rvh_id] = jdelegate;
+
+  if (ready_observers_) {
+    ready_observers_->Notify(
+        &AwContentsIoThreadClient::ReadyObserver::OnIoThreadClientReady,
+        rvh_id.first,
+        rvh_id.second);
+  }
+}
+
+void RvhToIoThreadClientMap::AddObserver(
+    AwContentsIoThreadClient::ReadyObserver* o) {
+  base::AutoLock lock(map_lock_);

[joth, 2012/11/16 06:24:31]

move down to 105

[mkosiba (inactive), 2012/11/16 12:28:32]

I'd prefer not to. Moving the lock would make it possible for 2 threads to new
up the ObserverList twice, so we'd have to DCHECK that this is called from a
specific thread.

I think having some methods on this class being threadsafe, while others being
XXX-thread only makes the API confusing.

[benm (inactive), 2012/11/16 12:29:09]

Done.

[benm (inactive), 2012/11/16 12:36:31]

As in, Martin makes a good point, so leaving the lock in place.

[joth, 2012/11/16 21:52:56]

It's not obvious ready_observers_ is intended to be guarded by map_lock_, partly
due to the name of map_lock_, and partly because it's not covered by the lock in
RemoveObserver() and partly because usage of the observer list itself is
inherently thread safe (just not assignment to the ref pointer).

How about using map_lock_ consistently in all methods, rename it to lock_, and
then you can use a plain old ObserverList, no need for it to do internal locking
too.
Ah hang on, the ThreadsafeObserverList also provides us with async callbacks on
the thread the client originally registered on. That's pretty important...
probably worth a note on line 68 that this is why we use the "ThreadSafe"
version. (although, see my other comments on the callback flow)

+
+  if (!ready_observers_) {
+    ready_observers_ =
+        new ObserverListThreadSafe<AwContentsIoThreadClient::ReadyObserver>();
+  }
+
+  ready_observers_->AddObserver(o);
+
+  // In case an AwIoThreadClient was added while we were adding the observer
+  // we should now notify.

[joth, 2012/11/16 06:24:31]

AIUI AddObserver() and Insert(() will both only be called on IO thread. so
perhaps this race couldn't happen? (and, the observer list need not be
threadsafe).

BUT we should add dchecks, to be sure.

[benm (inactive), 2012/11/16 12:29:09]

AddObserver will be coming from the IO thread, but calls to insert may come on
the UI thread, so I think it needs to stay.

+  RenderViewHostToWeakDelegateMapType::iterator it =
+      rvh_to_weak_delegate_map_.begin();
+  for (; it != rvh_to_weak_delegate_map_.end(); ++it) {
+    ready_observers_->Notify(
+        &AwContentsIoThreadClient::ReadyObserver::OnIoThreadClientReady,
+        it->first.first, it->first.second);
+  }
+}
+
+void RvhToIoThreadClientMap::RemoveObserver(
+    AwContentsIoThreadClient::ReadyObserver* o) {
+  DCHECK(ready_observers_);
+  ready_observers_->RemoveObserver(o);
 }
 
 ScopedJavaLocalRef<jobject> RvhToIoThreadClientMap::Get(
@@ -159,6 +203,18 @@ AwContentsIoThreadClient::FromID(int render_process_id, int render_view_id) {
 }
 
 // static
+void AwContentsIoThreadClient::AddReadyObserver(
+    AwContentsIoThreadClient::ReadyObserver* o) {
+  RvhToIoThreadClientMap::GetInstance()->AddObserver(o);
+}
+
+// static
+void AwContentsIoThreadClient::RemoveReadyObserver(
+    AwContentsIoThreadClient::ReadyObserver* o) {
+  RvhToIoThreadClientMap::GetInstance()->RemoveObserver(o);
+}
+
+// static
 void AwContentsIoThreadClientImpl::Associate(
     WebContents* web_contents,
     const JavaRef<jobject>& jclient) {