Closed Bug 918806 Opened 7 years ago Closed 7 years ago

Enable promises by default

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
relnote-firefox --- 29+

People

(Reporter: annevk, Assigned: nsm)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, feature, relnote, Whiteboard: [qa-])

Attachments

(2 files, 3 obsolete files)

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
Depends on: 879245, 887687
Summary: Implement ES6 promises in DOM → Enable promises by default
Attached patch enabled.patch (obsolete) — Splinter Review
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)
Attached patch enabled.patch (obsolete) — Splinter Review
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?
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.
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?
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".
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
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, 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
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?
Assignee: amarchesini → nsm.nikhil
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)
Get rid of dom.promise.enabled.
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+
Is there any need of manual QA for this feature?
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)
Whiteboard: [qa-]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.