Add support for closing Android Shell instances from Java.

Add support for closing Android Shell instances from Java.

This also fixes an issue where the java side would be deleted when
calling launchShell, but the native counter part was not being cleaned
up.

BUG=330902
TBR=avi@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244022

[Ted C, 2013-12-27 22:40:27]

PTAL

[cjhopman, 2013-12-30 17:29:48]

lgtm

[commit-bot: I haz the power, 2013-12-30 18:34:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedchoc@chromium.org/117403003/1

[commit-bot: I haz the power, 2013-12-30 18:50:38]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Ted C, 2013-12-30 18:52:01]

+pfeldman for OWNERS on shell_android.cc

[pfeldman, 2014-01-08 10:50:36]

rslgtm

[commit-bot: I haz the power, 2014-01-08 22:38:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedchoc@chromium.org/117403003/200001

[Ted C, 2014-01-09 01:04:28]

On 2014/01/08 22:38:21, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/tedchoc%40chromium.org/117403003/200001

@cjhopman, can you take another look at this as I switch to allow full closing
           of Shells from java per the discussion on chromium-dev

[cjhopman, 2014-01-09 02:12:44]

https://codereview.chromium.org/117403003/diff/460001/content/shell/browser/s...
File content/shell/browser/shell_android.cc (right):

https://codereview.chromium.org/117403003/diff/460001/content/shell/browser/s...
content/shell/browser/shell_android.cc:100: void CloseShell(JNIEnv* env, jclass
clazz, jlong nativeShell) {
This function should be generated by the jni generator. Looks like content_shell
jni doesn't yet use longs for pointers so that's why this isn't autogenerating.

[Ted C, 2014-01-09 02:46:31]

https://codereview.chromium.org/117403003/diff/460001/content/shell/browser/s...
File content/shell/browser/shell_android.cc (right):

https://codereview.chromium.org/117403003/diff/460001/content/shell/browser/s...
content/shell/browser/shell_android.cc:100: void CloseShell(JNIEnv* env, jclass
clazz, jlong nativeShell) {
On 2014/01/09 02:12:44, cjhopman wrote:
> This function should be generated by the jni generator. Looks like
content_shell
> jni doesn't yet use longs for pointers so that's why this isn't
autogenerating.

Oh Chris, how you got my hopes and dashed them so well.

Sadly, the generator will convert the Java side of:
static native void nativeClose(long nativeShell)

to:
static void Close(JNIEnv* env, jobject jcaller,
    jlong nativeShell) {
  Shell* native = reinterpret_cast<Shell*>(nativeShell);
  CHECK_NATIVE_PTR(env, jcaller, native, "Close");
  return native->Close(env, jcaller);
}

It's so close, but would need it to drop the env and jcaller params and allow
it to directly call non-java aware native functions (which would be amazing
BTW).  Since I don't want to pollute the cross platform shell.h with a bunch
of JNI bridging cruft, I would like to keep it isolated to this file and keep
it much like it was.

[cjhopman, 2014-01-09 02:48:58]

On 2014/01/09 02:46:31, Ted C wrote:
>
https://codereview.chromium.org/117403003/diff/460001/content/shell/browser/s...
> File content/shell/browser/shell_android.cc (right):
> 
>
https://codereview.chromium.org/117403003/diff/460001/content/shell/browser/s...
> content/shell/browser/shell_android.cc:100: void CloseShell(JNIEnv* env,
jclass
> clazz, jlong nativeShell) {
> On 2014/01/09 02:12:44, cjhopman wrote:
> > This function should be generated by the jni generator. Looks like
> content_shell
> > jni doesn't yet use longs for pointers so that's why this isn't
> autogenerating.
> 
> Oh Chris, how you got my hopes and dashed them so well.
> 
> Sadly, the generator will convert the Java side of:
> static native void nativeClose(long nativeShell)
> 
> to:
> static void Close(JNIEnv* env, jobject jcaller,
>     jlong nativeShell) {
>   Shell* native = reinterpret_cast<Shell*>(nativeShell);
>   CHECK_NATIVE_PTR(env, jcaller, native, "Close");
>   return native->Close(env, jcaller);
> }
> 
> It's so close, but would need it to drop the env and jcaller params and allow
> it to directly call non-java aware native functions (which would be amazing
> BTW).  Since I don't want to pollute the cross platform shell.h with a bunch
> of JNI bridging cruft, I would like to keep it isolated to this file and keep
> it much like it was.

That's sad... lgtm then.

[commit-bot: I haz the power, 2014-01-09 02:55:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedchoc@chromium.org/117403003/620001

[commit-bot: I haz the power, 2014-01-09 02:55:13]

Failed to apply patch for
content/shell/android/java/src/org/chromium/content_shell/ShellManager.java:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
content/shell/android/java/src/org/chromium/content_shell/ShellManager.java
  Hunk #2 FAILED at 83.
  Hunk #3 succeeded at 121 (offset 9 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
content/shell/android/java/src/org/chromium/content_shell/ShellManager.java.rej

Patch:      
content/shell/android/java/src/org/chromium/content_shell/ShellManager.java
Index:
content/shell/android/java/src/org/chromium/content_shell/ShellManager.java
diff --git
a/content/shell/android/java/src/org/chromium/content_shell/ShellManager.java
b/content/shell/android/java/src/org/chromium/content_shell/ShellManager.java
index
aad119716bb037582d6d59c34c68cba40865a031..a491294d19f5e1b33db98d556ded948eaf53140e
100644
---
a/content/shell/android/java/src/org/chromium/content_shell/ShellManager.java
+++
b/content/shell/android/java/src/org/chromium/content_shell/ShellManager.java
@@ -11,6 +11,7 @@ import android.widget.FrameLayout;
 
 import org.chromium.base.CalledByNative;
 import org.chromium.base.JNINamespace;
+import org.chromium.base.ThreadUtils;
 import org.chromium.content.browser.ContentView;
 import org.chromium.content.browser.ContentViewRenderView;
 import org.chromium.ui.base.WindowAndroid;
@@ -82,19 +83,20 @@ public class ShellManager extends FrameLayout {
      * @param url The URL the shell should load upon creation.
      */
     public void launchShell(String url) {
+        ThreadUtils.assertOnUiThread();
         nativeLaunchShell(url);
     }
 
     @SuppressWarnings("unused")
     @CalledByNative
-    private Object createShell() {
+    private Object createShell(long nativeShellPtr) {
         assert mContentViewRenderView != null;
         LayoutInflater inflater =
                 (LayoutInflater)
getContext().getSystemService(Context.LAYOUT_INFLATER_SERVICE);
         Shell shellView = (Shell) inflater.inflate(R.layout.shell_view, null);
-        shellView.setWindow(mWindow);
+        shellView.initialize(nativeShellPtr, mWindow);
 
-        if (mActiveShell != null) closeShell(mActiveShell);
+        if (mActiveShell != null) mActiveShell.close();
 
         shellView.setContentViewRenderView(mContentViewRenderView);
         addView(shellView, new FrameLayout.LayoutParams(
@@ -111,12 +113,11 @@ public class ShellManager extends FrameLayout {
 
     @SuppressWarnings("unused")
     @CalledByNative
-    private void closeShell(Shell shellView) {
+    private void removeShell(Shell shellView) {
         if (shellView == mActiveShell) mActiveShell = null;
         ContentView contentView = shellView.getContentView();
         if (contentView != null) contentView.onHide();
         shellView.setContentViewRenderView(null);
-        shellView.setWindow(null);
         removeView(shellView);
     }

[commit-bot: I haz the power, 2014-01-09 18:32:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedchoc@chromium.org/117403003/700001

[commit-bot: I haz the power, 2014-01-09 22:15:44]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-09 22:29:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedchoc@chromium.org/117403003/700001

[commit-bot: I haz the power, 2014-01-10 00:04:03]

Change committed as 244022