Closed
Bug 939332
Opened 11 years ago
Closed 11 years ago
Add Promise.all, Promise.cast, Promise.race
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: nsm, Assigned: nsm)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [qa-])
Attachments
(2 files, 3 obsolete files)
19.99 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
14.00 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
These utilities are part of the ap2 spec, and they are useful in getting rid of the legacy JS implemented Promise.jsm/promise.js from our code base.
Assignee | ||
Comment 1•11 years ago
|
||
mccr8 for CC bits.
baku for implementation.
Attachment #8333624 -
Flags: review?(continuation)
Attachment #8333624 -
Flags: review?(amarchesini)
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → nsm.nikhil
Assignee | ||
Comment 2•11 years ago
|
||
Comment on attachment 8333624 [details] [diff] [review]
Implement Promise.all, Promise.cast, Promise.race.
annevk for spec compliance and tests (Should I add any more tests?)
Attachment #8333624 -
Flags: review?(annevk)
Comment 3•11 years ago
|
||
Comment on attachment 8333624 [details] [diff] [review]
Implement Promise.all, Promise.cast, Promise.race.
Review of attachment 8333624 [details] [diff] [review]:
-----------------------------------------------------------------
r=me for CC
Attachment #8333624 -
Flags: review?(continuation) → review+
Comment 4•11 years ago
|
||
Domenic has a bunch of tests. They should be in the repository. I think at some point he's going to convert them into the preferred TC39 format.
Comment 5•11 years ago
|
||
Comment on attachment 8333624 [details] [diff] [review]
Implement Promise.all, Promise.cast, Promise.race.
Can we please just drop the insane "optional<any>" business from promises before we start propagating it futher? ;)
Assignee | ||
Comment 6•11 years ago
|
||
Allow Promise.cast to be called without arguments.
Attachment #8334025 -
Flags: review?(amarchesini)
Assignee | ||
Updated•11 years ago
|
Attachment #8333624 -
Attachment is obsolete: true
Attachment #8333624 -
Flags: review?(annevk)
Attachment #8333624 -
Flags: review?(amarchesini)
Assignee | ||
Comment 7•11 years ago
|
||
Andrea, could I have an ETA for review?
Flags: needinfo?(amarchesini)
Comment 8•11 years ago
|
||
Comment on attachment 8334025 [details] [diff] [review]
Implement Promise.all, Promise.cast, Promise.race.
Review of attachment 8334025 [details] [diff] [review]:
-----------------------------------------------------------------
looks good but I want to have a review from someone who knows memory management/CC/GC better than me.
::: dom/promise/Promise.cpp
@@ +506,5 @@
> + {
> + mozilla::DropJSObjects(this);
> + }
> +
> + void SetValue(uint32_t index, const Optional<JS::Handle<JS::Value> >& aValue)
>>&
that additional space is not needed any more.
index => aIndex
@@ +527,5 @@
> + }
> +
> + mCountdown--;
> + if (mCountdown == 0) {
> + Optional<JS::Handle<JS::Value> > result(cx, JS::ObjectValue(*mValues));
> > => >>
@@ +580,5 @@
> + {
> + }
> +
> + void
> + ResolvedCallback(const Optional<JS::Handle<JS::Value> >& aValue)
> >& => >>&
@@ +586,5 @@
> + mCountdownHolder->SetValue(mIndex, aValue);
> + }
> +
> + void
> + RejectedCallback(const Optional<JS::Handle<JS::Value> >& aValue)
ditto.
@@ +619,5 @@
> + }
> + }
> +
> + if (aIterable.Length() == 0) {
> + JS::Rooted<JSObject*> empty(aCx, JS_NewArrayObject(aCx, 0, nullptr));
Add a test for this.
@@ +662,5 @@
> + const Optional<JS::Handle<JS::Value> >& aValue, ErrorResult& aRv)
> +{
> + // If a Promise was passed, just return it.
> + JS::Rooted<JS::Value> value(aCx, aValue.WasPassed() ? aValue.Value() :
> + JS::UndefinedValue());
indentation here.
::: dom/promise/Promise.h
@@ +81,5 @@
>
> already_AddRefed<Promise>
> Catch(const Optional<OwningNonNull<AnyCallback> >& aRejectCallback);
>
> + // FIXME(nsm): We only support array's for now, not true iterables.
follow up? file a bug.
::: dom/promise/tests/test_promise_utils.html
@@ +112,5 @@
> +
> +function promiseCastArray() {
> + var p = Promise.cast([1,2,3]);
> + ok(p instanceof Promise, "Should cast to a Promise.");
> + p.then(runTest);
check the values. It must be == [1, 2, 3] right?
@@ +140,5 @@
> +
> +function promiseRaceEmpty() {
> + var p = Promise.race([]);
> + ok(p instanceof Promise, "Should return a Promise.");
> + // An empty race never resolves!
reject?
@@ +247,5 @@
> + test();
> +}
> +
> +var p = SpecialPowers.getBoolPref("dom.promise.enabled");
> +SpecialPowers.setBoolPref("dom.promise.enabled", p);
why these 2 lines?
Attachment #8334025 -
Flags: review?(bzbarsky)
Attachment #8334025 -
Flags: review?(amarchesini)
Attachment #8334025 -
Flags: review+
Assignee | ||
Comment 9•11 years ago
|
||
Got rid of Optional<>
Added empty Promise.all() test
Per spec, Promise.race([]) doesn't resolve or reject. Raising the issue in spec.
Filed follow up for iterables.
Requesting r?(bz) per :baku's request.
Attachment #8355402 -
Flags: review?(bzbarsky)
Comment 10•11 years ago
|
||
This bug doesn't seem to have a link to the spec being implemented. Could you add one, please? Hard to review otherwise...
Assignee | ||
Comment 11•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8334025 -
Attachment is obsolete: true
Attachment #8334025 -
Flags: review?(bzbarsky)
Comment 12•11 years ago
|
||
So some obvious issues for this patch wrt the spec:
1) The spec semantics for the argument to all() don't match the proposed sequences-are-iterables WebIDL semantics. Is this just a short-term situation in that we plan to transition off sequences here? Or do we need to raise a spec issue? Clearly the test suite doesn't test this stuff, if you're passing it.
I would personally prefer that we just matched the proposed WebIDL sequence sematics here: snapshot the iterator before doing anything with the things we get from it.
2) Calling Promise::Cast where the spec has Invoke(C, "cast", (nextValue)) leads to quite different behavior if someone has redefined Promise.cast. Looks like there's no coverage for this in the test suite either?
3) Same for calling Promise::Resolve in the "empty iterator" case.... That should end up calling whatever the resolve method returned by GetDeferred was, which can be something totally different from Promise::Resolve. Except never is in the test suite.
4) Same for Invoke(nextPromise, "then", (countdownFunction, deferred.[[Reject]])) being quite different from nextPromise->AppendCallbacks, if someone messed with Promise.prototype.then. Again, presumably not tested?
I can go ahead and review the details of the patch as needed, but I'd like to be clear on what behavior we're actually aiming for here. Are we aiming for "follow the spec" or "something sort of like the spec as long as no one does any subclassing or messes with any of our objects in any way" or something even weaker than that? I don't mean long-term; at some point this will presumably move into the JS engine and stop being our problem, but for the moment...
Flags: needinfo?(nsm.nikhil)
Flags: needinfo?(domenic)
Flags: needinfo?(domenic)
Comment 13•11 years ago
|
||
Oh, and I've only looked at all() so far. I'll bet money there are other undertested bits in race(), though cast() looks like it only uses internal state so is probably OK.
Comment 14•11 years ago
|
||
Oh, and I'm still sorting through the spec to make sure that it actually, like Humpty Dumpty, says what it means. Hence all the questions....
Assignee | ||
Comment 15•11 years ago
|
||
I think the intention is to be "something sort of like the spec as long as no one does any subclassing or messes with any of our objects in any way". Moving to the JS engine should allow "follow the spec". I'm not sure how easy and worth the time that would be in DOM.
Flags: needinfo?(nsm.nikhil)
Assignee | ||
Comment 16•11 years ago
|
||
Enter correct compartment in Race() when rejecting.
Attachment #8357620 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•11 years ago
|
Attachment #8355402 -
Attachment is obsolete: true
Attachment #8355402 -
Flags: review?(bzbarsky)
Comment 17•11 years ago
|
||
Comment on attachment 8357620 [details] [diff] [review]
Implement Promise.all, Promise.cast, Promise.race.
>+class CountdownHolder MOZ_FINAL : public nsISupports
I think you should document that the CountdownHolder class here combines the information stored in the CountdownHolder and Countdown Function in the spec. In particular, it stores all the shared state that Countdown Functions share (values, deferred) so that all we have to store in the functions themselves is the index.
>+ : mGlobal(aGlobal.Get()), mPromise(aPromise), mCountdown(aCountdown)
I don't believe you need an mGlobal member in this class. It's nearly unused, and can be entirely unused.
>+ void SetValue(uint32_t index, const JS::Handle<JS::Value> aValue)
>+ JSContext* cx = nsContentUtils::GetDefaultJSContextForThread();
We can never end up in a situation where we're reporting an exception on cx here, right? If we _can_, we need to actually find the right JSContext from mPromise's window and whatnot.
In any case, you need to either push it or at least enter a request on it. I'm a little surprised this didn't end up asserting...
>+ JSAutoCompartment ac(cx, mGlobal);
If you use mValues here, mGlobal will be unused and can die.
>+ if (!JS_DefineElement(cx, mValues, index, aValue, nullptr, nullptr, 0)) {
This doesn't match the spec. You need JSPROP_ENUMERATE for the last argument.
>+Promise::All(const GlobalObject& aGlobal, JSContext* aCx,
>+ JS::Rooted<JSObject*> empty(aCx, JS_NewArrayObject(aCx, 0, nullptr));
Need to handle this returning null. Probably just throw NS_ERROR_OUT_OF_MEMORY on aRv and return null.
>+ JS::Rooted<JS::Value> asValue(aCx, JS::ObjectValue(*empty));
I don't think you need the temporary. Just pass JS::ObjectValue(*empty) directly to the ctor of optValue.
>+ JS::Rooted<JS::Value> nextValue(aCx, aIterable.ElementAt(i));
Again, I don't think you need that temporary.
>+ if (aRv.IsJSException()) {
And if it's not, should we just throw right away?
Also, you need aRv.WouldReportJSException() before this check. And some tests that would exercise this codepath, since they would hit the fatal assert not having WouldReportJSException() should be triggering here.
>+ // Every promise get's its own resolve callback, which will set the right
s/get's/gets/
>+Promise::Race(const GlobalObject& aGlobal, JSContext* aCx,
>+ JS::Rooted<JS::Value> nextValue(aCx, aIterable.ElementAt(i));
Again, no need for temporary.
>+ if (aRv.IsJSException()) {
Same comments as in all(). Please add tests!
>+ EnterCompartment(ac, aCx, err);
Hrm. Why did we not need this in the all() case? I suspect we did... Worth factoring out this boilerplate into a helper function?
>@@ -651,15 +878,19 @@ Promise::RunResolveTask(JS::Handle<JS::V
>+ if (mState != Pending) {
Hmm... Why was this change needed?
r=me with all those fixed.
Attachment #8357620 -
Flags: review?(bzbarsky) → review+
Flags: needinfo?(domenic)
Flags: needinfo?(domenic)
Assignee | ||
Comment 18•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8364690 -
Flags: review?(bzbarsky)
Comment 19•11 years ago
|
||
Comment on attachment 8364690 [details] [diff] [review]
Interdiff for comment 17
> Promise::HandleException(JSContext* aCx)
> {
>+ AutoDontReportUncaught silenceExceptions(aCx);
No, this doesn't make sense. You want the AutoDontReportUncaught somewhere where you're about to _throw_ an exception. That's not going to be happening here, right?
The one in ResolveInternal is correct, on the other hand.
r=me with that "silenceExceptions" bit removed.
Attachment #8364690 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 20•11 years ago
|
||
Flags: needinfo?(amarchesini)
Comment 21•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Updated•11 years ago
|
Keywords: dev-doc-needed
Updated•11 years ago
|
Whiteboard: [qa-]
Comment 22•9 years ago
|
||
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/all
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/race
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/resolve
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•