Adding compositor callbacks to RenderWidgetHostViewAndroid

content/browser/android/content_view_render_view.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 "content/browser/android/content_view_render_view.h"
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_string.h"
 #include "base/android/scoped_java_ref.h"
 #include "base/bind.h"
@@ skipping 35 matching lines @@
 
 void ContentViewRenderView::Destroy(JNIEnv* env, jobject obj) {
   delete this;
 }
 
 void ContentViewRenderView::SetCurrentContentView(
     JNIEnv* env, jobject obj, int native_content_view) {
   InitCompositor();
   ContentViewCoreImpl* content_view =
       reinterpret_cast<ContentViewCoreImpl*>(native_content_view);
-  if (content_view)
+  if (content_view) {
     compositor_->SetRootLayer(content_view->GetLayer());
-  else
+    RenderWidgetHostViewAndroid* rwhva =
+        content_view->GetRenderWidgetHostViewAndroid();
+    if (rwhva) {

[no sievers, 2013/10/10 17:48:03]

This is the sort of logic I was afraid of :)

And is it not broken if you navigate across domains in a given tab? Because in
that case a new RWHV gets created and attached to the ContentViewCore without us
noticing here. In other words, I don't think it's safe to use
content_view->GetRWHV().

You also have to make sure that a RWHV is not blocked on any callbacks when it
gets removed as an observer (pending ack callbacks).

If we mimicked what AURA does, it could go something like this:
- The RWHV adds itself as an observer
- when it's being detached from the compositor (or root window, i.e. layer tree)
it runs pending ack/commit callbacks

[powei, 2013/10/16 22:42:35]

Done.  Good point.  Please take a look at the new patch to make sure i did not
make any more of these ownership mistakes.

+      compositor_->AddObserver(rwhva);
+    }
+  } else
     compositor_->SetRootLayer(cc::Layer::Create());
 }
 
+void ContentViewRenderView::RemovePreviousContentView(JNIEnv* env,
+                                                      jobject obj,
+                                                      int native_content_view) {
+  DCHECK(compositor_);
+  ContentViewCoreImpl* content_view =
+      reinterpret_cast<ContentViewCoreImpl*>(native_content_view);
+  if (content_view) {
+    RenderWidgetHostViewAndroid* rwhva =
+        content_view->GetRenderWidgetHostViewAndroid();
+    if (rwhva) {
+      compositor_->RemoveObserver(rwhva);
+    }
+  }
+}
+
 void ContentViewRenderView::SurfaceCreated(
     JNIEnv* env, jobject obj, jobject jsurface) {
   InitCompositor();
   compositor_->SetSurface(jsurface);
 }
 
 void ContentViewRenderView::SurfaceDestroyed(JNIEnv* env, jobject obj) {
   compositor_->SetSurface(NULL);
 }
 
@@ skipping 24 matching lines @@
   JNIEnv* env = base::android::AttachCurrentThread();
   Java_ContentViewRenderView_onSwapBuffersCompleted(env, java_obj_.obj());
 }
 
 void ContentViewRenderView::InitCompositor() {
   if (!compositor_)
     compositor_.reset(Compositor::Create(this));
 }
 
 }  // namespace content