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

android_webview/native/aw_contents_io_thread_client_impl.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 "android_webview/native/aw_contents_io_thread_client_impl.h"
 
 #include <map>
 #include <utility>
 
 #include "android_webview/native/intercepted_request_data_impl.h"
 #include "base/android/jni_helper.h"
 #include "base/android/jni_string.h"
 #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"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "googleurl/src/gurl.h"
 #include "net/url_request/url_request.h"
 
 #include "jni/AwContentsIoThreadClient_jni.h"
@@ skipping 16 matching lines @@
 typedef map<pair<int, int>, JavaObjectWeakGlobalRef>
     RenderViewHostToWeakDelegateMapType;
 
 static pair<int, int> GetRenderViewHostIdPair(RenderViewHost* rvh) {
   return pair<int, int>(rvh->GetProcess()->GetID(), rvh->GetRoutingID());
 }
 
 // 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
 LazyInstance<RvhToIoThreadClientMap> RvhToIoThreadClientMap::g_instance_ =
     LAZY_INSTANCE_INITIALIZER;
 
 // static
 RvhToIoThreadClientMap* RvhToIoThreadClientMap::GetInstance() {
   return g_instance_.Pointer();
 }
 
 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(
     pair<int, int> rvh_id) {
   base::AutoLock lock(map_lock_);
   RenderViewHostToWeakDelegateMapType::iterator weak_delegate_iterator =
       rvh_to_weak_delegate_map_.find(rvh_id);
   if (weak_delegate_iterator == rvh_to_weak_delegate_map_.end())
     return ScopedJavaLocalRef<jobject>();
 
@@ skipping 66 matching lines @@
   ScopedJavaLocalRef<jobject> java_delegate =
       RvhToIoThreadClientMap::GetInstance()->Get(rvh_id);
   if (java_delegate.is_null())
     return scoped_ptr<AwContentsIoThreadClient>();
 
   return scoped_ptr<AwContentsIoThreadClient>(
       new AwContentsIoThreadClientImpl(java_delegate));
 }
 
 // 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) {
   JNIEnv* env = AttachCurrentThread();
   // The ClientMapEntryUpdater lifespan is tied to the WebContents.
   new ClientMapEntryUpdater(env, web_contents, jclient.obj());
 }
 
 AwContentsIoThreadClientImpl::AwContentsIoThreadClientImpl(
     const JavaRef<jobject>& obj)
@@ skipping 52 matching lines @@
   JNIEnv* env = AttachCurrentThread();
   return Java_AwContentsIoThreadClient_shouldBlockNetworkLoads(
       env, java_object_.obj());
 }
 
 bool RegisterAwContentsIoThreadClientImpl(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }
 
 } // namespace android_webview