Closed Bug 941757 Opened 6 years ago Closed 6 years ago

Add Promise constructor to Promise.jsm

Categories

(Toolkit :: Async Tooling, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: Paolo, Assigned: bbenvie)

References

Details

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

Attachments

(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]
promise-constructor.patch

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.

https://tbpl.mozilla.org/?tree=Try&rev=1bb03addb48b
Attachment #8336403 - Attachment is obsolete: true
Comment on attachment 8374544 [details] [diff] [review]
promise-constructor.patch

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

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+
https://hg.mozilla.org/integration/fx-team/rev/a99fda3b6d2b
Whiteboard: [Async:ready] → [Async:ready] [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/a99fda3b6d2b
Status: NEW → RESOLVED
Closed: 6 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.