Closed Bug 974120 Opened 6 years ago Closed 6 years ago

Add helpers for using MaybeResolve/Reject from C++

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
1.4 S2 (28feb)

People

(Reporter: khuey, Assigned: khuey)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

Callers should not have to think about JSAPI.
Attached patch Patch (obsolete) — Splinter Review
Attachment #8377828 - Flags: review?(bzbarsky)
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
Attachment #8377828 - Flags: review?(bzbarsky) → review+
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.
Attachment #8377828 - Attachment is obsolete: true
Target Milestone: --- → 1.4 S2 (28feb)
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.
Attachment #8381535 - Flags: review?(bzbarsky) → review+
Does this code have or need tests?
Flags: in-testsuite?
This code is not actually used in this patch.  It's used by things like bug 969218, which hopefully come with their own tests.
Whiteboard: [qa-]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.