Switch Android code to use ExecuteJavascriptInWebFrameCallbackResult.

BUG=168169
TEST=should all pass


Review URL: https://chromiumcodereview.appspot.com/11788005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@175489 0039d316-1c4b-4281-b951-d872f2087c98

android_webview/javatests/src/org/chromium/android_webview/test/TestAwContentsClient.java

@@ -72,12 +72,6 @@ class TestAwContentsClient extends NullContentsClient {
         mOnReceivedErrorHelper.notifyCalled(errorCode, description, failingUrl);
     }
 
-    @Override
-    public void onEvaluateJavaScriptResult(int id, String jsonResult) {
-        super.onEvaluateJavaScriptResult(id, jsonResult);
-        mOnEvaluateJavaScriptResultHelper.notifyCalled(id, jsonResult);
-    }
-
     @Override
     public boolean onConsoleMessage(ConsoleMessage consoleMessage) {
         mAddMessageToConsoleHelper.setLevel(consoleMessage.messageLevel().ordinal());

android_webview/javatests/src/org/chromium/android_webview/test/util/JSUtils.java

@@ -51,7 +51,8 @@ public class JSUtils {
                     "var evObj = document.createEvent('Events'); " +
                     "evObj.initEvent('click', true, false); " +
                     "document.getElementById('" + linkId + "').dispatchEvent(evObj);" +
-                    "console.log('element with id [" + linkId + "] clicked');");
+                    "console.log('element with id [" + linkId + "] clicked');",
+                    null);
             }
         });
     }

build/android/findbugs_filter/findbugs_exclude.xml

@@ -19,4 +19,10 @@ In particular, ~ at the start of a string means it's a regex.
   </Match>
   <!-- Ignore "reliance on default String encoding" warnings, as we're not multi-platform -->
   <Bug pattern="DM_DEFAULT_ENCODING" />
+  <!-- Disabled due to impossible false positives found in -->
+  <!-- https://codereview.chromium.org/11788005/ (look in comments) -->
+  <Match>
+    <Method name="handleMessage" params="android.os.Message" returns="void" />
+    <Bug pattern="BC_IMPOSSIBLE_CAST" />
+  </Match>
 </FindBugsFilter>

content/browser/android/content_view_core_impl.cc

@@ -11,6 +11,7 @@
 #include "base/json/json_writer.h"
 #include "base/logging.h"
 #include "base/utf_string_conversions.h"
+#include "base/values.h"
 #include "cc/layer.h"
 #include "content/browser/android/interstitial_page_delegate_android.h"
 #include "content/browser/android/load_url_params.h"
@@ -174,10 +175,6 @@ ContentViewCoreImpl::ContentViewCoreImpl(JNIEnv* env, jobject obj,
     dpi_scale_ = device_info->GetDPIScale();
   }
 
-  notification_registrar_.Add(this,
-                              NOTIFICATION_EXECUTE_JAVASCRIPT_RESULT,
-                              NotificationService::AllSources());
-
   // Currently, the only use case we have for overriding a user agent involves
   // spoofing a desktop Linux user agent for "Request desktop site".
   // Automatically set it for all WebContents so that it is available when a
@@ -272,25 +269,6 @@ void ContentViewCoreImpl::Observe(int type,
       }
       break;
     }
-    case NOTIFICATION_EXECUTE_JAVASCRIPT_RESULT: {
-      if (!web_contents_ || Source<RenderViewHost>(source).ptr() !=
-          web_contents_->GetRenderViewHost()) {
-        return;
-      }
-
-      JNIEnv* env = base::android::AttachCurrentThread();
-      std::pair<int, Value*>* result_pair =
-          Details<std::pair<int, Value*> >(details).ptr();
-      std::string json;
-      base::JSONWriter::Write(result_pair->second, &json);
-      ScopedJavaLocalRef<jstring> j_json = ConvertUTF8ToJavaString(env, json);
-      ScopedJavaLocalRef<jobject> j_obj = java_ref_.get(env);
-      if (!j_obj.is_null()) {
-        Java_ContentViewCore_onEvaluateJavaScriptResult(env, j_obj.obj(),
-            static_cast<jint>(result_pair->first), j_json.obj());
-      }
-      break;
-    }
   }
 }
 
@@ -1241,15 +1219,46 @@ void ContentViewCoreImpl::UndoScrollFocusedEditableNodeIntoView(
       new ViewMsg_UndoScrollFocusedEditableNodeIntoView(host->GetRoutingID()));
 }
 
