Implement new promise constructor

RESOLVED FIXED in mozilla26

Status

()

RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: annevk, Assigned: baku)

Tracking

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

Trunk
mozilla26
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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).
(Reporter)

Updated

5 years ago
Blocks: 885333
(Reporter)

Updated

5 years ago
Blocks: 911216
(Assignee)

Updated

5 years ago
Assignee: baku → amarchesini
(Assignee)

Comment 1

5 years ago
Created attachment 798160 [details] [diff] [review]
patch
Attachment #798160 - Flags: review?(bzbarsky)
Attachment #798160 - Flags: feedback?(annevk)
Keywords: dev-doc-needed
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk

Comment 2

5 years ago
(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.
> 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

5 years ago
(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 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
Attachment #798160 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 6

5 years ago
Created attachment 799386 [details] [diff] [review]
patch

Can you take a look at it again? I have an issue with unsafeGet().
mValue.address() doesn't work because of a 'const'.
Attachment #798160 - Attachment is obsolete: true
Attachment #798160 - Flags: feedback?(annevk)
Attachment #799386 - Flags: review?(bzbarsky)
(Assignee)

Comment 7

5 years ago
Created attachment 799459 [details] [diff] [review]
patch
Attachment #799386 - Attachment is obsolete: true
Attachment #799386 - Flags: review?(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.
Attachment #799459 - Flags: review+
(Assignee)

Comment 9

5 years ago
Created attachment 799469 [details] [diff] [review]
patch
Attachment #799459 - Attachment is obsolete: true
Shouldn't that bug be marked WONTFIX in favour of bug 911216?
(Reporter)

Comment 11

5 years ago
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.
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.
(Reporter)

Comment 13

5 years ago
We know the path forward. This bug has always been advertized as an intermediate API-side fix until we get the proper solution ready.
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.
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).
https://hg.mozilla.org/mozilla-central/rev/07dafead9048
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.