941757 - Add Promise constructor to Promise.jsm

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, defect)

Description

[:Paolo Amadini]

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.

Comment 1

[Brandon Benvie [:benvie]]

Attachment: promise-constructor.patch

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.

Comment 2

[:Paolo Amadini]

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.

Comment 3

[:Paolo Amadini]

Hi Brandon, do you plan to move this forward?

Comment 4

[Brandon Benvie [:benvie]]

Indeed, I am planning to work it this week.

Comment 5

[Brandon Benvie [:benvie]]

Attachment: promise-constructor.patch

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

Comment 6

[Brandon Benvie [:benvie]]

Comment on attachment 8374544 [details] [diff] [review]
promise-constructor.patch

Pending the try run, this is ready for review.

Comment 7

[:Paolo Amadini]

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   ;-)

Comment 8

[Brandon Benvie [:benvie]]

Attachment: promise-constructor.patch

Thank you! Patch updated to address feedback. Will land as soon as the tree opens.

Comment 9

[Brandon Benvie [:benvie]]

https://hg.mozilla.org/integration/fx-team/rev/a99fda3b6d2b

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/a99fda3b6d2b