904479 - Add a createPromise() variant to DOMRequestIpcHelper that just returns the id to the callback after storing the resolver in the table

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

The usage will like

 let self = this;
 return this.createPromise(function(resolverId) {
   let resolver = self.getPromiseResolver(resolverId);
   ...
 });

See bug 899073

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Just to be clear.

A class of async JS implemented WebAPIs - SimplePush, Alarms, Keyboard and others[1] have a common pattern. The content side code returns a DOMRequest/Promise immediately to content code, after which it dispatches an IPC message to some other internal service running in the parent Gecko process on B2G. To associate requests across these boundaries, the DOMRequestIpcHelper provides a lookup tables for these DOMRequest/PromiseResolver where it associates them with an ID which can be sent over the wire. The Gecko parent process will return this ID back with the reply, which allows the WebAPI to pick the right item from the lookup table and resolve that request.

With the switch to Promises, this pattern of

{
  ImplementWebAPI: function() {
    let self = this;
    return this.createPromise(function(resolver) {
      let resolverId = self.getPromiseResolverId(resolver);
      // dispatch to parent
      messageManager.sendAsyncMessage("Message", { requestId: resolverId, ... });
    });
  },
  // receive from parent
  receiveMessage: function(msg) {
    let resolverId = msg.json.requestId;
    let resolver = this.getPromiseResolver(resolverId);
    resolver.resolve();
  }
}

becomes repetitive. The aim of the patch will be to allow a refactor on the dispatch side:

{
  ImplementWebAPI: function() {
    let self = this;
    return this.createPromiseWithId(function(resolverId) {
      // dispatch to parent
      messageManager.sendAsyncMessage("Message", { requestId: resolverId, ... });
    });
  },
  // receive from parent
  receiveMessage: function(msg) {
    let resolverId = msg.json.requestId;
    let resolver = this.getPromiseResolver(resolverId);
    resolver.resolve();
  }
}

I'm sure more refactoring can be done. There are common patterns in the receive too, but there can be separate bugs later.

[1]: http://mxr.mozilla.org/mozilla-central/search?string=DOMRequestIpcHelper

Comment 2

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Take this to give it a try.

Comment 3

[Josh Matthews [:jdm]]

Relevant code: http://mxr.mozilla.org/mozilla-central/source/dom/base/DOMRequestHelper.jsm

Comment 4

[nicko.robson]

I'd like to give this one a try

Comment 5

[Josh Matthews [:jdm]]

Go for it! Ask questions if anything is unclear :)

Comment 6

[nicko.robson]

Thanks you. I'm unable to assign it to myself, is that something you can change?

Comment 7

[Josh Matthews [:jdm]]

The policy is to assign bugs after a patch is posted. We hand out the privileges necessary to assign bugs to yourself after newcomers fix a bug or two, since that includes the ability to change any field of any bug.

Comment 8

[nicko.robson]

Ah ok, wasn't sure how it worked. Thanks :)

Comment 9

[nicko.robson]

Attachment: 0001-Bug-904479-Added-createPromiseWithId-that-returns-id.patch

First attempt at patch. I added the method and a test for it and refactored all the usages (that I could find) of the original version.

Comment 10

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8648399 [details] [diff] [review]
0001-Bug-904479-Added-createPromiseWithId-that-returns-id.patch

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

Great patch!

Comment 11

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8648399 [details] [diff] [review]
0001-Bug-904479-Added-createPromiseWithId-that-returns-id.patch

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

Great!

Comment 12

[Kan-Ru Chen [:kanru] (UTC+9)]

I pushed the patch to try https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b41dd73725d

If the try is good then we could check in!

Comment 13

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f5421c28b14

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/0f5421c28b14