Closed
Bug 911213
Opened 12 years ago
Closed 12 years ago
Implement new promise constructor
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: annevk, Assigned: baku)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 3 obsolete files)
48.73 KB,
patch
|
Details | Diff | Splinter Review |
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).
Assignee | ||
Updated•12 years ago
|
Assignee: baku → amarchesini
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #798160 -
Flags: review?(bzbarsky)
Attachment #798160 -
Flags: feedback?(annevk)
Updated•12 years ago
|
Keywords: dev-doc-needed
Updated•12 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Comment 2•12 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.
![]() |
||
Comment 3•12 years ago
|
||
> 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•12 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 5•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
Attachment #799386 -
Attachment is obsolete: true
Attachment #799386 -
Flags: review?(bzbarsky)
![]() |
||
Comment 8•12 years ago
|
||
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•12 years ago
|
||
Attachment #799459 -
Attachment is obsolete: true
Comment 10•12 years ago
|
||
Shouldn't that bug be marked WONTFIX in favour of bug 911216?
Reporter | ||
Comment 11•12 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.
Comment 12•12 years ago
|
||
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•12 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.
Comment 14•12 years ago
|
||
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•12 years ago
|
||
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).
Assignee | ||
Comment 16•12 years ago
|
||
Assignee | ||
Comment 17•12 years ago
|
||
Comment 18•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Updated•12 years ago
|
Blocks: inter-app-comm-api
Comment 19•9 years ago
|
||
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
•