[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..3712066bffd43e3f9d16c5b069aaadc1f327bec1 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,40 @@ 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_);
+  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.
+  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);
+  }

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

thinking about it, rather than run through the entire list here I think it would
be clearer to just require the client to register its observer *before* it calls
AwContentsIoThreadClient::FromID.

or even nicer would be to have the client provide the process/view ID it wants
to observe and then we only notify them for that single client getting
registered (either immediately, or asynchronously, depending on if it's already
in the map or not). We'd then just pass up the
scoped_ptr<AwContentsIoThreadClient> as the OnClientReady function param.

.... 
I'm now also debating in my mind if the ReadyObserver is needed at all. A
different approach would be to make AwResourceDispatcherHostDelegate hold a list
of deferred 'waiting for client' throttles, and then in
AwContents::SetIoThreadClient() we could poke AwResourceDispatcherHostDelegate
and have it walk that list and see if any deferred throttles can now make
progress. (it's easy to get process & routing ID from WebContents, and then
AwResourceDispatcherHostDelegate would internally post a task to IO thread in
order to walk the list)
Benefit of this approach is it keeps this "map" class simpler (more like a dumb
container, as it's name suggests), and reduces coupling between the throttles
and the map.
WDYT?

[benm (inactive), 2012/11/28 20:00:05]

How would we poke the AwResourceDispatcherHostDelegate? I don't see a way of
getting to it other than making the list static and having a static
IoThreadClientReady function that posts the task to the IO thread and resumes
the throttle if appropriate. Is the Delegate a singleton? (I guess so, as the
ResourceDispatcherHost appears to be).

[joth, 2012/11/28 20:38:13]

AwResourceDispatcherHostDelegate is indeed  a singleton (see 
base::LazyInstance... 
    g_webview_resource_dispatcher_host_delegate) so we'd add a static method on
AwResourceDispatcherHostDelegate that would walk it's list.

But yeah it's not nice to rely on the singleton nature more than we have to, so
keeping direct observer coupling as you have it may be the best option. 

+}
+
+void RvhToIoThreadClientMap::RemoveObserver(
+    AwContentsIoThreadClient::ReadyObserver* o) {
+  DCHECK(ready_observers_);
+  ready_observers_->RemoveObserver(o);
 }
 
 ScopedJavaLocalRef<jobject> RvhToIoThreadClientMap::Get(
@@ -159,6 +202,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) {