Closed Bug 1043633 Opened 6 years ago Closed 6 years ago

Use namespacing in Messaging.jsm

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 35

People

(Reporter: bnicholson, Assigned: bnicholson, Mentored)

References

Details

Attachments

(2 files, 2 obsolete files)

Most .jsms export an object that acts as a namespace for its function properties. Messaging.jsm, on the other hand, exports sendMessageToJava directly, meaning it goes into the global namespace. We should move sendMessageToJava (in addition to the request listeners from bug 967325) into a namespace object.

We might also want to rename the module to JavaBridge or something similar, so the API could look like this:

JavaBridge.sendMessage(...)
JavaBridge.addListener(...)
JavaBridge.removeListener(...)
+1, I agree that export has confused me in the past.

Making wesj into a mentor here :)
Mentor: wjohnston
Moves sendMessageToJava into Messaging namespace, deprecating the global one. This is built on top of bug 1055144, so this also moves the new promise-based sendRequestToJava.
Assignee: nobody → bnicholson
Status: NEW → ASSIGNED
Attachment #8474673 - Flags: review?(wjohnston)
Depends on: 1055144
Since sendMessageToJava is now namespaced, I would personally prefer to change all existing code in browser.js that uses a global "sendMessageToJava" to instead use the namespaced "Messaging.sendMessageToJava", which makes usage consistent with every other jsm module. But that's probably a change worth some discussion, so in the meantime, here's a simpler fix.

This gets rid of a lot of the deprecation warnings that get dumped to logcat. There are still many more, but we could handle these separately (maybe as mentor bugs). Or I can just go through our codebase now and fix them all in one big patch here...either is fine with me.
Attachment #8474682 - Flags: review?(wjohnston)
Blocks: rAc-android
Comment on attachment 8474673 [details] [diff] [review]
Move messaging functions into Messaging namespace

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

::: mobile/android/modules/Messaging.jsm
@@ +34,5 @@
>     *
>     * Only one request listener can be registered for a given message.
>     *
>     * Example usage:
> +   *   Messaging.addRequestListener({

I would probably just leave these method names alone. I don't think there's a lot of value in Messaging.addRequestListener vs Messaging.addListener();

@@ +88,5 @@
> +   *
> +   * @param aMessage Message to send; must be an object with a "type" property
> +   * @returns A promise that resolves to the response
> +   */
> +  sendRequestToJava: function (aMessage) {

In honor of Android, I think we should name this something like sendRequestForResult (and the other sendRequest);
Attachment #8474673 - Flags: review?(wjohnston) → review+
Comment on attachment 8474682 [details] [diff] [review]
Define Messaging.sendMessageToJava as global function in browser.js

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

I would much rather we just update all callers in the codebase (one massive patch seems fine). I would guess we've got more consumers who just access the method off the window anyway, like they do with NativeWindow,  or who use nsIAndroidBridge than we do ones that use Messaging.jsm. So for those guys, this is just masking their "error".

::: mobile/android/chrome/content/browser.js
@@ +192,5 @@
>  
> +// sendMessageToJava is namespaced in the Messaging module. Make it global here
> +// for convenience.
> +let { sendMessageToJava } = Messaging;
> +

I really hate destructuring like this. let sendMessageToJava = Messaging.sendMessageToJava is clearer in what its doing. Also, don't wrap the comment for two words.
Comment on attachment 8474682 [details] [diff] [review]
Define Messaging.sendMessageToJava as global function in browser.js

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

I would much rather we just update all callers in the codebase (one massive patch seems fine). I would guess we've got more consumers who just access the method off the window anyway, like they do with NativeWindow,  or who use nsIAndroidBridge than we do ones that use Messaging.jsm. So for those guys, this is just masking their "error".

::: mobile/android/chrome/content/browser.js
@@ +192,5 @@
>  
> +// sendMessageToJava is namespaced in the Messaging module. Make it global here
> +// for convenience.
> +let { sendMessageToJava } = Messaging;
> +

I really hate destructuring like this. let sendMessageToJava = Messaging.sendMessageToJava is clearer in what its doing. Also, don't wrap the comment for two words.
Attachment #8474682 - Flags: review?(wjohnston) → review-
Rather than creating a global sendRequest function in bug 1055144 and then moving it into a namespace here, it makes more sense to just create the namespace first and add the new function to it. Updating dependencies.
Blocks: 1055144
No longer depends on: 1055144
This moves the existing sendMessageToJava into the Messaging namespace. This is built on the existing code in the tree; Promises will be built on top of this in bug 1055144.
Attachment #8474673 - Attachment is obsolete: true
Attachment #8474682 - Attachment is obsolete: true
Attachment #8478422 - Flags: review?(wjohnston)
Updates all single-arg versions of sendMessageToJava to Messaging.sendRequest. I didn't touch the two-arg versions of sendMessageToJava (the ones that take a callback function) since those will have to be updated again in bug 1055144, and keeping them unchanged makes them easier to track down.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=98c875fc00fa

Reminder to self: Update MDN.
Attachment #8478446 - Flags: review?(wjohnston)
Comment on attachment 8478422 [details] [diff] [review]
Move sendMessageToJava into Messaging namespace

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

I assume this makes the log fill up with deprecation warnings :)
Attachment #8478422 - Flags: review?(wjohnston) → review+
Attachment #8478446 - Flags: review?(wjohnston) → review+
(In reply to Brian Nicholson (:bnicholson) from comment #10)
> Reminder to self: Update MDN.

On second thought, this API is probably too low-level for most add-ons. We should instead be encouraging add-on authors to use modules that don't directly involve interacting with the messaging layer to minimize breakage.

(In reply to Wesley Johnston (:wesj) from comment #11)
> Comment on attachment 8478422 [details] [diff] [review]
> Move sendMessageToJava into Messaging namespace
> 
> Review of attachment 8478422 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I assume this makes the log fill up with deprecation warnings :)

Indeed...most of them are fixed by patch 2 here, but the sendMessageToJava calls that use the callback param will still show a warning. Those are fixed by bug 1055144.
https://hg.mozilla.org/mozilla-central/rev/b87c78023ba3
https://hg.mozilla.org/mozilla-central/rev/c474691b9c0f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
No longer blocks: 1062097
You need to log in before you can comment on or make changes to this bug.