911213 - Implement new promise constructor

Status: RESOLVED FIXED

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

Description

[Anne (:annevk)]

Per http://lists.w3.org/Archives/Public/www-dom/2013JulSep/0211.html the Promise constructor signature will very likely change.

We should remove PromiseResolver and instead pass new Promise two anonymous functions that have the ability to resolve and reject it.

We should try to swap our current design before Firefox 26 is due (two weeks from now).

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 2

[David Bruant]

(In reply to Anne (:annevk) from comment #0)
> We should try to swap our current design before Firefox 26 is due (two weeks
> from now).
Despite my enthusiasm about promises being part of the platform, I can't help thinking that this schedule is too short, the feature is being rushed and we (web developers and maybe implementors) are likely to pay it later down the road.
I know this isn't the right place for this comment. I know it won't really change anyone's mind. My apologies.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> I can't help thinking that this schedule is too short

I don't see why, if we're talking about landing the patch (as opposed to flipping the "promises are enabled in release builds" pref).

Comment 4

[David Bruant]

(In reply to Boris Zbarsky [:bz] from comment #3)
> > I can't help thinking that this schedule is too short
> 
> I don't see why, if we're talking about landing the patch (as opposed to
> flipping the "promises are enabled in release builds" pref).
I mistakenly assume it would be available to FirefoxOS app. My mistake. Ignore me.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 798160 [details] [diff] [review]
patch

>+#include "mozilla/dom/FunctionBinding.h"

I don't think we should represent these callbacks as Function in the IDL.

Doing that forces us to create a Function object to wrap the JSObject* in, but then the bindings code pulls the JSObject* right out of it again.  It's just wasted work.

There is an open issue on WebIDL to have different types for "platform-provided function" and "user-provided function"; if that happens we can use the former here.  For now, just use "object", please.

>+  PromiseResolverTask(Promise* aPromise,
>+                      const JS::Handle<JS::Value> aValue,

I don't think you need the const here.

>+    mPromise->RunResolveTask(JS::Handle<JS::Value>::fromMarkedLocation(&mValue),

You'll want mValue.adddress() here.

>+                              mState, Promise::SyncTask);

Please fix the indent.

>+  JS::Value mValue;

JS::Heap<JS::Value>.

>+Promise::JSCallback(JSContext *aCx, unsigned aArgc, JS::Value *aVp)
>+  JS::Value v = js::GetFunctionNativeReserved(&args.callee(), SLOT_PROMISE);

This should probably be a JS::Rooted, since UNWRAP_OBJECT can in fact GC.

>+  mozilla::DebugOnly<nsresult> rv = UNWRAP_OBJECT(Promise, aCx, &v.toObject(),
>+                                                  promise);
>+  MOZ_ASSERT(NS_SUCCEEDED(rv));
>+

It's not immediately obvious to me that rv will be success here.  In particular, I can't tell whether we might run into security-wrapper issues...

I would prefer that we handled failure here by throwing.

>+  Optional<JS::Handle<JS::Value> > value(aCx);

<sigh>.  Can we please file a followup to get rid of all this optional goop?  The fact that AnyCallback has an optional argument is just a spec bug, and we should just fix our implementation and wait for the spec issue I raised on that to be addressed.

>+    value.Value() = JS_ARGV(aCx, aVp)[0];

  value.Value() = args[0];

> Promise::Constructor(const GlobalObject& aGlobal,
>+  if (!resolveFunc) {
>+    HandleException(cx, promise);

We're not going to return the promise, so turning the exception into a reject on the promise is not useful.  We should just straight-out throw on aRv instead.  And then I think HandleException is unused.

>+Promise::ResolveCall(JSContext* aCx,

Why not just call this Resolve?  Just because of the static method with that name?

Maybe we should call this MaybeResolve instead?

>+Promise::RejectCall(JSContext* aCx,

Similar here.

>+Promise::ResolveInternal(JSContext* aCx,
>+  // If the synchronous flag is set, process promise's resolve callbacks with

s/promise's/our/

>+  // promise's resolve callbacks with value. Otherwise, the synchronous flag is

And here.

>+  // unset, queue a task to process promise's resolve callbacks with value.

And here.

>+++ b/dom/promise/PromiseCallback.cpp
> ResolvePromiseCallback::Call(const Optional<JS::Handle<JS::Value> >& aValue)
>   // Run resolver's algorithm with value and the synchronous flag set.

s/resolver's/promise's resolve/?

> RejectPromiseCallback::Call(const Optional<JS::Handle<JS::Value> >& aValue)
>   // Run resolver's algorithm with value and the synchronous flag set.

Similar.

>@@ -150,36 +149,36 @@ WrapperPromiseCallback::Call(const Optio
>   // If invoking callback threw an exception, run resolver's reject with the

And here.

>   // Otherwise, run resolver's resolve with value and the synchronous flag

And here.

>+++ b/dom/promise/PromiseCallback.h
>+// value is sent to the aNextPromise->resolveFunction() or to
>+// aNextPromise->RejectFunction() if the JS Callback throws.

Those function names don't match the actual function names.

>+// ResolvePromiseCallback calls aPromise->ResolveFunction() with the value

And here.

>+// RejectPromiseCallback calls aPromise->RejectFunction() with the value

And here.

r=me with those fixed

Comment 6

[Andrea Marchesini [:baku]]

Attachment: patch

Can you take a look at it again? I have an issue with unsafeGet().
mValue.address() doesn't work because of a 'const'.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 799459 [details] [diff] [review]
patch

>+Promise::JSCallback(JSContext *aCx, unsigned aArgc, JS::Value *aVp)
>+  if (NS_FAILED(rv)) {
>+    return false;
>+  }

You should throw an exception on aCx here.  Maybe:

  return Throw<true>(aCx, NS_ERROR_UNEXPECTED);

?

>+  // value. Otherwise, the synchronous flag is unset, queue a task to process
>+  // promise's resolve callbacks with value.

s/promise's/our/

Add a comment to the WebIDL explaining why we have "object" instead of Function.

r=me with those and the comment about unsafeAdd that we talked about added.

Comment 9

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 10

[Mounir Lamouri (:mounir)]

Shouldn't that bug be marked WONTFIX in favour of bug 911216?

Comment 11

[Anne (:annevk)]

That bug? Fixing this seems fine as it's not clear when bug 911216 will be done and getting the API aligned seems like a good thing.

Comment 12

[Mounir Lamouri (:mounir)]

Except that we implement the same thing twice. I feel like we should know what's the path forward instead of writing different implementations in different bugs and see which one wins.

Comment 13

[Anne (:annevk)]

We know the path forward. This bug has always been advertized as an intermediate API-side fix until we get the proper solution ready.

Comment 14

[Mounir Lamouri (:mounir)]

I would strongly recommend saving Mozilla's limited resources and not waste developer cycles in patch writing/review if we know that the outcome will be ditched. Note that this is still behind a pref so the benefit in improving the current implementation is limited.

Comment 15

[Boris Zbarsky [:bzbarsky]]

It depends on timeframe.  I expect it will take 1+ full cycles to get bug 911216 anywhere close to landable (e.g. we'll need more Xray machinery, binding codegen changes, etc).

Comment 16

[Andrea Marchesini [:baku]]

https://tbpl.mozilla.org/?tree=Try&rev=12a8f1dde84d

Comment 17

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/07dafead9048

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/07dafead9048

Comment 19

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise