983811 - Add way for JS and Java code to communicate in Robocop tests

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

Some types of tests, such as IME and prompts, require communication between JS and Java.

For example, an IME test could focus an input in JS and check to see the keyboard is shown in Java; then it would enter a string in Java and check to see the same string matches input.value in JS.

A prompts test could call confirm() in JS and check to see the message is shown in Java; then it would click "OK" in Java and check to see confirm() returned the correct value in JS.

Right now we have JavascriptTest which AFAICT is suitable for Javascript-only tests, but for the above examples that require back-and-forth communication, it wouldn't work so well. I think we can add something on top of the JS harness to allow this sort of communication.

Comment 1

[:Margaret Leibovic]

Have you seen some of the roboextender tests that wesj and I wrote? It's kinda hacky, but they do allow Java/JS communication. Example:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testHomeBanner.java
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/roboextender/robocop_home_banner.html?force=1

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Yup, I'm using those tests as examples, and I'm working on something that's easily reusable by other tests.

Comment 3

[Nick Alexander :nalexander [he/him]]

JavascriptTest can be made to do some of this; the Java framework already waits for the JS framework, IIRC.  It won't be particularly clean.

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Expose JavascriptTest.MessageParser for JavascriptBridge (v1)

Making the message parser from JavascriptTest reusable.

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add JavascriptBridge (v1)

Here's part of what I have so far. The patch implements a JavascriptBridge to allow Java to communic
ate with JS. The patch also has some documentations.

So far, my goals have been,
* To have a simple API, so that test developers can easily use it in their tests.
* To have the same API on both sides (Java to JS and JS to Java), to make it easier for test develop
ers as they switch between writing Java and JS.

JavascriptBridge uses a simple ping/pong API. In the typical case, Java uses ping to call a method in JS, and JS replies by using pong to call another method back in Java; this process can be repeated as needed. Another patch will contain JavaBridge that implements the same API for the JS side.

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Move functions from robocop_testharness.js to robocop_head.js (v1)

This patch moves some utilities functions from robocop_testharness.js to robocop_head.js. So that unpriviledged tests only have to include robocop_head.js.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Fix Components for unprivileged tests (v1)

This patch makes robocop_head.js usable by unpriviledged tests.

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add JavaBridge (v1)

This patch adds JavaBridge, which is companion to JavascriptBridge.

Comment 9

[Wesley Johnston (:wesj)]

Comment on attachment 8393840 [details] [diff] [review]
Move functions from robocop_testharness.js to robocop_head.js (v1)

Review of attachment 8393840 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/tests/robocop_head.js
@@ +29,5 @@
> +
> +function sendMessageToJava(message) {
> +  let data = JSON.stringify(message);
> +  androidBridge.handleGeckoMessage(data);
> +}

Please just use Messaging.jsm so that we don't have this thing repeated everywhere in the world.

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Move functions from robocop_testharness.js to robocop_head.js (v2)

> Please just use Messaging.jsm so that we don't have this thing repeated
> everywhere in the world.

Okay, changed to using Messaging.jsm.

The reason I want to move these from robocop_testharness.js to robocop_head.js is I want to be able to use robocop_head.js from unpriviledged code in addition to priviledged code. So that an unpriviledged test HTML page can just include robocop_head.js and do testing in inlined JS. Another upcoming patch will actually make robocop_head.js usable from unpriviledged code through SpecialPowers.

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8393776 [details] [diff] [review]
Expose JavascriptTest.MessageParser for JavascriptBridge (v1)

JavascriptBridge uses JavascriptTest.MessageParser to log test messages coming from Javascript.

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8393842 [details] [diff] [review]
Fix Components for unprivileged tests (v1)

This patch makes robocop_head.js usable from unprivileged code by setting Components to its SpecialPowers version.

Comment 13

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8393776 [details] [diff] [review]
Expose JavascriptTest.MessageParser for JavascriptBridge (v1)

Review of attachment 8393776 [details] [diff] [review]:
-----------------------------------------------------------------

Break this out into its own file.  /me is secretly happy that this tiny piece of code is going to get re-used; I always hoped it would :)

Comment 14

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8393779 [details] [diff] [review]
Add JavascriptBridge (v1)

Review of attachment 8393779 [details] [diff] [review]:
-----------------------------------------------------------------

Have some unsolicited nits.  I can see this being valuable, but I'm having a hard time understanding exactly the messaging semantics you're looking for.  Also, we *haven't* done this in Gecko itself.  We could.  Why are we building it for test code when we've steered hard away from it in App code?

::: mobile/android/base/tests/helpers/GeckoHelper.java
@@ +68,5 @@
> +     * Java; then pass an object to JavaBridge in Javascript. Once a link is established,
> +     * three methods will be used for communication: ping, pong, and finish.
> +     *
> +     * General rules for communicating with JavascriptBridge and JavaBridge are,
> +     *  1) In Java code, use ping() to call a method in Javascript.

nit: these methods are oriented, but |ping| and |pong| don't tell you the orientation.  Why not bridgeMessageToJavascript and bridgeMessageToJava, or similar?

@@ +71,5 @@
> +     * General rules for communicating with JavascriptBridge and JavaBridge are,
> +     *  1) In Java code, use ping() to call a method in Javascript.
> +     *  2) In Javascript code, use pong() to call back another method in Java.
> +     *  3) Repeat the above two steps as necessary.
> +     *  4) In Java code, call finish() after the *first* ping().

I haven't read the code well enough to understand why this is necessary, but this is really complicated.  The Java code is in a position to know when the first |ping| call is -- why doesn't it track this and remove |finish| entirely?  Is |finish| really some kind of |waitFor...|?

@@ +72,5 @@
> +     *  1) In Java code, use ping() to call a method in Javascript.
> +     *  2) In Javascript code, use pong() to call back another method in Java.
> +     *  3) Repeat the above two steps as necessary.
> +     *  4) In Java code, call finish() after the *first* ping().
> +     *  5) In Javascript code, call finish() after the *last* pong().

At least |finish| is last here; but is it really some kind of |waitFor...|?

@@ +76,5 @@
> +     *  5) In Javascript code, call finish() after the *last* pong().
> +     *
> +     * For a simple example, here's a Java test class that extends JavascriptBridge.
> +     *
> +     *  class Test extends JavascriptBridge {

This would be a lot more useful if it was a real test run on TBPL.  As well, you know, as testing this functionality :)

@@ +112,5 @@
> +     * To start the test, call start(). For example, use (new Test()).start().
> +     * In this example, the program flow goes from Test.start(), to js.add(),
> +     * to Test.result(), to js.string(), then finally to Test.end().
> +     */
> +    public static class JavascriptBridge {

Definitely break this into a separate file.

@@ +125,5 @@
> +        private final JavascriptTest.MessageParser mLogParser;
> +        // Expecter of our internal Robocop event
> +        private final EventExpecter mExpecter;
> +        // Saved pong message; see processMessage() for its purpose.
> +        private JSONObject mSavedMessage;

Only one?  Why isn't this a queue?  More generally, I'm not seeing message tagging to track multiple send/request.  Is this strictly a ping/pong channel?  How do we enforce good behaviour?

@@ +226,5 @@
> +                    for (final Object arg : args) {
> +                        jsonArgs.put(convertToJSONValue(arg));
> +                    }
> +                }
> +                message.put("type", "Robocop:Status")

Make "Robocop:Status" a constant, plz.

Comment 15

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8394259 [details] [diff] [review]
Move functions from robocop_testharness.js to robocop_head.js (v2)

Review of attachment 8394259 [details] [diff] [review]:
-----------------------------------------------------------------

I can see that this won't *hurt* anything, but I'm lacking a little context on what it's *enabling*.  You've moved some things from _testharness into _head, but the test harness still loads _head, so nothing much has changed.  I'll read the final patch to understand, but I'm confused.

::: mobile/android/base/tests/robocop_head.js
@@ +23,5 @@
>  
> +/**
> + * Import sendMessageToJava to communicate with Fennec Java code.
> + */
> +Components.utils.import("resource://gre/modules/Messaging.jsm", this);

I have a gentle preference for not importing things into the scope of |this|, but follow whatever the convention in this file is.

@@ +26,5 @@
> + */
> +Components.utils.import("resource://gre/modules/Messaging.jsm", this);
> +
> +/**
> + * Output from head.js is fed, line by line, to this function.

nit: should this be robocop_head.js?  Or something more specific, like "output from the tests"?

@@ +39,5 @@
> +};
> +
> +
> +/**
> + * Start of code adapted from xpcshell head.js

I always knew copying this was going to cause divergence in future, and here it is.

Comment 16

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8393843 [details] [diff] [review]
Add JavaBridge (v1)

Review of attachment 8393843 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/tests/robocop_head.js
@@ +99,5 @@
> +  /**
> +   * Synchronously call a method in Java,
> +   * given the method name followed by a list of arguments.
> +   */
> +  obj.ping = function (method /*, ... */) {

ping and pong imply directions to me, where-as you appear to have them implying sync/async.  I'd suggest more descriptive names: |bridgeMessageToJavaAndWait| or whatever.

Comment 17

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #15)
> Comment on attachment 8394259 [details] [diff] [review]
> Move functions from robocop_testharness.js to robocop_head.js (v2)
> 
> Review of attachment 8394259 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I can see that this won't *hurt* anything, but I'm lacking a little context
> on what it's *enabling*.  You've moved some things from _testharness into
> _head, but the test harness still loads _head, so nothing much has changed. 
> I'll read the final patch to understand, but I'm confused.

I've read all the patches and I'm still confused.  There are two things that I'm not seeing the big picture of:

1) moving code between _harness and _head;
2) allowing unprivileged access to things that require privileges;

An example test (the test that tests the framework) might help me understand why this was necessary.

Comment 18

[Nick Alexander :nalexander [he/him]]

> > +                message.put("type", "Robocop:Status")
> 
> Make "Robocop:Status" a constant, plz.

In fact, why are we using "Robocop:Status" at all?  I know I included an inner type, but the messages you're sending and receiving aren't test harness status messages at all.

Comment 19

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8393842 [details] [diff] [review]
Fix Components for unprivileged tests (v1)

Review of attachment 8393842 [details] [diff] [review]:
-----------------------------------------------------------------

This does what you say it does, but I'm not sure why it's needed.  r+ assuming further discussion on the ticket.

Comment 20

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add test for JavascriptBridge/JavaBridge (v1)

Comment 21

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8394259 [details] [diff] [review]
Move functions from robocop_testharness.js to robocop_head.js (v2)

As nalexander said, we should focus on Java-JS communication for this bug. For being able to include robocop_head.js from unprivileged HTML pages, that's a separate discussion.

Comment 22

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8393776 [details] [diff] [review]
Expose JavascriptTest.MessageParser for JavascriptBridge (v1)

Moved MessageParser work to bug 988553.

Comment 23

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add getTest to UITestContext (v1)

The JS-Java communication helper needs access to the test from the helper.

Comment 24

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add JavascriptBridge (v2)

Comment 25

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add static JavascriptBridge instance in GeckoHelper (v1)

Comment 26

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add JavaBridge (v2)

Comment 27

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add test for JavascriptBridge/JavaBridge (v2)

Comment 28

[:Margaret Leibovic]

Comment on attachment 8398010 [details] [diff] [review]
Add JavaBridge (v2)

Review of attachment 8398010 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/tests/robocop_head.js
@@ +1151,5 @@
> + */
> +
> +function JavaBridge(obj) {
> +
> +  const self = this;

Drive-by: I don't like this. It looks like you're only doing this to be able to reference the right `this` inside observe, but you could do that with a bind, or my suggestion down below.

@@ +1174,5 @@
> +      }
> +    },
> +  };
> +  self._obsSvc = Components.classes["@mozilla.org/observer-service;1"]
> +               .getService(Components.interfaces.nsIObserverService);

You could use Services.obs if you import the Services module. It looks like this file doesn't actually use Services at all... not sure if that's a historical artifact or something that's necessary for some reason.

@@ +1175,5 @@
> +    },
> +  };
> +  self._obsSvc = Components.classes["@mozilla.org/observer-service;1"]
> +               .getService(Components.interfaces.nsIObserverService);
> +  self._obsSvc.addObserver(self._observer, self._EVENT_TYPE, false);

By convention, we usually just declare an observe method directly on the object, so this would instead be:
...addObserver(this, EVENT_TYPE, false);

So you could make an observe method on the prototype. That would also solve your `this` issue up above.

Comment 29

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8398012 [details] [diff] [review]
Add test for JavascriptBridge/JavaBridge (v2)

Review of attachment 8398012 [details] [diff] [review]:
-----------------------------------------------------------------

Drive-by while checking out the API on a high level.

