Callers should not have to think about JSAPI.

Please document what Maybe* do. (and { goes to its own line at the beginning of a method.)

Comment on attachment 8377828 [details] [diff] [review] Patch >+ JS::Rooted<JSObject*> scope(aCx, GetWrapper()); Are we relying on this to not return null? If so, what guarantees that? >+ return WrapNewBindingObject(aCx, scope, *aArgument, aValue); Why *aArgument instead of just aArgument? >+ typedef void (Promise::*MaybeFunc)(JSContext* aCx, >+ JS::Handle<JS::Value> aValue); Does this need to be public? >+ JSAutoCompartment ac(cx, GetWrapper()); We're _definitely_ assuming GetWrapper() doesn't return null here. Again, what guarantees this? And yes, need documentation. r=me with the above addressed

Mmm, I was thinking that GetWrapper would wrap. I guess I need a bigger gun. Will fix it up in the morning.

You could just always preserve the wrapper of promises? Seems like that should be enough.

There's no guarantee that we'll have a wrapper at all, I think. What if C++ wants to hand back a pre-rejected Promise?

I think the correct thing to do is probably to replace mWindow with an nsCOMPtr<nsIGlobalObject>, and set that up on both the main and worker threads. Then we'll have a guaranteed non-null JSObject, and I can add assertions.

> What if C++ wants to hand back a pre-rejected Promise? Ah, if all the promise consumption is happening in C++, yeah.

(In reply to Boris Zbarsky [:bz] from comment #8) > > What if C++ wants to hand back a pre-rejected Promise? > > Ah, if all the promise consumption is happening in C++, yeah. Yep, which is what's happening in https://bugzilla.mozilla.org/attachment.cgi?id=8376950&action=diff#a/dom/telephony/Telephony.cpp_sec5.

Ah, I see. One other note: the patch assumes that wrappercached things are using WebIDL bindings. That's not quite true so far, but maybe it's enough to MOZ_ASSERT here so if someone pases in something that's not a WebIDL object they notice quickly?

Comment on attachment 8377828 [details] [diff] [review] Patch New patch coming tomorrow. Hsin-Yi is testing it now.

Comment on attachment 8381535 [details] [diff] [review] Patch >+ // Helpers for using Promise from C++. >+ // To use a new type T, add an ArgumentToJSValue overload below. But this new setup works with most of the types, right? I mean, every don't need to implement ArgumentToJSValue for their type.

Comment on attachment 8381535 [details] [diff] [review] Patch >+ JS::Rooted<JSObject*> scope(aCx, global->GetGlobalJSObject()); >+ MOZ_ASSERT(scope); I don't think we actually guarantee non-null there, do we? See discussion in bug 975419. >+ // Accept objects that inherit from nsWrapperCache and nsISupports (e.g. most >+ // most DOM objects). Undouble the "most". r=me with that, I guess.

Does this code have or need tests?

This code is not actually used in this patch. It's used by things like bug 969218, which hopefully come with their own tests.