1003670 - Add test for EventListener/NativeJSObject

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Comment 1

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

Attachment: Add testEventDispatcher (v1)

This gives us some test coverage in EventDispatcher and NativeJSObject, including tests for sending responses to JS. The EventDispatcher usage in this patch depends on the refactoring going on in bug 1004073.

Comment 2

[Brian Nicholson (:bnicholson)]

Comment on attachment 8416030 [details] [diff] [review]
Add testEventDispatcher (v1)

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

Seems like a good basic test. At some point, we should also probably add some tests for the particular constraints (making sure that calling success/error/cancel multiple times will throw, making sure that dispatched events without listeners don't leak, etc.). But I guess that should be my job since I owned bug 993195!

::: mobile/android/base/tests/testEventDispatcher.java
@@ +26,5 @@
> +    private static final String GECKO_RESPONSE_EVENT = "Robocop:TestGeckoResponse";
> +    private static final String NATIVE_EVENT = "Robocop:TestNativeEvent";
> +    private static final String NATIVE_RESPONSE_EVENT = "Robocop:TestNativeResponse";
> +
> +    private JavascriptBridge js;

Nit: final

@@ +68,5 @@
> +
> +    @Override
> +    public void handleMessage(final String event, final JSONObject message) {
> +        ThreadUtils.assertOnGeckoThread();
> +        try {

Nit: newline above here for consistency with other handleMessage

::: mobile/android/base/tests/testEventDispatcher.js
@@ +7,5 @@
> +});
> +do_test_pending();
> +
> +function send_test_message(type) {
> +    sendMessageToJava({

2-space indentation for JS code

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8416030 [details] [diff] [review]
Add testEventDispatcher (v1)

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

WIP.

::: mobile/android/base/tests/testEventDispatcher.java
@@ +34,5 @@
> +        super.setUp();
> +        js = new JavascriptBridge(this);
> +
> +        EventDispatcher.getInstance().registerGeckoThreadListener(
> +            (GeckoEventListener) this, GECKO_EVENT, GECKO_RESPONSE_EVENT);

nit: Double indent on these underhangs, here and below.

@@ +127,5 @@
> +    }
> +
> +    private void checkNativeJSObject(final NativeJSObject object) {
> +        fAssertEquals("Native boolean has correct value", true, object.getBoolean("boolean"));
> +        fAssertEquals("Missing boolean is handled", true, object.optBoolean("no_boolean", true));

You should also test that optBoolean returns a stored value, e.g. object.optBoolean("boolean", false) -> true.

You should probably also use a different value from the one you stored, just in case (e.g. use `false`).

nit: I'd prefer "non_existent_boolean".

::: mobile/android/base/util/EventCallback.java
@@ +8,5 @@
>   * For each instance of EventCallback, exactly one of sendResponse, sendError, or sendCancel
>   * must be called to prevent observer leaks. If more than one send* method is called, or if a
>   * single send method is called multiple times, an {@link IllegalStateException} will be thrown.
>   */
> +@RobocopTarget

You only need this on sendSuccess, sendCancel, and sendError, I believe. Putting this here is inefficient because it prevents the entire interface from being optimized, not just the parts you've used.

Note that this is also present in your patch for bug 1004073.

::: mobile/android/base/util/ThreadUtils.java
@@ +17,1 @@
>  public final class ThreadUtils {

Same, you should only need this on `assertOnGeckoThread`.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8416030 [details] [diff] [review]
Add testEventDispatcher (v1)

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

::: mobile/android/base/tests/testEventDispatcher.java
@@ +99,5 @@
> +
> +        if (NATIVE_EVENT.equals(event)) {
> +            checkNativeJSObject(message);
> +            checkNativeJSObject(message.getObject("object"));
> +            checkNativeJSObject(message.optObject("no_object", message));

As with my previous comment, I would also test if optObject works on a real value, e.g. "object".

Comment 5

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

Attachment: Add testEventDispatcher (v1.1)

Addressed comments.

(In reply to Brian Nicholson (:bnicholson) from comment #2)
> Comment on attachment 8416030 [details] [diff] [review]
> Add testEventDispatcher (v1)
> 
> ::: mobile/android/base/tests/testEventDispatcher.java
> @@ +26,5 @@
> > +    private static final String GECKO_RESPONSE_EVENT = "Robocop:TestGeckoResponse";
> > +    private static final String NATIVE_EVENT = "Robocop:TestNativeEvent";
> > +    private static final String NATIVE_RESPONSE_EVENT = "Robocop:TestNativeResponse";
> > +
> > +    private JavascriptBridge js;
> 
> Nit: final

We're initializing |js| in setUp() so it can't be final.

(In reply to Michael Comella (:mcomella) from comment #3)
> Comment on attachment 8416030 [details] [diff] [review]
> Add testEventDispatcher (v1)
> 
> ::: mobile/android/base/util/EventCallback.java
> @@ +8,5 @@
> >   * For each instance of EventCallback, exactly one of sendResponse, sendError, or sendCancel
> >   * must be called to prevent observer leaks. If more than one send* method is called, or if a
> >   * single send method is called multiple times, an {@link IllegalStateException} will be thrown.
> >   */
> > +@RobocopTarget
> 
> You only need this on sendSuccess, sendCancel, and sendError, I believe.
> Putting this here is inefficient because it prevents the entire interface
> from being optimized, not just the parts you've used.

But the entire interface is sendSuccess, sendCancel, and sendError :) We're using all the parts.

Also, RobocopTarget simply tells ProGuard to not discard these entry points. That's one type of optimization ProGuard performs, but AFAIK RobocopTarget has no effect on the other types of optimization ProGuard does.

I did change the ThreadUtils case.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/4ed94f132f5b

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4ed94f132f5b