::: mobile/android/base/tests/testJavascriptBridge.java
@@ +5,5 @@
> +
> +import org.mozilla.gecko.tests.helpers.*;
> +
> +/**
> + * Tests the proper operation of JavascriptBridge

nit: Make a comment that we're testing test code.

@@ +9,5 @@
> + * Tests the proper operation of JavascriptBridge
> + */
> +public class testJavascriptBridge extends UITest {
> +
> +    private static final String TEST_TITLE = "testJavascriptBridge";

nit: Unused.

@@ +10,5 @@
> + */
> +public class testJavascriptBridge extends UITest {
> +
> +    private static final String TEST_TITLE = "testJavascriptBridge";
> +    private static final String TEST_JS = "testJavascriptBridge.js";

Robocop pages titles are also in StringHelper.

::: mobile/android/base/tests/testJavascriptBridge.js
@@ +1,1 @@
> +

nit: ws at the top of the file.

Comment 30

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8398012 [details] [diff] [review]
Add test for JavascriptBridge/JavaBridge (v2)

Review of attachment 8398012 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/tests/testJavascriptBridge.js
@@ +4,5 @@
> +  pongJava("javaStep1", int1);
> +}
> +
> +function jsStep2(double1) {
> +  do_check_eq(double1, 1);

Make this value not 1 (specifically, not the same as jsStep1) to be sure we're not processing a repeat message.

Comment 31

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: a. Add getTest to UITestContext (v2)

To be able to call methods in the test class, we need to be able to get the test instance from GeckoHelper.

Comment 32

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: b. Add JavascriptBridge (v3)

Nick, feel free to punt any of these reviews to other people. I think I addressed most of your comments from the last set of patches (got rid of the finish() call, moved to top-level class, etc.).

Comment 33

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: c. Add static JavascriptBridge instance in GeckoHelper (v2)

So to properly uninitialize the communication bridge, I had to add a term() method to GeckoHelper, which is called from a test's tearDown() method.

Comment 34

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: d. Add JavaBridge (v3)

Comment 35

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: e. Add test for JavascriptBridge/JavaBridge (v3)

Addressed some of :mcomella's comments.

Comment 36

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8398790 [details] [diff] [review]
e. Add test for JavascriptBridge/JavaBridge (v3)

Asking for feedback on these latest patches that addressed the previous comments.

Comment 37

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8398790 [details] [diff] [review]
e. Add test for JavascriptBridge/JavaBridge (v3)

Also :rnewman since he was at last week's meeting.

Comment 38

[Richard Newman [:rnewman]]

Comment on attachment 8398790 [details] [diff] [review]
e. Add test for JavascriptBridge/JavaBridge (v3)

Review of attachment 8398790 [details] [diff] [review]:
-----------------------------------------------------------------

This is super-brief, and it only seems to be going Java-to-JS.

::: mobile/android/base/tests/testJavascriptBridge.java
@@ +20,5 @@
> +        NavigationHelper.enterAndLoadUrl(StringHelper.ROBOCOP_JS_HARNESS_URL +
> +                                         "?path=" + TEST_JS);
> +        mToolbar.assertTitle(TEST_JS); // Test sets JS path as its title
> +
> +        pingJS("jsStep1", 1);

This feels like it should be something like:

  JSBridge bridge = Something.getJSBridge();
  bridge.call("jsStep1", 1);

I don't like the implicit global, and I don't like the existence of a single bridge, even if this is only intended for use in tests -- after all, test code should be *easier* to understand than app code, not harder.

Comment 39

[:Margaret Leibovic]

Comment on attachment 8398790 [details] [diff] [review]
e. Add test for JavascriptBridge/JavaBridge (v3)

Review of attachment 8398790 [details] [diff] [review]:
-----------------------------------------------------------------

I would be more of a fan of directly calling bridge.syncCall()/bridge.asyncCall(), since that makes it easier for me to understand what's going on under the hood. However, I'm not the one using this to write new tests, so if you think that this is a suitable API for your IME tests, I think it's fine to go ahead. We can always iterate on these APIs more later, since they're just our own internal test infrastructure APIs, and I think the easiest way to learn what works best is just to start using them. Or, as a compromise, maybe you can include the ping/pong APIs as part of your IME tests, and we can factor them out to be more global if people want to use them for other tests.

Comment 40

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8398782 [details] [diff] [review]
a. Add getTest to UITestContext (v2)

Review of attachment 8398782 [details] [diff] [review]:
-----------------------------------------------------------------

I'm skeptical about allowing access to UITest like this - it defeats the purpose of UITestContext in some ways.

I was thinking an alternatives: move the reflection to a method in UITest/UITestContext and access it via the UITestContext because at least that way, people would be less likely to use it.

Additionally (or alternatively), and terribly, we can grab a stack trace and ensure the caller of these methods is JavaScriptBridge.

At the very least, a very angry comment needs to be added!

What do you think?

Comment 41

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8398788 [details] [diff] [review]
c. Add static JavascriptBridge instance in GeckoHelper (v2)

Review of attachment 8398788 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/tests/helpers/GeckoHelper.java
@@ +62,5 @@
> +     * @param method Name of the method to call
> +     * @param args Arguments to pass to the Javascript method; must be a list of
> +     *             values allowed by JSONObject.
> +     */
> +    public static void pingJS(final String method, final Object... args) {

Why wrap these calls? Why shouldn't we just make the calls directly to a static JavaScriptBridge helper in UITest? It would avoid the need to add "term" methods when we can just call "sJSBridge.disconnect" from tearDown.

nit: I don't like ping and pong: to me, ping is a question, pong is the response. I'd prefer explicit method names (e.g. "syncCallJS", "asyncCallJS").

::: mobile/android/base/tests/helpers/HelperInitializer.java
@@ +24,5 @@
>          NavigationHelper.init(context);
>          WaitHelper.init(context);
>      }
> +
> +    public static void term(final UITestContext context) {

nit: Again, I think I'd prefer `finish`. This class (HelperInitializer) should probably also be renamed.

Comment 42

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8398790 [details] [diff] [review]
e. Add test for JavascriptBridge/JavaBridge (v3)

Review of attachment 8398790 [details] [diff] [review]:
-----------------------------------------------------------------

Probably want to test async calls too.

I'm not sure I agree with Richard with regards to the existence of more than one bridge, at least at this juncture, nor the implicit global, but as I mentioned, I think it should be a static JavaBridge, rather than the GeckoHelper wrapper method.

Otherwise, the general idea appears to be in the right direction.

::: mobile/android/base/tests/testJavascriptBridge.java
@@ +1,4 @@
> +package org.mozilla.gecko.tests;
> +
> +import static org.mozilla.gecko.tests.helpers.AssertionHelper.*;
> +import static org.mozilla.gecko.tests.helpers.GeckoHelper.*;

nit: I don't think this is necessary.

::: mobile/android/base/tests/testJavascriptBridge.js
@@ +1,3 @@
> +function jsStep1(int1) {
> +  do_check_eq(int1, 1);
> +  pongJava("javaStep1", int1);

nit: It's unclear whether js or java is first. I would name the first method, "step1", the second "step2", etc., regardless of where they're being called from.

@@ +3,5 @@
> +  pongJava("javaStep1", int1);
> +}
> +
> +function jsStep2(double1) {
> +  do_check_eq(double1, 1);

Again, this shouldn't be the same value as jsStep1.

Comment 43

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8398784 [details] [diff] [review]
b. Add JavascriptBridge (v3)

Review of attachment 8398784 [details] [diff] [review]:
-----------------------------------------------------------------

Quickly skimmed:

::: mobile/android/base/tests/helpers/JavascriptBridge.java
@@ +12,5 @@
> +import org.json.JSONException;
> +import org.json.JSONObject;
> +
> +import java.lang.reflect.Method;
> +import java.lang.reflect.InvocationTargetException;

nit: Alphabetize.

@@ +23,5 @@
> +    public static final String EVENT_TYPE = "Robocop:JS";
> +    private static final int MESSAGE_EMPTY = 0;
> +    private static final int MESSAGE_PROCESSED = 1;
> +    private static final int MESSAGE_REPLIED = 2;
> +    private static final int MESSAGE_SAVED = 3;

Can this be an enum?

@@ +150,5 @@
> +                    jsonArgs.put(convertToJSONValue(arg));
> +                }
> +            }
> +            message.put("type", EVENT_TYPE)
> +                   .put("innerType", type)

nit: "innerType" is ambiguous to me. Perhaps, "messageType" or "callType"?

@@ +231,5 @@
> +        } catch (final NoSuchMethodException e) {
> +            // Try fallback below
> +        }
> +        // Fall back to try calling each method
> +        for (final Method method : mMethods) {

Why should invokeMethod fail above and this methodology succeed?

Comment 44

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Michael Comella (:mcomella) from comment #43)
> Comment on attachment 8398784 [details] [diff] [review]
> b. Add JavascriptBridge (v3)
> 
> @@ +150,5 @@
> > +                    jsonArgs.put(convertToJSONValue(arg));
> > +                }
> > +            }
> > +            message.put("type", EVENT_TYPE)
> > +                   .put("innerType", type)
> 
> nit: "innerType" is ambiguous to me. Perhaps, "messageType" or "callType"?

We're already using innerType for JavascriptTest, so maybe we can change the name in a separate bug.

