Closed Bug 941757 Opened 9 years ago Closed 9 years ago

Add Promise constructor to Promise.jsm


(Toolkit :: Async Tooling, defect)

Windows XP
Not set





(Reporter: Paolo, Assigned: bbenvie)



(Keywords: dev-doc-complete, Whiteboard: [Async:ready] )


(1 file, 2 obsolete files)

To facilitate the migration to DOM or ES6 Promises, code written to use Promise.jsm should be able to use the Promise constructor instead of Promise.defer():

return new Promise((resolve, reject) => { ... });

The Promise.defer() method should be marked as deprecated in the documentation, but still remain available for backwards compatibility. No deprecation warning is needed at this point if Promise.defer() is used.
Attached patch promise-constructor.patch (obsolete) — Splinter Review
I've taken a stab at implementing this. Let me know if you think this is the right direction and I'll finish it off and add tests.
Attachment #8336403 - Flags: feedback?(paolo.mozmail)
Comment on attachment 8336403 [details] [diff] [review]

Looks good to me, thanks a lot! Since I'll be away next week you might want to ask Dave for final review once the tests are in place.

> +  this.nextPromise = new Promise(noop);

nit: I think "new Promise(() => {});" is clearer.
Attachment #8336403 - Flags: feedback?(paolo.mozmail) → feedback+
Assignee: nobody → bbenvie
Whiteboard: [Async] → [Async:ready]
Hi Brandon, do you plan to move this forward?
Indeed, I am planning to work it this week.
Blocks: 941920
Attached patch promise-constructor.patch (obsolete) — Splinter Review
This patch adds tests for the constructor and fixes the behavior of the constructor. When the executor is run, if it throws this should result in a rejection. The |this| in the executor should be the promise as well.
Attachment #8336403 - Attachment is obsolete: true
Comment on attachment 8374544 [details] [diff] [review]

Pending the try run, this is ready for review.
Attachment #8374544 - Flags: review?(paolo.mozmail)
Comment on attachment 8374544 [details] [diff] [review]

Review of attachment 8374544 [details] [diff] [review]:

Looks quite good to me! Finding a single line to nitpick was difficult :-)

::: toolkit/modules/tests/xpcshell/test_Promise.js
@@ +671,5 @@
> +    } catch (e) {
> +      do_check_true(true, "Constructor fails when not passed a function");
> +    }
> +
> +    let executorRun = false;

nit: executorRan   ;-)
Attachment #8374544 - Flags: review?(paolo.mozmail) → review+
Thank you! Patch updated to address feedback. Will land as soon as the tree opens.
Attachment #8374544 - Attachment is obsolete: true
Attachment #8375648 - Flags: review+
Whiteboard: [Async:ready] → [Async:ready] [fixed-in-fx-team]
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [Async:ready] [fixed-in-fx-team] → [Async:ready]
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.