Refactor ContentViewClient (1/6)

chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastShellActivity.java

Index: chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastShellActivity.java
diff --git a/chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastShellActivity.java b/chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastShellActivity.java
index 6c576f55ee091d1a7d0a1aee00be44d679eb954e..879a58fad4435f1a9c2c7507a0e899d8d981bf14 100644
--- a/chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastShellActivity.java
+++ b/chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastShellActivity.java
@@ -21,7 +21,6 @@ import android.widget.Toast;
 
 import org.chromium.base.Log;
 import org.chromium.content.browser.ActivityContentVideoViewEmbedder;
-import org.chromium.content.browser.ContentVideoViewEmbedder;
 import org.chromium.content.browser.ContentViewClient;
 import org.chromium.content.browser.ContentViewCore;
 import org.chromium.ui.base.WindowAndroid;
@@ -115,12 +114,12 @@ public class CastShellActivity extends Activity {
         Log.d(TAG, "onCreate startupUrl: %s", url);
         mNativeCastWindow = mCastWindowManager.launchCastWindow(url);
 
-        getActiveContentViewCore().setContentViewClient(new ContentViewClient() {
-            @Override
-            public ContentVideoViewEmbedder getContentVideoViewEmbedder() {
-                return new ActivityContentVideoViewEmbedder(CastShellActivity.this);
-            }
-        });
+        getActiveContentViewCore().setContentViewClient(new ContentViewClient());
+        CastWindowAndroid shellView = getActiveCastWindow();
+        if (shellView != null) {
+            shellView.setContentVideoViewEmbedder(

[Simeon, 2016/09/28 00:24:52]

I think a better way to do this is to pass the ContentVideoViewEmbedder into
launchCastWindow().

So we could have:

mNativeCastWindow = mCastWindowManager.launchCastWindow(url, new
ActivityContentVideoViewEmbedder(this));

Then, in cast_window_manager.cc:

jlong LaunchCastWindow(JNIEnv* env,
                       const JavaParamRef<jclass>& clazz,
                       const JavaParamRef<jstring>& jurl,
                       const JavaParamRef<jobject>&
j_content_video_view_embedder) {
  GURL url(base::android::ConvertJavaStringToUTF8(env, jurl));
  return reinterpret_cast<jlong>(
      CastWindowAndroid::CreateNewWindow(
          CastBrowserProcess::GetInstance()->browser_context(),
          url, j_content_video_view_embedder));
}


And in cast_window_android.cc:

// static
CastWindowAndroid* CastWindowAndroid::CreateNewWindow(
    content::BrowserContext* browser_context,
    const GURL& url,
    const JavaParamRef<jobject>& j_content_video_view_embedder) {
  CastWindowAndroid* window_android = new CastWindowAndroid(browser_context,
j_content_video_view_embedder);
  window_android->Initialize();

  if (!url.is_empty())
    window_android->LoadURL(url);

  content::RenderWidgetHostView* rwhv =
      window_android->web_contents_->GetRenderWidgetHostView();
  if (rwhv) {
    rwhv->SetBackgroundColor(SK_ColorBLACK);
  }

  return window_android;
}

CastWindowAndroid::CastWindowAndroid(
    content::BrowserContext* browser_context, 
    const JavaParamRef<jobject>& j_content_video_view_embedder)
    : browser_context_(browser_context),
      content_window_(new CastContentWindow),
      j_content_video_view_embedder_(j_content_video_view_embedder),
      weak_factory_(this) {
}
...
base::android::ScopedJavaLocalRef<jobject>
CastWindowAndroid::GetContentVideoViewEmbedder() {
  return j_content_video_view_embedder_;
}

And of course, create a private member for j_content_video_view_embedder_ in
cast_window_android.h.

(I'm not 100% sure it's right to use JavaParamRef as I did above, but it looks
to me like the lifetimes of these objects fit together so this works. I might be
missing some fundamental reason we can't do it this way, of course.)

[Jinsuk Kim, 2016/09/28 03:58:52]

While appreciating your suggestion, I tried a simpler approach not holding a
reference to ContentVideoViewEmbedder after verifying that
CastWindow{Manager|Android} are inflated and associated with the activity
context of CastShellActivity. Please review it and see if it looks okay.

+                    new ActivityContentVideoViewEmbedder(CastShellActivity.this));

[Simeon, 2016/09/28 00:24:52]

Now that this line isn't in an anonymous class, we can use "this" instead of
"CastShellActivity.this"

[Jinsuk Kim, 2016/09/28 03:58:52]

Removed.

+        }
     }
 
     @Override