> @@ +231,5 @@
> > +        } catch (final NoSuchMethodException e) {
> > +            // Try fallback below
> > +        }
> > +        // Fall back to try calling each method
> > +        for (final Method method : mMethods) {
> 
> Why should invokeMethod fail above and this methodology succeed?

Class.getMethod() only matches exact argument types. In case we don't have the exact types, we then call the methods one-by-one, hoping Java will implicitly convert some of the arguments to the exact types. I'll add a comment.

Comment 45

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add JavascriptBridge (v4)

A common theme from the last round of feedback was to not use the static ping/pong methods. The updated patch makes JavascriptBridge usable from tests directly through instantiation.

Comment 46

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add JavaBridge (v4)

Got rid of static ping/pong methods.

Comment 47

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add test for JavascriptBridge/JavaBridge (v4)

Updated test that uses JavascriptBridge/JavaBridge more extensively, including sync call from JS and async call from Java.

Comment 48

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8402925 [details] [diff] [review]
Add JavaBridge (v4)

Review of attachment 8402925 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/tests/robocop_head.js
@@ +1197,5 @@
> +  syncCall: function (method /*, ... */) {
> +    this._sendMessage("sync-call", arguments);
> +    let thread = this._Services.tm.currentThread;
> +    let initialReplies = this._repliesNeeded;
> +    for (this._repliesNeeded++; this._repliesNeeded > initialReplies;) {

I don't really understand the JS event loop, but are you saying the *only* way to get an event back is via the Java harness replying to our message?  What if the invoked Java code sent a message to Gecko itself (not the harness)?

Wait -- maybe your loop is just funky?  Can you replace that for loop with something more clear?  It looks like you are increment _repliesNeeded immediately, then... even if this is correct, you're dooming folks to a tough parse.

Comment 49

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8402926 [details] [diff] [review]
Add test for JavascriptBridge/JavaBridge (v4)

Review of attachment 8402926 [details] [diff] [review]:
-----------------------------------------------------------------

This is pretty nice.

::: mobile/android/base/tests/testJavascriptBridge.js
@@ +1,1 @@
> +let java = new JavaBridge(this);

Maybe inJava?  Then things read like, 'inJava.syncCall'.  Not a big deal.

Comment 50

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8402922 [details] [diff] [review]
Add JavascriptBridge (v4)

Review of attachment 8402922 [details] [diff] [review]:
-----------------------------------------------------------------

The provided test is nice, but I'm not convinced just a single async call will work.  I'd prefer many tiny test cases rather than a long chain; too hard to figure out what's busted with such a chain.

::: mobile/android/base/tests/helpers/JavascriptBridge.java
@@ +67,5 @@
> +    private JSONObject mSavedMessage;
> +    // Number of levels in the synchronous call stack
> +    private int mCallStackDepth;
> +
> +    /* package */ static void init(final UITestContext context) {

Why tie this to UI Test anything?

@@ +99,5 @@
> +        }
> +        if (mCallStackDepth == 1) {
> +            // We want to wait for all asynchronous calls to finish,
> +            // because the test may end immediately after this method returns.
> +            finishPendingCalls();

What do we do if every call is async?  This will never run.  In fact, why is waiting tied to synchronous anything?  I find this whole idea of tracking stack depths on both sides fragile.

@@ +230,5 @@
> +                // Reply to Java-to-Javascript sync call
> +                return MessageStatus.REPLIED;
> +            }
> +
> +            if (!"sync-call".equals(type) && !"async-call".equals(type)) {

It's hard to figure out what are legal innerTypes.  Can this be a switch, or can you check innerType for set membership earlier?

@@ +269,5 @@
> +        }
> +        // One scenario for getMethod() to fail above is that we don't have the exact
> +        // argument types in argTypes. Now we find all the methods with the given name
> +        // and try calling them one-by-one. Java will try to convert our arguments to
> +        // the right types.

Does this really do the right thing when you have different *numbers* of arguments?  If so, explain why.

Comment 51

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Nick Alexander :nalexander from comment #48)
> Comment on attachment 8402925 [details] [diff] [review]
> Add JavaBridge (v4)
> 
> Review of attachment 8402925 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/tests/robocop_head.js
> @@ +1197,5 @@
> > +  syncCall: function (method /*, ... */) {
> > +    this._sendMessage("sync-call", arguments);
> > +    let thread = this._Services.tm.currentThread;
> > +    let initialReplies = this._repliesNeeded;
> > +    for (this._repliesNeeded++; this._repliesNeeded > initialReplies;) {
> 
> I don't really understand the JS event loop, but are you saying the *only*
> way to get an event back is via the Java harness replying to our message? 
> What if the invoked Java code sent a message to Gecko itself (not the
> harness)?
> 
> Wait -- maybe your loop is just funky?  Can you replace that for loop with
> something more clear?  It looks like you are increment _repliesNeeded
> immediately, then... even if this is correct, you're dooming folks to a
> tough parse.

I'll change it. The for loop is essentially an increment followed by a while loop that continues until this._repliesNeeded has returned to its previous level. This means we'll continue to process events (any Gecko event including our reply event) until we get the reply we wanted.

Comment 52

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Nick Alexander :nalexander from comment #50)
> Comment on attachment 8402922 [details] [diff] [review]
> Add JavascriptBridge (v4)
> 
> Review of attachment 8402922 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The provided test is nice, but I'm not convinced just a single async call
> will work.  I'd prefer many tiny test cases rather than a long chain; too
> hard to figure out what's busted with such a chain.
> 
> ::: mobile/android/base/tests/helpers/JavascriptBridge.java
> @@ +67,5 @@
> > +    private JSONObject mSavedMessage;
> > +    // Number of levels in the synchronous call stack
> > +    private int mCallStackDepth;
> > +
> > +    /* package */ static void init(final UITestContext context) {
> 
> Why tie this to UI Test anything?

Classes in tests/helpers/ are helper classes under the UI Test framework. For example, GeckoHelper.java is tied to UI Tests too.

> @@ +99,5 @@
> > +        }
> > +        if (mCallStackDepth == 1) {
> > +            // We want to wait for all asynchronous calls to finish,
> > +            // because the test may end immediately after this method returns.
> > +            finishPendingCalls();
> 
> What do we do if every call is async?  This will never run.  In fact, why is
> waiting tied to synchronous anything?  I find this whole idea of tracking
> stack depths on both sides fragile.

Using only async calls doesn't make sense for a test. A Robocop test finishes as soon as its test method finishes. If you only use async calls, by definition they're not guaranteed to be called before the test finishes, and the test is broken by design. Can you give examples on why tracking stack depths is fragile?

> @@ +230,5 @@
> > +                // Reply to Java-to-Javascript sync call
> > +                return MessageStatus.REPLIED;
> > +            }
> > +
> > +            if (!"sync-call".equals(type) && !"async-call".equals(type)) {
> 
> It's hard to figure out what are legal innerTypes.  Can this be a switch, or
> can you check innerType for set membership earlier?

I can check membership earlier.

> @@ +269,5 @@
> > +        }
> > +        // One scenario for getMethod() to fail above is that we don't have the exact
> > +        // argument types in argTypes. Now we find all the methods with the given name
> > +        // and try calling them one-by-one. Java will try to convert our arguments to
> > +        // the right types.
> 
> Does this really do the right thing when you have different *numbers* of
> arguments?  If so, explain why.

Yeah, Method.invoke() will fail with IllegalArgumentException if the number of arguments is not correct. So we move on to the next method, until we find one that matches in both argument types and argument count.

Comment 53

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8402922 [details] [diff] [review]
Add JavascriptBridge (v4)

Review of attachment 8402922 [details] [diff] [review]:
-----------------------------------------------------------------

Definitely in the right direction but we need to iron out the details.

Note that I also agree w/ nalexander's comments.

::: mobile/android/base/tests/helpers/JavascriptBridge.java
@@ +25,5 @@
> +public final class JavascriptBridge {
> +
> +    private static enum MessageStatus {
> +        QUEUE_EMPTY, // Did not process a message; queue was empty.
> +        PROCESSED,   // A non-sync message was processed.

nit: "non-sync" is just "async", right?

@@ +52,5 @@
> +
> +    public static final String EVENT_TYPE = "Robocop:JS";
> +
> +    private static Actions sActions;
> +    private static Assert sAsserter;

nit: We changed the naming conventions to remove the prepended scope (i.e. actions & asserter).

With the following comment, I might be too careful so you might want to ignore it: for correctness, these should be member variables.

However, to simplify the Bridge's use in `? extends UITest`, we can have these static variables shadow member variables as so:
---

private static Actions defaultActions; // Pretend I did Assert too
private final Actions actions;

public JavascriptBridge(..., final Actions actions) {
  ...
  this.actions = actions;
}

public JavascriptBridge(...) {
  ...
  // TODO: Ensure init() was called, throw if not. (Maybe rename init to `setDefaults` or something too)
  this.actions = defaultActions;
}

@@ +63,5 @@
> +    private final JavascriptMessageParser mLogParser;
> +    // Expecter of our internal Robocop event
> +    private final EventExpecter mExpecter;
> +    // Saved async message; see processMessage() for its purpose.
> +    private JSONObject mSavedMessage;

nit: savedAsyncMessage - might as well be explicit.

@@ +96,5 @@
> +        } catch (final AssertionFailedError e) {
> +            // Most likely an event expecter time out
> +            throw new CallException("Cannot call " + method, e);
> +        }
> +        if (mCallStackDepth == 1) {

Won't mCallStackDepth always be 1 here? How does this determine the status of async calls? Note that I'm assuming this method is not called concurrently.

nit: ws above this - it looks confusing because it could be a `finally`.

@@ +175,5 @@
> +            }
> +        } while (result != MessageStatus.QUEUE_EMPTY);
> +    }
> +
> +    private void sendMessage(final String type, final String method, final Object[] args) {

nit: type -> messageType; If we can't rename innerType, at least we can make type clearer. Same with the passed messages, if applicable.

@@ +202,5 @@
> +        final JSONArray argsArray;
> +        final Object[] args;
> +        try {
> +            if (!EVENT_TYPE.equals(message.getString("type"))) {
> +                throw new IllegalStateException("Message type is correct");

"Message type is incorrect", right? You should probably also specify what message type we're expecting here (i.e. EVENT_TYPE).

@@ +210,5 @@
> +            if ("progress".equals(type)) {
> +                // Javascript harness message
> +                mLogParser.logMessage(message.getString("message"));
> +                return MessageStatus.PROCESSED;
> +            } else if ("async-call".equals(type)) {

nit: This code block (below `else if ("async...`) is a little hard to read. I'd prefer if you did,

if (message == savedMessage) {
  // We are processing the saved message.
  ...
} else { ... }

Though I'm not sure how repetitious it would be.

@@ +216,5 @@
> +                // process the saved message and save the new one. This is done as a
> +                // form of tail call optimization, by making sync-replies come before
> +                // async-calls. On the other hand, if (message == mSavedMessage), it means
> +                // we're currently processing the saved message and should clear
> +                // mSavedMessage.

Why don't we process async calls instantly? From my understanding of this description, we do it to ensure we make our synchronous replies first (for efficiency). However, after the first async message is stored, we lose this efficiency with each subsequent async call. I'm assuming this is for waiting for the response in syncCall - why not queue up all the pending async calls and respond to them all at the end of syncCall?

If the scope of the queue is limited to what I mentioned above, I would name it as such, for clarity (e.g. pendingAsyncMessageQueue).

@@ +258,5 @@
> +     */
> +    private Object invokeMethod(final String methodName, final Object[] args) {
> +        final Class<?>[] argTypes = new Class<?>[args.length];
> +        for (int i = 0; i < argTypes.length; i++) {
> +            argTypes[i] = args[i].getClass();

This will throw if args[i] is null, right? Since we call convertFromJSONValue to get these values, it is possible that this is the case.

@@ +260,5 @@
> +        final Class<?>[] argTypes = new Class<?>[args.length];
> +        for (int i = 0; i < argTypes.length; i++) {
> +            argTypes[i] = args[i].getClass();
> +        }
> +        // Try using argument types directly without casting.

nit: ws above.

@@ +262,5 @@
> +            argTypes[i] = args[i].getClass();
> +        }
> +        // Try using argument types directly without casting.
> +        try {
> +            return invokeMethod(mTarget.getClass().getMethod(methodName, argTypes), args);

Do we ever want to call superclass methods (i.e. those not defined in the test)? If not, getDeclaredMethod. If so, please explain.

@@ +267,5 @@
> +        } catch (final NoSuchMethodException e) {
> +            // getMethod() failed; try fallback below.
> +        }
> +        // One scenario for getMethod() to fail above is that we don't have the exact
> +        // argument types in argTypes. Now we find all the methods with the given name

Why would we not have the exact argument types in argTypes? I'm assuming it's because we don't get type info from JS. If so, just elaborate in this comment, and perhaps mention how Java tries to do the argument conversion (will this always work?).

@@ +288,5 @@
> +
> +    private Object invokeMethod(final Method method, final Object[] args) {
> +        try {
> +            return method.invoke(mTarget, args);
> +        } catch (final IllegalAccessException e) {

Will this break if we're trying a bunch of methods with the same name? e.g. we want to call foo(int) and:

private foo() { ... }
public foo(int bar) { ... }

@@ +292,5 @@
> +        } catch (final IllegalAccessException e) {
> +            throw new IllegalStateException("Cannot access method " + method.getName(), e);
> +        } catch (final InvocationTargetException e) {
> +            final Throwable cause = e.getCause();
> +            if (cause instanceof CallException) {

Add a comment explaining when this might happen.

Comment 54

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

> Yeah, Method.invoke() will fail with IllegalArgumentException if the number of
> arguments is not correct. So we move on to the next method, until we find one
> that matches in both argument types and argument count.

Add a comment.

Comment 55

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8402925 [details] [diff] [review]
Add JavaBridge (v4)

Review of attachment 8402925 [details] [diff] [review]:
-----------------------------------------------------------------

r-, but could flop to r+ after I know what repliesNeeded is all about.

::: mobile/android/base/tests/robocop_head.js
@@ +1146,5 @@
> +
> +
> +/**
> + * JavaBridge facilitates communication between Java and JS. See
> + * JavascriptHelper.java for the corresponding JavascriptBridge and docs.

nit: JavascriptBridge.java.

@@ +1159,5 @@
> +};
> +
> +JavaBridge.prototype = {
> +
> +  _Services: Components.utils.import(

nit: Overly nitty for test code, perhaps, but did you consider lazily loading this? Same with Messaging.jsm.

@@ +1165,5 @@
> +
> +  _sendMessageToJava: Components.utils.import(
> +    "resource://gre/modules/Messaging.jsm", {}).sendMessageToJava,
> +
> +  _sendMessage: function (type, args) {

nit: type -> messageType, like JavascriptBridge comment.

nit: I'd prefer (messageType, methodName, args)

@@ +1193,5 @@
> +  /**
> +   * Synchronously call a method in Java,
> +   * given the method name followed by a list of arguments.
> +   */
> +  syncCall: function (method /*, ... */) {

nit: methodName

@@ +1197,5 @@
> +  syncCall: function (method /*, ... */) {
> +    this._sendMessage("sync-call", arguments);
> +    let thread = this._Services.tm.currentThread;
> +    let initialReplies = this._repliesNeeded;
> +    for (this._repliesNeeded++; this._repliesNeeded > initialReplies;) {

Agree with nalexander - why not `this._repliesNeeded++; while (this._repliesNeeded > ...`? There might be a better way to do it too. In any case, add a comment explaining what this is waiting for.

Comment 56

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Is there any expectation for this code to run concurrently (both Java and JS (e.g. Task.jsm))? I haven't been very thorough, if that's the case.

Comment 57

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8402925 [details] [diff] [review]
Add JavaBridge (v4)

Review of attachment 8402925 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/tests/robocop_head.js
@@ +1198,5 @@
> +    this._sendMessage("sync-call", arguments);
> +    let thread = this._Services.tm.currentThread;
> +    let initialReplies = this._repliesNeeded;
> +    for (this._repliesNeeded++; this._repliesNeeded > initialReplies;) {
> +      thread.processNextEvent(true);

via IRC:

<khuey> if you're spinning hte event loop you're probably doing a bad thing
<mcomella> khuey: By spinning you mean `while (whatever) { Thread.processNextEvent(...); }`?
<khuey> I mean calling processNextEvent at all
<mcomella> khuey: The test framework is waiting synchronously for messages - bad thing?
<mcomella> It's Java calling sychronously into JS
<mcomella> And we need to respond with, "Hey, the call completed."
<khuey> yeah, it's usually used for turning something async into something sync
<khuey> whether or not that's ok depends on whether the code underneath can handle events running
<khuey> if it's just testing code it might be fine
<khuey> we've had issues in the past where destructors spin the event loop in gecko
<khuey> and destructors can run from GC callbacks
<khuey> which means we end up running random events (and maybe JS) during a GC :P
<khuey> predictably the JS engine is upset

---
Since we mentioned maybe using this in prod at some point, add a comment that describes the situation (maybe with a "TODO: Research ...").

Comment 58

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8402926 [details] [diff] [review]
Add test for JavascriptBridge/JavaBridge (v4)

Review of attachment 8402926 [details] [diff] [review]:
-----------------------------------------------------------------

WIP.

::: mobile/android/base/tests/testJavascriptBridge.java
@@ +7,5 @@
> +import org.json.JSONException;
> +import org.json.JSONObject;
> +
> +/**
> + * Tests the proper operation of JavascriptBridge,

nit: and JavaBridge

@@ +34,5 @@
> +        GeckoHelper.blockForReady();
> +        NavigationHelper.enterAndLoadUrl(StringHelper.ROBOCOP_JS_HARNESS_URL +
> +                                         "?path=" + TEST_JS);
> +        mToolbar.assertTitle(TEST_JS); // Test sets JS path as its title
> +        js.syncCall("step1", 1);

Be explicit with your types here and below. e.g.:

final int i = 1;
js.syncCall("step1", i);

You should probably also test the remaining primitive types (e.g. float, long).

@@ +50,5 @@
> +        js.syncCall("step5", "step 5");
> +    }
> +
> +    public void step6(final String string) throws JSONException {
> +        // Async call fro JS

nit: "from", here and below

::: mobile/android/base/tests/testJavascriptBridge.js
@@ +28,5 @@
> +  do_check_eq(obj.step, 7);
> +  java.asyncCall("step8", {step: 8});
> +}
> +
> +let javaResponded = false;

nit: Declare at the top of the file. If you think it's missing context, make the name more specific.

Comment 59

[Wesley Johnston (:wesj)]

(In reply to Michael Comella (:mcomella) from comment #57)
> Since we mentioned maybe using this in prod at some point, add a comment
> that describes the situation (maybe with a "TODO: Research ...").

Do we need this or are you just building framework? I would avoid it if at all possible and discourage people from doing it. We've avoided haven't builting it into API's because of that. The calls we have that need it currently only do so because

1.) they're tied to platform API's that are sync. This is mainly the PrmomptSerivce, FilePicker, and NSSDialogService (although the sync FilePicker API is deprecated)

2.) they should be fixed. That's
  a.) TabSource.js - This is brand new. We should fix the API ASAP.
  b.) SharedPreferences.jsm - This is our internal API. Is should be async.
  c.) HelperApps.jsm - Needed for context menus api which is sync. We should fix that.
  d.) browser.js#7345 - This is the remote debugger. Its should just not be sync. Easy fix.

I'll file bugs on those second 4 (and maybe some platform bugs on the first 2)... It vaguely makes sense for things that show a modal dialog to the user to block, but its still probably better to just make the API sync and let the implementor decide if they want to block.

Comment 60

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

(In reply to Wesley Johnston (:wesj) from comment #59)
> (In reply to Michael Comella (:mcomella) from comment #57)
> > Since we mentioned maybe using this in prod at some point, add a comment
> > that describes the situation (maybe with a "TODO: Research ...").
> 
> Do we need this or are you just building framework? I would avoid it if at
> all possible and discourage people from doing it. We've avoided haven't
> builting it into API's because of that.

I suppose I was jumping to conclusions - I remember nalexander questioning why we have multiple Java-JS messaging systems for testing, and why this system wasn't identical to the system used in fennec. I think it's use will be limited to testing, but in any case, I think it would be worthwhile to add a comment to the effect of, "THIS IS BAD."

Comment 61

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8402926 [details] [diff] [review]
Add test for JavascriptBridge/JavaBridge (v4)

Review of attachment 8402926 [details] [diff] [review]:
-----------------------------------------------------------------

You may also want to test that exceptions are propagated as expected, if possible.

::: mobile/android/base/tests/testJavascriptBridge.java
@@ +29,5 @@
> +        js.disconnect();
> +        super.tearDown();
> +    }
> +
> +    public void testJavascriptBridge() {

Won't the test finish when js.syncCall returns in testJavascriptBridge? Are step2+ definitely called?

::: mobile/android/base/tests/testJavascriptBridge.js
@@ +35,5 @@
> +  // Sync call from Java
> +
> +  // Java calls step11() from step10()
> +  java.syncCall("step10");
> +  // Because it was a sync call above, step11() should have run by now.

nit: I think you can merge the two comments into something like:

java.syncCall("step10");
// java's step10 sets javaResponded to True
do_check_true(javaResponded)

Comment 62

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add JavascriptBridge (v5)

(In reply to Michael Comella (:mcomella) from comment #53)
> Comment on attachment 8402922 [details] [diff] [review]
> Add JavascriptBridge (v4)
> 
> ::: mobile/android/base/tests/helpers/JavascriptBridge.java
> @@ +25,5 @@
> > +public final class JavascriptBridge {
> > +
> > +    private static enum MessageStatus {
> > +        QUEUE_EMPTY, // Did not process a message; queue was empty.
> > +        PROCESSED,   // A non-sync message was processed.
> 
> nit: "non-sync" is just "async", right?

It's supposed to mean "anything that's not sync"

> @@ +52,5 @@
> > +
> > +    public static final String EVENT_TYPE = "Robocop:JS";
> > +
> > +    private static Actions sActions;
> > +    private static Assert sAsserter;
> 
> nit: We changed the naming conventions to remove the prepended scope (i.e.
> actions & asserter).

I don't think we have consensus on it? Last time there were still people arguing for using prefixes. Using prefixes is also in the Android code style guidelines.

> @@ +96,5 @@
> > +        } catch (final AssertionFailedError e) {
> > +            // Most likely an event expecter time out
> > +            throw new CallException("Cannot call " + method, e);
> > +        }
> > +        if (mCallStackDepth == 1) {
> 
> Won't mCallStackDepth always be 1 here? How does this determine the status
> of async calls? Note that I'm assuming this method is not called
> concurrently.

processPendingMessage() may invoke code that calls syncCall() reentrantly. In that case mCallStackDepth will be greater than 1. The method is not called concurrently.

> @@ +210,5 @@
> > +            if ("progress".equals(type)) {
> > +                // Javascript harness message
> > +                mLogParser.logMessage(message.getString("message"));
> > +                return MessageStatus.PROCESSED;
> > +            } else if ("async-call".equals(type)) {
> 
> nit: This code block (below `else if ("async...`) is a little hard to read.
> I'd prefer if you did,

Hm I didn't find the other way easier to read.

> @@ +216,5 @@
> > +                // process the saved message and save the new one. This is done as a
> > +                // form of tail call optimization, by making sync-replies come before
> > +                // async-calls. On the other hand, if (message == mSavedMessage), it means
> > +                // we're currently processing the saved message and should clear
> > +                // mSavedMessage.
> 
> Why don't we process async calls instantly? From my understanding of this
> description, we do it to ensure we make our synchronous replies first (for
> efficiency). However, after the first async message is stored, we lose this
> efficiency with each subsequent async call. I'm assuming this is for waiting
> for the response in syncCall - why not queue up all the pending async calls
> and respond to them all at the end of syncCall?

Yeah, we save the last async message purely as an optimization, to place the sync reply before the async message. But in a typical scenario (back and forth message passing), there's no more than one async messages in a row, so I think using a queue is adding more complexity for not much gain.

> @@ +258,5 @@
> > +     */
> > +    private Object invokeMethod(final String methodName, final Object[] args) {
> > +        final Class<?>[] argTypes = new Class<?>[args.length];
> > +        for (int i = 0; i < argTypes.length; i++) {
> > +            argTypes[i] = args[i].getClass();
> 
> This will throw if args[i] is null, right? Since we call
> convertFromJSONValue to get these values, it is possible that this is the
> case.

Good catch.

> @@ +262,5 @@
> > +            argTypes[i] = args[i].getClass();
> > +        }
> > +        // Try using argument types directly without casting.
> > +        try {
> > +            return invokeMethod(mTarget.getClass().getMethod(methodName, argTypes), args);
> 
> Do we ever want to call superclass methods (i.e. those not defined in the
> test)? If not, getDeclaredMethod. If so, please explain.

Yup we want superclasses. The class hierarchy of the target object is an implementation detail that we shouldn't make assumptions about. It could be the test class itself or it could be another class the test class instantiates.

> @@ +288,5 @@
> > +
> > +    private Object invokeMethod(final Method method, final Object[] args) {
> > +        try {
> > +            return method.invoke(mTarget, args);
> > +        } catch (final IllegalAccessException e) {
> 
> Will this break if we're trying a bunch of methods with the same name? e.g.
> we want to call foo(int) and:
> 
> private foo() { ... }
> public foo(int bar) { ... }

Corrected to skip private methods if possible.

Comment 63

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add JavaBridge (v5)

(In reply to Michael Comella (:mcomella) from comment #55)
> Comment on attachment 8402925 [details] [diff] [review]
> Add JavaBridge (v4)
> 
> @@ +1159,5 @@
> > +};
> > +
> > +JavaBridge.prototype = {
> > +
> > +  _Services: Components.utils.import(
> 
> nit: Overly nitty for test code, perhaps, but did you consider lazily
> loading this? Same with Messaging.jsm.

I think you'd likely be using JavaBridge if you are including robocop_head.js.

(In reply to Michael Comella (:mcomella) from comment #56)

> Is there any expectation for this code to run concurrently (both Java and JS
> (e.g. Task.jsm))? I haven't been very thorough, if that's the case.

No, it's single threaded on either side. Note that Task.jsm is asynchronous and sequential (i.e. not concurrent).

(In reply to Michael Comella (:mcomella) from comment #57)
> Comment on attachment 8402925 [details] [diff] [review]
> Add JavaBridge (v4)
> 
> Review of attachment 8402925 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/tests/robocop_head.js
> @@ +1198,5 @@
> > +    this._sendMessage("sync-call", arguments);
> > +    let thread = this._Services.tm.currentThread;
> > +    let initialReplies = this._repliesNeeded;
> > +    for (this._repliesNeeded++; this._repliesNeeded > initialReplies;) {
> > +      thread.processNextEvent(true);
> 
> via IRC:
> 
> <khuey> if you're spinning hte event loop you're probably doing a bad thing
> <mcomella> khuey: By spinning you mean `while (whatever) {
> Thread.processNextEvent(...); }`?
> <khuey> I mean calling processNextEvent at all
> <mcomella> khuey: The test framework is waiting synchronously for messages -
> bad thing?
> <mcomella> It's Java calling sychronously into JS
> <mcomella> And we need to respond with, "Hey, the call completed."
> <khuey> yeah, it's usually used for turning something async into something
> sync
> <khuey> whether or not that's ok depends on whether the code underneath can
> handle events running
> <khuey> if it's just testing code it might be fine
> <khuey> we've had issues in the past where destructors spin the event loop
> in gecko
> <khuey> and destructors can run from GC callbacks
> <khuey> which means we end up running random events (and maybe JS) during a
> GC :P
> <khuey> predictably the JS engine is upset
> 
> ---
> Since we mentioned maybe using this in prod at some point, add a comment
> that describes the situation (maybe with a "TODO: Research ...").

(In reply to Wesley Johnston (:wesj) from comment #59)
> (In reply to Michael Comella (:mcomella) from comment #57)
> > Since we mentioned maybe using this in prod at some point, add a comment
> > that describes the situation (maybe with a "TODO: Research ...").
> 
> Do we need this or are you just building framework? I would avoid it if at
> all possible and discourage people from doing it. We've avoided haven't
> builting it into API's because of that. The calls we have that need it
> currently only do so because
> 
> 1.) they're tied to platform API's that are sync. This is mainly the
> PrmomptSerivce, FilePicker, and NSSDialogService (although the sync
> FilePicker API is deprecated)
> 
> 2.) they should be fixed. That's
>   a.) TabSource.js - This is brand new. We should fix the API ASAP.
>   b.) SharedPreferences.jsm - This is our internal API. Is should be async.
>   c.) HelperApps.jsm - Needed for context menus api which is sync. We should
> fix that.
>   d.) browser.js#7345 - This is the remote debugger. Its should just not be
> sync. Easy fix.
> 
> I'll file bugs on those second 4 (and maybe some platform bugs on the first
> 2)... It vaguely makes sense for things that show a modal dialog to the user
> to block, but its still probably better to just make the API sync and let
> the implementor decide if they want to block.

(In reply to Michael Comella (:mcomella) from comment #60)
> (In reply to Wesley Johnston (:wesj) from comment #59)
> > (In reply to Michael Comella (:mcomella) from comment #57)
> > > Since we mentioned maybe using this in prod at some point, add a comment
> > > that describes the situation (maybe with a "TODO: Research ...").
> > 
> > Do we need this or are you just building framework? I would avoid it if at
> > all possible and discourage people from doing it. We've avoided haven't
> > builting it into API's because of that.
> 
> I suppose I was jumping to conclusions - I remember nalexander questioning
> why we have multiple Java-JS messaging systems for testing, and why this
> system wasn't identical to the system used in fennec. I think it's use will
> be limited to testing, but in any case, I think it would be worthwhile to
> add a comment to the effect of, "THIS IS BAD."

Yeah we'll only be using this framework for testing and not in product. It wouldn't work in product anyways because of reflection in the Java portion. In product you should not be spinning the event loop yourself, but here we're only doing it in controlled tests and it's necessary to make our sync call API work, so it's okay IMO.

Comment 64

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add test for JavascriptBridge/JavaBridge (v5)

(In reply to Michael Comella (:mcomella) from comment #58)
> Comment on attachment 8402926 [details] [diff] [review]
> Add test for JavascriptBridge/JavaBridge (v4)
> 
> @@ +34,5 @@
> > +        GeckoHelper.blockForReady();
> > +        NavigationHelper.enterAndLoadUrl(StringHelper.ROBOCOP_JS_HARNESS_URL +
> > +                                         "?path=" + TEST_JS);
> > +        mToolbar.assertTitle(TEST_JS); // Test sets JS path as its title
> > +        js.syncCall("step1", 1);
> 
> Be explicit with your types here and below. e.g.:
> 
> final int i = 1;
> js.syncCall("step1", i);

I changed it to using casting, e.g. | js.syncCall("step1", (int) 1); |. I think that puts the point across without being too verbose.

> You should probably also test the remaining primitive types (e.g. float,
> long).

I don't want to support float and long because they don't correspond to native types in the JS engine. I should add a comment somewhere explaining that. Boolean is worth testing though.

(In reply to Michael Comella (:mcomella) from comment #61)
> Comment on attachment 8402926 [details] [diff] [review]
> Add test for JavascriptBridge/JavaBridge (v4)
> 
> Review of attachment 8402926 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You may also want to test that exceptions are propagated as expected, if
> possible.

Like other tests, uncaught exceptions are logged and cause the test to exit.

> ::: mobile/android/base/tests/testJavascriptBridge.java
> @@ +29,5 @@
> > +        js.disconnect();
> > +        super.tearDown();
> > +    }
> > +
> > +    public void testJavascriptBridge() {
> 
> Won't the test finish when js.syncCall returns in testJavascriptBridge? Are
> step2+ definitely called?

syncCall doesn't return until async messages have been processed, so essentially that syncCall doesn't return until all our calls have finished.

Comment 65

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Correctly handle multiline JS test harness messages (v1)

When I was testing, I found some JS test harness messages contained multiple lines, and as a result, our regex didn't match these messages. I changed the regex to handle multiple lines, and made the TEST- group only match what we expect. Beginning/trailing newlines are now handled by String.trim() rather than inside the regex to make the regex easier to read.

Comment 66

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8405002 [details] [diff] [review]
Correctly handle multiline JS test harness messages (v1)

Review of attachment 8405002 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm.

::: mobile/android/base/tests/helpers/JavascriptMessageParser.java
@@ +25,5 @@
>  
>      // Messages matching this pattern are handled specially.  Messages not
>      // matching this pattern are still printed.
>      private static final Pattern testMessagePattern =
> +        Pattern.compile("TEST-([A-Z\\-]+) \\| (.*) \\| (.*)", Pattern.DOTALL);

Hmm, should these pieces be non-greedy, that is, should '.*' be '.*?'?  In any case, lgtm.

Comment 67

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #66)
> Comment on attachment 8405002 [details] [diff] [review]
> Correctly handle multiline JS test harness messages (v1)
> 
> Review of attachment 8405002 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm.

Although, a comment explaining that you want to handle multiple lines would be good.  And possibly only accepting TEST at the start of a line.

Comment 68

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

(In reply to Jim Chen [:jchen :nchen] from comment #62)
> > nit: We changed the naming conventions to remove the prepended scope (i.e.
> > actions & asserter).
> 
> I don't think we have consensus on it? Last time there were still people
> arguing for using prefixes. Using prefixes is also in the Android code style
> guidelines.

I think we did: https://mail.mozilla.org/pipermail/mobile-firefox-dev/2014-February/000551.html

Comment 69

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

(In reply to Jim Chen [:jchen :nchen] from comment #62)
> processPendingMessage() may invoke code that calls syncCall() reentrantly.
> In that case mCallStackDepth will be greater than 1.

Add a comment to that effect, please.

Comment 70

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8404889 [details] [diff] [review]
Add JavascriptBridge (v5)

Review of attachment 8404889 [details] [diff] [review]:
-----------------------------------------------------------------

r+ w/ nits.

Please do comment 69 also.

::: mobile/android/base/tests/helpers/JavascriptBridge.java
@@ +302,5 @@
> +            }
> +        }
> +        // Now we're out of options
> +        throw new UnsupportedOperationException(
> +            "Cannot call method " + methodName + " (private? wrong argument types?)",

nit: protected and package-private will also cause problems, right? Maybe "private" -> "non-public"?

@@ +317,5 @@
> +            final Throwable cause = e.getCause();
> +            if (cause instanceof CallException) {
> +                // Don't wrap CallExceptions; this can happen if a call is nested on top
> +                // of existing sync calls, and the nested call throws a CallException
> +                throw (CallException) cause;

nit: Is this cast necessary?

Comment 71

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8404904 [details] [diff] [review]
Add JavaBridge (v5)

Review of attachment 8404904 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm.

Comment 72

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8405000 [details] [diff] [review]
Add test for JavascriptBridge/JavaBridge (v5)

Review of attachment 8405000 [details] [diff] [review]:
-----------------------------------------------------------------

r+ w/ nits.

::: mobile/android/base/tests/testJavascriptBridge.java
@@ +37,5 @@
> +        mToolbar.assertTitle(TEST_JS); // Test sets JS path as its title
> +        js.syncCall("check_js_int_arg", (int) 1);
> +    }
> +
> +    public void check_java_int_arg(final int int2) {

nit: -> `checkJavaIntArg`. Same below.

::: mobile/android/base/tests/testJavascriptBridge.js
@@ +5,5 @@
> +  java.disconnect();
> +});
> +do_test_pending();
> +
> +function check_js_int_arg(int1) {

nit: I think the mobile JS convention typically is camel case, but I can go either way here, I think.

Comment 73

[(inactive) Jim Chen [:jchen] [:darchons]]

Thanks everyone!

https://hg.mozilla.org/integration/fx-team/rev/e4a8ab9d7140
https://hg.mozilla.org/integration/fx-team/rev/5cece5a197f2
https://hg.mozilla.org/integration/fx-team/rev/3c680467893a
https://hg.mozilla.org/integration/fx-team/rev/be1e13f5edcc

Comment 74

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e4a8ab9d7140
https://hg.mozilla.org/mozilla-central/rev/5cece5a197f2
https://hg.mozilla.org/mozilla-central/rev/3c680467893a
https://hg.mozilla.org/mozilla-central/rev/be1e13f5edcc