Closed Bug 904479 Opened 11 years ago Closed 9 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: kanru, Assigned: nicko.robson, Mentored)

References

Details

(Whiteboard: [good first bug][mentor-lang=zh][lang=js])

Attachments

(1 file)

The usage will like let self = this; return this.createPromise(function(resolverId) { let resolver = self.getPromiseResolver(resolverId); ... }); See bug 899073
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
Take this to give it a try.
Assignee: nobody → rlu
Status: NEW → ASSIGNED
Whiteboard: [good first bug][mentor=nsm][mentor=kanru] → [good first bug][mentor=nsm][mentor=kanru][mentor-lang=zh]
Mentor: nsm.nikhil kchen
Mentor: kchen, nsm.nikhil
Whiteboard: [good first bug][mentor=nsm][mentor=kanru][mentor-lang=zh] → [good first bug][mentor-lang=zh]
Mentor: nsm.nikhil, kchen
Whiteboard: [good first bug][mentor-lang=zh] → [good first bug][mentor-lang=zh][lang=js]
I'd like to give this one a try
Go for it! Ask questions if anything is unclear :)
Thanks you. I'm unable to assign it to myself, is that something you can change?
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.
Ah ok, wasn't sure how it worked. Thanks :)
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.
Flags: needinfo?(nsm.nikhil)
Attachment #8648399 - Flags: review?(kchen)
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!
Attachment #8648399 - Flags: review+
Comment on attachment 8648399 [details] [diff] [review] 0001-Bug-904479-Added-createPromiseWithId-that-returns-id.patch Review of attachment 8648399 [details] [diff] [review]: ----------------------------------------------------------------- Great!
Attachment #8648399 - Flags: review?(kchen) → review+
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!
Keywords: checkin-needed
Assignee: nobody → nicko.robson
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: