[WebLayer] Change Tab#executeScript() to return String directly

Clients wish to be able to process the result in ways other than it
becoming a JSONObject.

For convenience, we leave the test method that wraps this public API
returning a JSONObject; the wrapping of the String into a JSONObject
now occurs in that test API rather than in the public API.

Bug: 1284580
Change-Id: Idf4e8c3e8cfe624ab047cf018316168bf51bb5b9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3370223
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/main@{#956422}

weblayer/browser/android/javatests/src/org/chromium/weblayer/test/ExecuteScriptTest.java

@@ -14,7 +14,6 @@ import org.junit.runner.RunWith;
 
 import org.chromium.base.test.util.UrlUtils;
 import org.chromium.content_public.browser.test.util.TestThreadUtils;
-import org.chromium.weblayer.Tab;
 import org.chromium.weblayer.shell.InstrumentationActivity;
 
 /**
@@ -35,7 +34,8 @@ public class ExecuteScriptTest {
         InstrumentationActivity activity = mActivityTestRule.launchShellWithUrl(DATA_URL);
         JSONObject result = mActivityTestRule.executeScriptSync(
                 "document.body.innerHTML", true /* useSeparateIsolate */);
-        Assert.assertEquals(result.getString(Tab.SCRIPT_RESULT_KEY), "foo");
+        Assert.assertEquals(
+                result.getString(InstrumentationActivityTestRule.SCRIPT_RESULT_KEY), "foo");
     }
 
     @Test
@@ -44,7 +44,7 @@ public class ExecuteScriptTest {
         InstrumentationActivity activity = mActivityTestRule.launchShellWithUrl(DATA_URL);
         JSONObject result =
                 mActivityTestRule.executeScriptSync("bar", true /* useSeparateIsolate */);
-        Assert.assertTrue(result.isNull(Tab.SCRIPT_RESULT_KEY));
+        Assert.assertTrue(result.isNull(InstrumentationActivityTestRule.SCRIPT_RESULT_KEY));
     }
 
     @Test
@@ -53,7 +53,7 @@ public class ExecuteScriptTest {
         InstrumentationActivity activity = mActivityTestRule.launchShellWithUrl(DATA_URL);
         JSONObject result =
                 mActivityTestRule.executeScriptSync("bar", false /* useSeparateIsolate */);
-        Assert.assertEquals(result.getInt(Tab.SCRIPT_RESULT_KEY), 10);
+        Assert.assertEquals(result.getInt(InstrumentationActivityTestRule.SCRIPT_RESULT_KEY), 10);
     }
 
     @Test
@@ -63,7 +63,7 @@ public class ExecuteScriptTest {
         mActivityTestRule.executeScriptSync("var foo = 20;", true /* useSeparateIsolate */);
         JSONObject result =
                 mActivityTestRule.executeScriptSync("foo", true /* useSeparateIsolate */);
-        Assert.assertEquals(result.getInt(Tab.SCRIPT_RESULT_KEY), 20);
+        Assert.assertEquals(result.getInt(InstrumentationActivityTestRule.SCRIPT_RESULT_KEY), 20);
     }
 
     @Test
@@ -76,7 +76,7 @@ public class ExecuteScriptTest {
         mActivityTestRule.navigateAndWait(newUrl);
         JSONObject result =
                 mActivityTestRule.executeScriptSync("foo", true /* useSeparateIsolate */);
-        Assert.assertTrue(result.isNull(Tab.SCRIPT_RESULT_KEY));
+        Assert.assertTrue(result.isNull(InstrumentationActivityTestRule.SCRIPT_RESULT_KEY));
     }
 
     @Test

weblayer/browser/android/javatests/src/org/chromium/weblayer/test/GeolocationTest.java

@@ -37,7 +37,6 @@ import org.chromium.base.test.util.MinAndroidSdkLevel;
 import org.chromium.content_public.browser.test.util.TestThreadUtils;
 import org.chromium.net.test.util.TestWebServer;
 import org.chromium.weblayer.Browser;
-import org.chromium.weblayer.Tab;
 import org.chromium.weblayer.TestWebLayer;
 import org.chromium.weblayer.shell.InstrumentationActivity;
 
