Closed
Bug 945766
Opened 10 years ago
Closed 10 years ago
DOM Promises should pass Promises/A+ test suite
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: nsm, Assigned: nsm)
References
Details
Attachments
(1 file, 2 obsolete files)
11.69 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
Assignee | ||
Comment 1•10 years ago
|
||
Got full compliance with the Promises/A+ test suite, but there are hacks. 1) To silently ignore non callables passed to then() and catch(), I had to change the type signatures and then manually cast and use AnyCallback generated code. This isn't so bad, but would it be nicer to add a extended attribute and switch to the codegen to ignore rather than TypeError? 2) There seems to be no way to throw a TypeError (rather than an Error) using the current JSAPI. Only StopIteration is exposed publicly. I'm not sure if adding a public JSAPI method just for this use case is nice.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → nsm.nikhil
Assignee | ||
Updated•10 years ago
|
Attachment #8355409 -
Flags: feedback?(bzbarsky)
Assignee | ||
Comment 2•10 years ago
|
||
There are a few failures on a complete run (although I had one run with 0 failures). They seem to be because of the 200ms timeout. Running just the failed tests individually works.
Comment 3•10 years ago
|
||
Comment on attachment 8355409 [details] [diff] [review] DOM Promise should pass Promise/A+ tests. > but would it be nicer to add a extended attribute and switch to the codegen to > ignore rather than TypeError Does [TreatNonCallableAsNull] not do what you want here? I'd think it does. A bigger concern may be that it might get removed from the spec, but we can still keep it in our implementation... I don't understand why you had to duplicate any part of AnyCallback here, honestly, but maybe I'm missing some change your code made from the default generated code? > 2) There seems to be no way to throw a TypeError (rather than an Error) using > the current JSAPI. There actually is. Bindings already have machinery to do this, in fact; see mozilla::ErrorResult::ThrowTypeError/ReportTypeError (I _hate_ the naming; the latter actually just sets the pending exception on the cx; it doesn't report it via the error reporter or anything like that.... but that's how JSAPI names their throwing methods :( ). A minor problem is that you don't actually want to _throw_ a TypeError. You want to reject with a TypeError. But you could JS_GetPendingException/JS_ClearPendingException. It's a tad hacky, but would let you provide a useful error message here and doesn't require JSAPI changes.
Attachment #8355409 -
Flags: feedback?(bzbarsky) → feedback+
Comment 4•10 years ago
|
||
I actually have a spec question here. If I read Promise.prototype.then right, it will basically treat this.constructor as the thing to use to construct promises. But there is no guarantee that this.constructor == Promise; the page can mess with the proto chain. Does the test suite simply not test this case? Because I don't see us doing anything remotely like what the spec says here in our implementation...
Flags: needinfo?(nsm.nikhil)
Comment 5•10 years ago
|
||
And the point is, the spec is doing this generically on purpose, afaict, so you can subclass Promise.... so it's not like it's a spec bug that it's using a non-canonical Promise constructor.
Comment 6•10 years ago
|
||
> Does the test suite simply not test this case? Because I don't see us doing anything remotely like what the spec says here in our implementation... Indeed, the test suite is not exhaustive, unfortunately; it only tests the subset of the spec from the predecessor "spec" at http://promisesaplus.com/. All the subclassing stuff was hashed out only in the ES6 spec (promises-unwrapping), and there aren't any tests for that part of it. (Yet... I need an intern.) But yes, you use `this.constructor` for new instances in `Promise.prototype.then`, just like you use `this.constructor` for new instances in ES6's `Array.prototype.map`.
Comment 7•10 years ago
|
||
OK. I guess we could just ignore the subclassing stuff here for now just like the JS engine does for Array. That's certainly the path of least resistance for the moment. :(
Summary: DOM Promises should pass Promises/A+ spec → DOM Promises should pass Promises/A+ test suite
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #3) > Comment on attachment 8355409 [details] [diff] [review] > DOM Promise should pass Promise/A+ tests. > > > but would it be nicer to add a extended attribute and switch to the codegen to > > ignore rather than TypeError > > Does [TreatNonCallableAsNull] not do what you want here? I'd think it does. > A bigger concern may be that it might get removed from the spec, but we can > still keep it in our implementation... No, it still requires the argument to be an object. > > I don't understand why you had to duplicate any part of AnyCallback here, > honestly, but maybe I'm missing some change your code made from the default > generated code? Because the codegen doesn't generate code when AnyCallback isn't actually used in the IDL. > > > 2) There seems to be no way to throw a TypeError (rather than an Error) using > > the current JSAPI. > > There actually is. Bindings already have machinery to do this, in fact; see > mozilla::ErrorResult::ThrowTypeError/ReportTypeError (I _hate_ the naming; > the latter actually just sets the pending exception on the cx; it doesn't > report it via the error reporter or anything like that.... but that's how > JSAPI names their throwing methods :( ). > > A minor problem is that you don't actually want to _throw_ a TypeError. You > want to reject with a TypeError. But you could > JS_GetPendingException/JS_ClearPendingException. It's a tad hacky, but > would let you provide a useful error message here and doesn't require JSAPI > changes. I'll try this, but it seems like the report error system doesn't play well with GetPendingException. IIRC JS_IsPendingException() would still be false.
Flags: needinfo?(nsm.nikhil)
Comment 9•10 years ago
|
||
> No, it still requires the argument to be an object. No, as a matter of fact it doesn't. The codegen for [TreatNonCallableAsNone] arguments looks like this: nsRefPtr<EventHandlerNonNull> arg0; if (args[0].isObject() && JS_ObjectIsCallable(cx, &args[0].toObject())) { { // Scope for tempRoot JS::Rooted<JSObject*> tempRoot(cx, &args[0].toObject()); arg0 = new EventHandlerNonNull(tempRoot, mozilla::dom::GetIncumbentGlobal()); } } else { arg0 = nullptr; } which seems like exactly what you want. > Because the codegen doesn't generate code when AnyCallback isn't actually used in the > IDL. This is what DummyBinding.webidl is for. ;) > but it seems like the report error system doesn't play well with GetPendingException. > IIRC JS_IsPendingException() would still be false. JS_ReportErrorNumberUCArray (called by ReportTypeError) should land us in ReportError, which will do js_ErrorToException, which will in fact JS_SetPendingException. But only if JS_IsRunning(cx), apparently.... Which I guess in your case it's not. Jason, what's a sane way to do what we want here? We basically need to create, but not report, a TypeError object, and our cx doesn't have JS running on it.
Flags: needinfo?(jorendorff)
Assignee | ||
Comment 10•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8355409 -
Attachment is obsolete: true
Comment 11•10 years ago
|
||
Comment on attachment 8357615 [details] [diff] [review] DOM Promise should pass Promise/A+ tests. Drive-by comments: 1) If we want to treat undefined and null the same, just give the argument a default value of null in the IDL and drop the Optional complications in then/catch? 2) Good catch on the compartment bit we need in Race, but shouldn't that be in the bug that implements Race? 3) Don't pass the second arg to CheckedUnwrap(); there's no reason to unwrap outers in this case, I think (not that it matters in practice for functions). 4) If CheckedUnwrap() returns null, you're done trying to get info out of unwrapped. Don't try to work with that null. That will also give you a nice scope for "ac", by the way. 5) You probably don't need the rooted callbackObj temporary. 6) If you do the mNextPromise->RejectInternal() call, shouldn't you return right after that instead of falling through to the ResolveInternal() call? 7) The "We've to manually" comment in the IDL seems to be out of date.
Assignee | ||
Updated•10 years ago
|
Attachment #8357615 -
Attachment is obsolete: true
Comment 13•10 years ago
|
||
Comment on attachment 8357652 [details] [diff] [review] DOM Promise should pass Promise/A+ tests. >+ JS::Rooted<JSString*> fn(cx, JS_NewStringCopyZ(cx, fileName)); This can return null. If it does, you have to deal with the OOM. >+ JS::Rooted<JSString*> message(cx, Same here. >+ MOZ_CRASH("Could not throw TypeError!"); This is OOM again... Still not sure what you should do here if it happens. :( r=me with that fixed.
Attachment #8357652 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 14•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/6f90f9c6b49d
Flags: needinfo?(jorendorff)
Comment 15•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6f90f9c6b49d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•