Enable promises by default

RESOLVED FIXED in mozilla29

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: annevk, Assigned: nsm)

Tracking

(Blocks: 1 bug, {dev-doc-complete, feature, relnote})

Trunk
mozilla29
dev-doc-complete, feature, relnote
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 29+)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
Due to limitations of SpiderMonkey we cannot implement promises there for now (see bug 911216). We should update our implementation in DOM to match https://github.com/domenic/promises-unwrapping as closely as possible and enable it by default now TC39 has declared consensus.
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
(Reporter)

Updated

4 years ago
Depends on: 879245, 887687
Summary: Implement ES6 promises in DOM → Enable promises by default
Created attachment 8341024 [details] [diff] [review]
enabled.patch
Attachment #8341024 - Flags: review?(ehsan)
Uh, how is ehsan the right person to make this call?
Also, why do we want to switch to disabling in chrome if the pref is disabled?
Comment on attachment 8341024 [details] [diff] [review]
enabled.patch

(In reply to :Ms2ger from comment #2)
> Uh, how is ehsan the right person to make this call?

mmm good question.
He just reviewed a lot of my promise/datastore patches, so in automatic I selected him.
Can you? :)
Attachment #8341024 - Flags: review?(ehsan) → review?(Ms2ger)
Created attachment 8341061 [details] [diff] [review]
enabled.patch
Attachment #8341061 - Flags: review?(Ms2ger)
Attachment #8341024 - Attachment is obsolete: true
Attachment #8341024 - Flags: review?(Ms2ger)
What is the latest status of the spec and our implementation's conformance to it?
(Reporter)

Comment 7

4 years ago
https://github.com/domenic/promises-unwrapping is stable as far as what we can implement in DOM is concerned (subclassing details are still twiddled). However, we have not made an attempt to do that yet. I believe our implementation is still based on the old DOM promises with only the constructor changed around.
(In reply to Anne (:annevk) from comment #7)
> https://github.com/domenic/promises-unwrapping is stable as far as what we
> can implement in DOM is concerned (subclassing details are still twiddled).
> However, we have not made an attempt to do that yet. I believe our
> implementation is still based on the old DOM promises with only the
> constructor changed around.

That is correct.

I'm not sure how we'd support subclassing and the internal methods in a DOM implementation.

We are spec compliant in the behaviour of the constructor, Promise.resolve()/reject() and then()/catch(). Bug 939332 implements the rest of the spec. I think that covers 90% of the use cases for Promise. In fact, I don't even understand the reason subclassing and so on is supported. I'd appreciate some pointers to use cases.
> I'm not sure how we'd support subclassing and the internal methods in a DOM
> implementation.

As in, allowing web pages to subclass our Promise impl?

We could in fact support that with some cooperation from the JS engine; it just needs to create the right sort of object.  We need to solve this for DOM object in general, imo.
(Reporter)

Comment 10

4 years ago
I don't think we need to support subclassing in order to ship promises. Subclassing is an ES6 feature that we do not support overall. We should aim for supporting promises to the extent they can be implemented in ES5.

http://domenic.me/aplus-tests-against-the-browser/ has tests of which we fail most at the moment.
Yep, most of those have to do with the ES spec having totally different edge-case argument semantics from the WebIDL version.
(In reply to comment #11)
> Yep, most of those have to do with the ES spec having totally different
> edge-case argument semantics from the WebIDL version.

Hrm, so what's the plan to reconcile those differences?
Depends on: 945766
Presumably moving away from doing any webidl argument stuff for our worker code.  That is, declare all arguments as "optional any", and do all the argument processing using manual JSAPI in the C++.  I should really fix it so that "any" implies "optional".

Comment 14

4 years ago
Here is a page that allows you to run most of the existing test suite in your browser: http://domenic.me/aplus-tests-against-the-browser/

Firefox Aurora is at 84 tests passing out of 879 total.

I am working on more tests beyond just the Promises/A+ ones (which test `then`, `resolve`, and `reject`); see more details at https://github.com/domenic/aplus-tests-against-the-browser/tree/gh-pages#readme

Comment 15

4 years ago
Oh goodness, Anne already posted those and I didn't scroll up, sorry :(
Comment on attachment 8341061 [details] [diff] [review]
enabled.patch

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

Is this still in my queue?
Attachment #8341061 - Flags: review?(Ms2ger) → review-
I think we need to get more up-to-date with the final spec before we can flip this on.

Latest word from dherman is that he thinks that we can get rid of the hash table which would make the spec much simpler. Need to get that confirmed with the others working on the spec though.

However he also recommended that we implement some of the "static" functions, like .cast(), .all() etc, before flipping it on.

Dave, can you let us know when the relevant pieces have been added to the ecma spec?
Dave, see comment 17
Flags: needinfo?(dherman)

Comment 19

4 years ago
> Dave, can you let us know when the relevant pieces have been added to the ecma spec?

They are already added (and have been for over two months); the spec is API complete [1].

> Latest word from dherman is that he thinks that we can get rid of the hash table which would make the spec much simpler. Need to get that confirmed with the others working on the spec though.

Not sure what you mean by "the hash table" but if it's the thenable coercions weak map, then that was removed a while ago [2]. We want to add something similar, where a promise caches its fulfillment value to prevent against changes [3], but this is proving annoying due to all the extra internal complexity necessary to support the monadic stuff. I will try to do so tonight though.

Relatedly, that fulfillment-value caching is the last remaining change before the spec is feature complete [4]. What remains after that are simple non-normative editorial issues.

More worryingly, however, is the fact that the promises in Firefox don't implement promise semantics, as can be seen from bug 945766 and comment 10. Chrome's promises, notably, do pass these tests.

[1]: https://github.com/domenic/promises-unwrapping/issues/milestones?state=closed
[2]: https://github.com/domenic/promises-unwrapping/commit/65a5e6295534abda714dced18fe4d8ab8ff9137a
[3]: https://github.com/domenic/promises-unwrapping/issues/79
[4]: https://github.com/domenic/promises-unwrapping/issues/milestones?state=open

Comment 20

3 years ago
Isn't this done already?
(In reply to Mayhem~ from comment #20)
> Isn't this done already?

Not for web pages. Only for certified apps and chrome workers.
I think we are ready to flip this on and ship. Will upload a rebased patch right now.

Mossop, will enabling this affect addons that use core/promise.js from the addon sdk?
Flags: needinfo?(dherman) → needinfo?(dtownsend+bugmail)
Jordan?
Flags: needinfo?(dtownsend+bugmail) → needinfo?(jsantell)
The sdk's `core/promise.js` is its own implementation and this would not affect anyone using it
Flags: needinfo?(jsantell)
Jonas, is it ok to enable this in workers too? I mean, Promises are a JS thing and so will be present in workers, but is it observable for workers to know whether they are running DOM Promise or JS Promise and is that a problem?
Created attachment 8367424 [details] [diff] [review]
Enable DOM Promises.
Attachment #8367424 - Flags: review?(bzbarsky)
Assignee: amarchesini → nsm.nikhil
Attachment #8367424 - Flags: superreview?(jonas)
Attachment #8341061 - Attachment is obsolete: true
(Reporter)

Comment 27

3 years ago
Should we not remove Promise::EnabledForScope altogether? Seems this should always be available, including in workers.
Attachment #8367424 - Attachment is obsolete: true
Attachment #8367424 - Flags: superreview?(jonas)
Attachment #8367424 - Flags: review?(bzbarsky)
Created attachment 8367493 [details] [diff] [review]
Enable DOM Promises.

Get rid of dom.promise.enabled.
Attachment #8367493 - Flags: superreview?(bzbarsky)
Created attachment 8367501 [details] [diff] [review]
Remove all mention of dom.promise.enabled from other tests.
Attachment #8367501 - Flags: review?(schien)
Attachment #8367501 - Flags: review?(bzbarsky)
Comment on attachment 8367493 [details] [diff] [review]
Enable DOM Promises.

sr=me
Attachment #8367493 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 8367501 [details] [diff] [review]
Remove all mention of dom.promise.enabled from other tests.

r=me
Attachment #8367501 - Flags: review?(bzbarsky) → review+
Yes, go ahead and enable this both in Windows and in Workers! Woot!
Comment on attachment 8367501 [details] [diff] [review]
Remove all mention of dom.promise.enabled from other tests.

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

lgtm.
Attachment #8367501 - Flags: review?(schien) → review+
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1d7c2025bb56
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6db8b08a3aea
Backed out for B2G test_interfaces.html failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e4ed3b79f4d

https://tbpl.mozilla.org/php/getParsedLog.php?id=33824940&tree=Mozilla-Inbound
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fc70e3e4a241
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/50aff7651394
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f33534019590
https://hg.mozilla.org/mozilla-central/rev/fc70e3e4a241
https://hg.mozilla.org/mozilla-central/rev/50aff7651394
https://hg.mozilla.org/mozilla-central/rev/f33534019590
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

Updated

3 years ago
relnote-firefox: --- → ?
Keywords: dev-doc-needed, feature, relnote
relnote-firefox: ? → 29+

Comment 38

3 years ago
Is there any need of manual QA for this feature?

Updated

3 years ago
Flags: needinfo?(nsm.nikhil)
(In reply to Ioana Budnar, QA [:ioana] from comment #38)
> Is there any need of manual QA for this feature?

Nope
Flags: needinfo?(nsm.nikhil)

Updated

3 years ago
Whiteboard: [qa-]
I updated the compat. table at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise and saw that this was already mentioned on https://developer.mozilla.org/en-US/Firefox/Releases/29
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.