989098 - Improve EventDispatcher/Messaging.jsm

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

We can make several improvements to our messaging:
1) Prevent double JSON.parsing. We parse the result at [1] to get the GUID. We then dispatch the string, rather than the parsed data, at [2]. That means we need to call JSON.parse all over again, which is inefficient.
2) Allow passing more than just JSONObjects in the API. For example, in bug 946022, I'd like to pass null data for a response. If we change sendResponse to accept an Object instead of a JSONObject, we can send anything supported by JSONObject#put (null, strings, JSONArray, etc).

[1] http://hg.mozilla.org/mozilla-central/file/bb4dd9872236/mobile/android/modules/Messaging.jsm#l22
[2] http://hg.mozilla.org/mozilla-central/file/bb4dd9872236/mobile/android/modules/Messaging.jsm#l30

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Use wrapper object to pass generic objects in sendResponse

Comment 2

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Update response listeners to accept parsed JSON

Comment 3

[Brian Nicholson (:bnicholson)]

One more advantage: we no longer expose the __guid__ property in the data for response listeners.

Comment 4

[Wesley Johnston (:wesj)]

Comment on attachment 8398154 [details] [diff] [review]
Part 1: Use wrapper object to pass generic objects in sendResponse

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

This will break the tree, so make sure you fold these when you land 'em.

::: mobile/android/base/EventDispatcher.java
@@ +117,5 @@
>          try {
> +            JSONObject wrapper = new JSONObject();
> +            wrapper.put(GUID, message.getString(GUID));
> +            wrapper.put("response", error);
> +            GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent(message.getString("type") + ":Error", wrapper.toString()));

We should probably just delegate both of these to one private function that takes the suffix for the message.

Comment 5

[Brian Nicholson (:bnicholson)]

Attachment: Use wrapper object to pass generic objects in sendResponse

Made suggested changes. I'll just fold these now so I don't forget.

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 8398155 [details] [diff] [review]
Part 2: Update response listeners to accept parsed JSON

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

You should send out a newsgroup post when you land these, since they'll likely break WIP patches by people in ways that aren't obvious.

::: mobile/android/modules/Prompt.jsm
@@ +160,1 @@
>        Services.obs.removeObserver(this, "Prompt:Return", false);

You might as well remove this while you're here. And flip aData->data if you want.

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 8398175 [details] [diff] [review]
Use wrapper object to pass generic objects in sendResponse

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

Err. I think I broke things. r+ for everyone.

Comment 8

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/fx-team/rev/1bf54f059927

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1bf54f059927