-jint ContentViewCoreImpl::EvaluateJavaScript(JNIEnv* env,
+namespace {
+void JavaScriptResultCallback(ScopedJavaGlobalRef<jobject>* message,
+                              const base::Value* result) {
+  // |message| is passed as base::Owned, so it will automatically be deleted
+  // when the callback goes out of scope.
+  JNIEnv* env = base::android::AttachCurrentThread();
+  std::string json;
+  base::JSONWriter::Write(result, &json);
+  ScopedJavaLocalRef<jstring> j_json = ConvertUTF8ToJavaString(env, json);
+  Java_ContentViewCore_onEvaluateJavaScriptResult(env,
+                                                  j_json.obj(),
+                                                  message->obj());
+}
+}  // namespace
+
+void ContentViewCoreImpl::EvaluateJavaScript(JNIEnv* env,
                                              jobject obj,
-                                             jstring script) {
+                                             jstring script,
+                                             jobject message) {
   RenderViewHost* host = web_contents_->GetRenderViewHost();
   DCHECK(host);
 
-  string16 script_utf16 = ConvertJavaStringToUTF16(env, script);
-  return host->ExecuteJavascriptInWebFrameNotifyResult(string16(),
-                                                       script_utf16);
+  if (!message) {
+    // No callback requested.
+    host->ExecuteJavascriptInWebFrame(string16(),  // frame_xpath
+                                      ConvertJavaStringToUTF16(env, script));
+    return;
+  }
+
+  // Secure the message in a scoped object and give ownership of it to the
+  // callback.
+  ScopedJavaGlobalRef<jobject>* j_message = new ScopedJavaGlobalRef<jobject>();
+  j_message->Reset(env, message);
+  content::RenderViewHost::JavascriptResultCallback callback =
+      base::Bind(&JavaScriptResultCallback, base::Owned(j_message));
+
+  host->ExecuteJavascriptInWebFrameCallbackResult(
+      string16(),  // frame_xpath
+      ConvertJavaStringToUTF16(env, script),
+      callback);
 }
 
 bool ContentViewCoreImpl::GetUseDesktopUserAgent(
@@ -1325,14 +1334,6 @@ jint Init(JNIEnv* env, jobject obj,
   return reinterpret_cast<jint>(view);
 }
 
-jint EvaluateJavaScript(JNIEnv* env, jobject obj, jstring script) {
-  ContentViewCoreImpl* view = static_cast<ContentViewCoreImpl*>(
-      ContentViewCore::GetNativeContentViewCore(env, obj));
-  DCHECK(view);
-
-  return view->EvaluateJavaScript(env, obj, script);
-}
-
 bool RegisterContentViewCore(JNIEnv* env) {
   if (!base::android::HasClass(env, kContentViewCoreClassPath)) {
     DLOG(ERROR) << "Unable to find class ContentViewCore!";

content/browser/android/content_view_core_impl.h

@@ -157,7 +157,10 @@ class ContentViewCoreImpl : public ContentViewCore,
   void ContinuePendingReload(JNIEnv* env, jobject obj);
   jboolean NeedsReload(JNIEnv* env, jobject obj);
   void ClearHistory(JNIEnv* env, jobject obj);
-  jint EvaluateJavaScript(JNIEnv* env, jobject obj, jstring script);
+  void EvaluateJavaScript(JNIEnv* env,
+                          jobject obj,
+                          jstring script,
+                          jobject message);
   int GetNativeImeAdapter(JNIEnv* env, jobject obj);
   void SetFocus(JNIEnv* env, jobject obj, jboolean focused);
   void ScrollFocusedEditableNodeIntoView(JNIEnv* env, jobject obj);

content/public/android/java/src/org/chromium/content/browser/ContentView.java

@@ -420,17 +420,11 @@ public class ContentView extends FrameLayout implements ContentViewCore.Internal
 
     /**
      * Injects the passed JavaScript code in the current page and evaluates it.
-     * Once evaluated, an asynchronous call to
-     * ContentViewClient.onJavaScriptEvaluationResult is made. Used in automation
-     * tests.
      *
-     * @return an id that is passed along in the asynchronous onJavaScriptEvaluationResult callback
      * @throws IllegalStateException If the ContentView has been destroyed.
-     *
-     * TODO(nileshagrawal): Remove this method from the public interface.
      */
-    public int evaluateJavaScript(String script) throws IllegalStateException {
-        return mContentViewCore.evaluateJavaScript(script);
+    public void evaluateJavaScript(String script) throws IllegalStateException {
+        mContentViewCore.evaluateJavaScript(script, null);
     }
 
     /**

content/public/android/java/src/org/chromium/content/browser/ContentViewClient.java

@@ -92,15 +92,6 @@ public class ContentViewClient {
     public void onImeEvent() {
     }
 
-    /**
-     * A callback invoked after the JavaScript code passed to evaluateJavaScript
-     * has finished execution.
-     * Used in automation tests.
-     * @hide
-     */
-    public void onEvaluateJavaScriptResult(int id, String jsonResult) {
-    }
-
     // TODO (dtrainor): Should expose getScrollX/Y from ContentView or make
     // computeHorizontalScrollOffset()/computeVerticalScrollOffset() public.
     /**

content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java

@@ -18,6 +18,7 @@ import android.graphics.Rect;
 import android.os.Build;
 import android.os.Bundle;
 import android.os.Handler;
+import android.os.Message;
 import android.os.ResultReceiver;
 import android.text.Editable;
 import android.util.Log;
@@ -1049,17 +1050,19 @@ public class ContentViewCore implements MotionEventDelegate, NavigationClient {
     }
 
     /**
-     * Injects the passed JavaScript code in the current page and evaluates it.
-     * Once evaluated, an asynchronous call to
-     * ContentViewClient.onJavaScriptEvaluationResult is made. Used in automation
-     * tests.
+     * Injects the passed Javascript code in the current page and evaluates it.
+     * If a result is required, pass in a Message to be sent to its target.
+     * Used in automation tests.
      *
-     * @return an id that is passed along in the asynchronous onJavaScriptEvaluationResult callback
+     * @param script The Javascript to execute.
+     * @param message The message to be fired off when a result is ready. The script's
+     *                result will be json encoded in the Message's {@link Message#obj} field.
+     *                If no result is required, pass null.
      * @throws IllegalStateException If the ContentView has been destroyed.
      */
-    public int evaluateJavaScript(String script) throws IllegalStateException {
+    public void evaluateJavaScript(String script, Message message) throws IllegalStateException {
         checkIsAlive();
-        return nativeEvaluateJavaScript(script);
+        nativeEvaluateJavaScript(mNativeContentViewCore, script, message);
     }
 
     /**
@@ -1990,8 +1993,9 @@ public class ContentViewCore implements MotionEventDelegate, NavigationClient {
 
     @SuppressWarnings("unused")
     @CalledByNative
-    private void onEvaluateJavaScriptResult(int id, String jsonResult) {
-        getContentViewClient().onEvaluateJavaScriptResult(id, jsonResult);
+    private static void onEvaluateJavaScriptResult(String jsonResult, Message message) {
+        message.obj = jsonResult;
+        message.sendToTarget();
     }
 
     @SuppressWarnings("unused")
@@ -2502,7 +2506,8 @@ public class ContentViewCore implements MotionEventDelegate, NavigationClient {
 
     private native void nativeClearHistory(int nativeContentViewCoreImpl);
 
-    private native int nativeEvaluateJavaScript(String script);
+    private native void nativeEvaluateJavaScript(int nativeContentViewCoreImpl,
+            String script, Message message);
 
     private native int nativeGetNativeImeAdapter(int nativeContentViewCoreImpl);
 

content/public/android/java/src/org/chromium/content/browser/accessibility/AccessibilityInjector.java

@@ -132,7 +132,7 @@ public class AccessibilityInjector extends WebContentsObserverAndroid {
 
                 if (onDeviceScriptInjectionEnabled && js != null && mContentViewCore.isAlive()) {
                     addOrRemoveAccessibilityApisIfNecessary();
-                    mContentViewCore.evaluateJavaScript(js);
+                    mContentViewCore.evaluateJavaScript(js, null);
                     mInjectedScriptEnabled = true;
                     mScriptInjected = true;
                 }
@@ -183,7 +183,7 @@ public class AccessibilityInjector extends WebContentsObserverAndroid {
         if (mContentViewCore.isAlive()) {
             String js = String.format(TOGGLE_CHROME_VOX_JAVASCRIPT, Boolean.toString(
                     mInjectedScriptEnabled));
-            mContentViewCore.evaluateJavaScript(js);
+            mContentViewCore.evaluateJavaScript(js, null);
 
             if (!mInjectedScriptEnabled) {
                 // Stop any TTS/Vibration right now.

content/public/android/java/src/org/chromium/content/browser/accessibility/JellyBeanAccessibilityInjector.java

@@ -172,7 +172,7 @@ class JellyBeanAccessibilityInjector extends AccessibilityInjector {
             final int resultId = mResultIdCounter.getAndIncrement();
             final String js = String.format(JAVASCRIPT_ACTION_TEMPLATE, mInterfaceName, resultId,
                     code);
-            contentView.evaluateJavaScript(js);
+            contentView.evaluateJavaScript(js, null);
 
             return getResultAndClear(resultId);
         }
@@ -228,7 +228,7 @@ class JellyBeanAccessibilityInjector extends AccessibilityInjector {
          * request to a waiting (or potentially timed out) thread.
          *
          * @param id The result id of the request as a {@link String}.
-         * @param result The result o fa request as a {@link String}.
+         * @param result The result of a request as a {@link String}.
          */
         @JavascriptInterface
         @SuppressWarnings("unused")
@@ -249,4 +249,4 @@ class JellyBeanAccessibilityInjector extends AccessibilityInjector {
             }
         }
     }
-}
\ No newline at end of file
+}

content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java

@@ -5,6 +5,8 @@
 package org.chromium.content.browser.test.util;
 
 
+import android.os.Handler;
+import android.os.Message;
 import android.util.Log;
 
 import org.chromium.content.browser.ContentView;
@@ -81,8 +83,7 @@ public class TestCallbackHelperContainer {
     }
 
     public static class OnEvaluateJavaScriptResultHelper extends CallbackHelper {
-        private volatile Integer mRequestId;
-        private volatile Integer mId;
+        private boolean mRequestSent;
         private String mJsonResult;
 
         /**
@@ -91,14 +92,24 @@ public class TestCallbackHelperContainer {
          * @param code A JavaScript code to be evaluated.
          */
         public void evaluateJavaScript(ContentViewCore contentViewCore, String code) {
-            setRequestId(contentViewCore.evaluateJavaScript(code));
+            assert !mRequestSent;
+            Message message = Message.obtain(new Handler() {
+                @Override
+                public void handleMessage(Message msg) {
+                    String json = (String)msg.obj;
+                    notifyCalled(json);
+                }
+            });
+            contentViewCore.evaluateJavaScript(code, message);
+            mRequestSent = true;
+            mJsonResult = null;
         }
 
         /**
          * Returns true if the evaluation started by evaluateJavaScript() has completed.
          */
         public boolean hasValue() {
-            return mId != null;
+            return mJsonResult != null;
         }
 
         /**
@@ -109,7 +120,8 @@ public class TestCallbackHelperContainer {
         public String getJsonResultAndClear() {
             assert hasValue();
             String result = mJsonResult;
-            setRequestId(null);
+            mRequestSent = false;
+            mJsonResult = null;
             return result;
         }
 
@@ -141,21 +153,8 @@ public class TestCallbackHelperContainer {
             return hasValue();
         }
 
-        private void setRequestId(Integer requestId) {
-            mRequestId = requestId;
-            mId = null;
-            mJsonResult = null;
-        }
-
-        public void notifyCalled(int id, String jsonResult) {
-            if (mRequestId == null) {
-                Log.w("TestCallbackHelperContainer",
-                        "Received JavaScript eval result when request id was not set");
-                return;
-            }
-            if (id != mRequestId.intValue()) return;
-            assert mId == null;
-            mId = Integer.valueOf(id);
+        public void notifyCalled(String jsonResult) {
+            assert !hasValue();
             mJsonResult = jsonResult;
             notifyCalled();
         }

content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestContentViewClient.java

@@ -35,16 +35,11 @@ public class TestContentViewClient extends ContentViewClient {
     }
 
     /**
-     * ATTENTION!: When overriding the following methods, be sure to call
-     * the corresponding methods in the super class. Otherwise
+     * ATTENTION!: When overriding the following method, be sure to call
+     * the corresponding method in the super class. Otherwise
      * {@link CallbackHelper#waitForCallback()} methods will
      * stop working!
      */
-    @Override
-    public void onEvaluateJavaScriptResult(int id, String jsonResult) {
-        super.onEvaluateJavaScriptResult(id, jsonResult);
-        mOnEvaluateJavaScriptResultHelper.notifyCalled(id, jsonResult);
-    }
 
     @Override
     public void onStartContentIntent(Context context, String contentUrl) {