@@ -242,8 +241,9 @@ public final class GeolocationTest {
                 false);
         CriteriaHelper.pollInstrumentationThread(() -> {
             try {
-                String result = mActivityTestRule.executeScriptSync("queryResult || ''", false)
-                                        .getString(Tab.SCRIPT_RESULT_KEY);
+                String result =
+                        mActivityTestRule.executeScriptSync("queryResult || ''", false)
+                                .getString(InstrumentationActivityTestRule.SCRIPT_RESULT_KEY);
                 Criteria.checkThat(result, Matchers.not(""));
             } catch (JSONException ex) {
                 throw new CriteriaNotSatisfiedException(ex);
@@ -251,7 +251,7 @@ public final class GeolocationTest {
         });
         Assert.assertEquals("prompt",
                 mActivityTestRule.executeScriptSync("queryResult", false)
-                        .getString(Tab.SCRIPT_RESULT_KEY));
+                        .getString(InstrumentationActivityTestRule.SCRIPT_RESULT_KEY));
         CriteriaHelper.pollInstrumentationThread(
                 () -> { return mTestWebLayer.isPermissionDialogShown(); });
     }
@@ -277,7 +277,7 @@ public final class GeolocationTest {
         int result = -1;
         try {
             result = mActivityTestRule.executeScriptSync(variableName, false)
-                             .getInt(Tab.SCRIPT_RESULT_KEY);
+                             .getInt(InstrumentationActivityTestRule.SCRIPT_RESULT_KEY);
         } catch (Exception e) {
             Assert.fail("Unable to get " + variableName);
         }

weblayer/browser/android/javatests/src/org/chromium/weblayer/test/InstrumentationActivityTestRule.java

@@ -43,17 +43,20 @@ import java.util.concurrent.TimeoutException;
  */
 public class InstrumentationActivityTestRule
         extends WebLayerActivityTestRule<InstrumentationActivity> {
+    /** The top level key of the JSON object returned by executeScriptSync(). */
+    public static final String SCRIPT_RESULT_KEY = "result";
+
     @Rule
     private EmbeddedTestServerRule mTestServerRule = new EmbeddedTestServerRule();
 
-    private static final class JSONCallbackHelper extends CallbackHelper {
-        private JSONObject mResult;
+    private static final class StringCallbackHelper extends CallbackHelper {
+        private String mResult;
 
-        public JSONObject getResult() {
+        public String getResult() {
             return mResult;
         }
 
-        public void notifyCalled(JSONObject result) {
+        public void notifyCalled(String result) {
             mResult = result;
             notifyCalled();
         }
@@ -149,22 +152,31 @@ public class InstrumentationActivityTestRule
     }
 
     /**
-     * Executes the script passed in and waits for the result.
+     * Executes the script passed in and waits for the result. Wraps that result in a JSONObject for
+     * convenience of callers that want to process that result as a type other than String.
      */
     public JSONObject executeScriptSync(String script, boolean useSeparateIsolate, Tab tab) {
-        JSONCallbackHelper callbackHelper = new JSONCallbackHelper();
+        StringCallbackHelper callbackHelper = new StringCallbackHelper();
         int count = callbackHelper.getCallCount();
         TestThreadUtils.runOnUiThreadBlocking(() -> {
             Tab scriptTab = tab == null ? getActivity().getBrowser().getActiveTab() : tab;
             scriptTab.executeScript(script, useSeparateIsolate,
-                    (JSONObject result) -> { callbackHelper.notifyCalled(result); });
+                    (String result) -> { callbackHelper.notifyCalled(result); });
         });
         try {
             callbackHelper.waitForCallback(count);
         } catch (TimeoutException e) {
             throw new RuntimeException(e);
         }
-        return callbackHelper.getResult();
+        JSONObject resultAsJSONObject;
+        try {
+            resultAsJSONObject = new JSONObject(
+                    "{\"" + SCRIPT_RESULT_KEY + "\":" + callbackHelper.getResult() + "}");
+        } catch (JSONException e) {
+            // This should never happen since the result should be well formed.
+            throw new RuntimeException(e);
+        }
+        return resultAsJSONObject;
     }
 
     public JSONObject executeScriptSync(String script, boolean useSeparateIsolate) {
@@ -174,7 +186,7 @@ public class InstrumentationActivityTestRule
     public int executeScriptAndExtractInt(String script) {
         try {
             return executeScriptSync(script, true /* useSeparateIsolate */)
-                    .getInt(Tab.SCRIPT_RESULT_KEY);
+                    .getInt(SCRIPT_RESULT_KEY);
         } catch (JSONException e) {
             throw new RuntimeException(e);
         }
@@ -186,7 +198,7 @@ public class InstrumentationActivityTestRule
 
     public String executeScriptAndExtractString(String script, boolean useSeparateIsolate) {
         try {
-            return executeScriptSync(script, useSeparateIsolate).getString(Tab.SCRIPT_RESULT_KEY);
+            return executeScriptSync(script, useSeparateIsolate).getString(SCRIPT_RESULT_KEY);
         } catch (JSONException e) {
             throw new RuntimeException(e);
         }
@@ -198,7 +210,7 @@ public class InstrumentationActivityTestRule
 
     public boolean executeScriptAndExtractBoolean(String script, boolean useSeparateIsolate) {
         try {
-            return executeScriptSync(script, useSeparateIsolate).getBoolean(Tab.SCRIPT_RESULT_KEY);
+            return executeScriptSync(script, useSeparateIsolate).getBoolean(SCRIPT_RESULT_KEY);
         } catch (JSONException e) {
             throw new RuntimeException(e);
         }

weblayer/public/java/org/chromium/weblayer/Tab.java

@@ -13,9 +13,6 @@ import android.webkit.ValueCallback;
 import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
 
-import org.json.JSONException;
-import org.json.JSONObject;
-
 import org.chromium.weblayer_private.interfaces.APICallException;
 import org.chromium.weblayer_private.interfaces.IClientNavigation;
 import org.chromium.weblayer_private.interfaces.IContextMenuParams;
@@ -42,9 +39,6 @@ import java.util.Set;
  * configuring state of the tab, such as delegates and callbacks.
  */
 public class Tab {
-    /** The top level key of the JSON object returned by executeScript(). */
-    public static final String SCRIPT_RESULT_KEY = "result";
-
     // Maps from id (as returned from ITab.getId()) to Tab.
     private static final Map<Integer, Tab> sTabMap = new HashMap<Integer, Tab>();
 
@@ -233,9 +227,7 @@ public class Tab {
     }
 
     /**
-     * Executes the script, and returns the result as a JSON object to the callback if provided. The
-     * object passed to the callback will have a single key SCRIPT_RESULT_KEY which will hold the
-     * result of running the script.
+     * Executes the script, and returns the result to the callback if provided.
      * @param useSeparateIsolate If true, runs the script in a separate v8 Isolate. This uses more
      * memory, but separates the injected scrips from scripts in the page. This prevents any
      * potentially malicious interaction between first-party scripts in the page, and injected
@@ -243,24 +235,11 @@ public class Tab {
      * or you need to interact with first-party scripts.
      */
     public void executeScript(@NonNull String script, boolean useSeparateIsolate,
-            @Nullable ValueCallback<JSONObject> callback) {
+            @Nullable ValueCallback<String> callback) {
         ThreadCheck.ensureOnUiThread();
         throwIfDestroyed();
         try {
-            ValueCallback<String> stringCallback = (String result) -> {
-                if (callback == null) {
-                    return;
-                }
-
-                try {
-                    callback.onReceiveValue(
-                            new JSONObject("{\"" + SCRIPT_RESULT_KEY + "\":" + result + "}"));
-                } catch (JSONException e) {
-                    // This should never happen since the result should be well formed.
-                    throw new RuntimeException(e);
-                }
-            };
-            mImpl.executeScript(script, useSeparateIsolate, ObjectWrapper.wrap(stringCallback));
+            mImpl.executeScript(script, useSeparateIsolate, ObjectWrapper.wrap(callback));
         } catch (RemoteException e) {
             throw new APICallException(e);
         }