Closed Bug 1003670 Opened 6 years ago Closed 6 years ago

Add test for EventListener/NativeJSObject

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 32

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Depends on: 1004073
Attached patch Add testEventDispatcher (v1) (obsolete) — Splinter Review
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.
Attachment #8416030 - Flags: review?(michael.l.comella)
Attachment #8416030 - Flags: review?(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
Attachment #8416030 - Flags: review?(bnicholson) → review+
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 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".
Attachment #8416030 - Flags: review?(michael.l.comella) → review+
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.
Attachment #8417601 - Flags: review+
Attachment #8416030 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/4ed94f132f5b
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/4ed94f132f5b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
You need to log in before you can comment on or make changes to this bug.