Closed Bug 1270360 Opened 4 years ago Closed 4 years ago

Implement runtime.sendNativeMessage

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

Tracking

(firefox50 fixed)

RESOLVED FIXED
mozilla50
Iteration:
50.1 - Jun 20
Tracking Status
firefox50 --- fixed

People

(Reporter: aswan, Assigned: aswan)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [native messaging] triaged)

Attachments

(1 file)

Assignee: nobody → aswan
Iteration: --- → 50.1
Comment on attachment 8763065 [details]
Bug 1270360 Implement runtime.sendNativeMessage()

https://reviewboard.mozilla.org/r/59400/#review56506

Looks good. Thanks!

::: toolkit/components/extensions/NativeMessaging.jsm:364
(Diff revision 1)
> +
> +  sendMessage(msg) {
> +    let responsePromise = new Promise((resolve, reject) => {
> +      let msgListener, disconnectListener;
> +
> +      msgListener = (what, msg) => {

Please add a `let` here rather than predeclaring this.

::: toolkit/components/extensions/NativeMessaging.jsm:371
(Diff revision 1)
> +        this.off("disconnect", disconnectListener);
> +        resolve(msg);
> +      };
> +      disconnectListener = (what, err) => {
> +        this.off("message", msgListener);
> +        this.off("disconnect", disconnectListener);

I don't think there's a need to remove these listeners. Just disable the balanced listeners rule for this scope, and then we can just use inline arrow functions for the listeners.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_native_messaging.html:251
(Diff revision 1)
> +    }, err => {
> +      browser.test.succeed("sendNativeMessage() to a nonexistent app failed");
> +    });
> +
> +    // Check regular message exchange
> +    browser.runtime.sendNativeMessage("echo", MSG).then(reply => {

We should chain this to the last listener, so we're sure that the promise either resolves or rejects.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_native_messaging.html:269
(Diff revision 1)
> +  }, ID);
> +
> +  yield extension.startup();
> +  yield extension.awaitMessage("finished");
> +
> +  // with sendNativeMessage(), the subprocess should be disconnected

Please capitalize and end with full stop.
Attachment #8763065 - Flags: review?(kmaglione+bmo) → review+
https://reviewboard.mozilla.org/r/59400/#review56506

> I don't think there's a need to remove these listeners. Just disable the balanced listeners rule for this scope, and then we can just use inline arrow functions for the listeners.

I think we do need to remove them.  If the native app happens to send two messages back-to-back or do something to trigger an error after sending a message, then we'll end up calling `resolve()` or `reject()` on a promise that is already settled.
https://reviewboard.mozilla.org/r/59400/#review56506

> I think we do need to remove them.  If the native app happens to send two messages back-to-back or do something to trigger an error after sending a message, then we'll end up calling `resolve()` or `reject()` on a promise that is already settled.

It doesn't matter. Calling resolve or reject has no effect after the first time one of them is called.
https://reviewboard.mozilla.org/r/59400/#review56506

> It doesn't matter. Calling resolve or reject has no effect after the first time one of them is called.

Hm, is that documented behavior?  I went looking for it on MDN and couldn't find anything definitive.
Comment on attachment 8763065 [details]
Bug 1270360 Implement runtime.sendNativeMessage()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59400/diff/1-2/
https://reviewboard.mozilla.org/r/59400/#review56506

> Hm, is that documented behavior?  I went looking for it on MDN and couldn't find anything definitive.

Yes[1]: "Attempting to resolve or reject a resolved promise has no effect."

[1]: http://www.ecma-international.org/ecma-262/6.0/#sec-promise-objects
Comment on attachment 8763065 [details]
Bug 1270360 Implement runtime.sendNativeMessage()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59400/diff/2-3/
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5592e9d2a86e
Implement runtime.sendNativeMessage() r=kmag
https://hg.mozilla.org/mozilla-central/rev/5592e9d2a86e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.