Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Description • 11 years ago

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.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 1 • 11 years ago

mccr8 for CC bits.
baku for implementation.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) 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?)

	Andrew McCreight [:mccr8]
	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

	Anne (:annevk)
	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.

	Boris Zbarsky [:bzbarsky]
	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?  ;)

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 6 • 11 years ago

Allow Promise.cast to be called without arguments.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 7 • 11 years ago

Andrea, could I have an ETA for review?

	Andrea Marchesini [:baku]
	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?

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) 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.

	Boris Zbarsky [: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...

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 11 • 11 years ago

https://github.com/domenic/promises-unwrapping

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 10 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...

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 10 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.

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 10 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....

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 15 • 10 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.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 16 • 10 years ago

Enter correct compartment in Race() when rejecting.

	Boris Zbarsky [:bzbarsky]
	Comment 17 • 10 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.

	Boris Zbarsky [:bzbarsky]
	Comment 19 • 10 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.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 20 • 10 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a4c6de5079e5

	Carsten Book [:Tomcat]
	Comment 21 • 10 years ago

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

	Florian Scholz (Open Web Docs)
	Comment 22 • 8 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