Closed Bug 983619 Opened 8 years ago Closed 8 years ago

Add Window.getInterface/QueryInterface to WebIDL

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: peterv, Assigned: peterv)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch v1 (obsolete) — Splinter Review
No description provided.
Attachment #8391154 - Flags: review?(bzbarsky)
Attached patch v1Splinter Review
Attachment #8391154 - Attachment is obsolete: true
Attachment #8391154 - Flags: review?(bzbarsky)
Attachment #8391155 - Flags: review?(bzbarsky)
Comment on attachment 8391155 [details] [diff] [review]
v1

>+  JS::Rooted<JSObject*> global(aCx, GetWrapper());

We need a "JSAutoCompartment ac(aCx, global);" after this line.

>+  if (!dom::WrapObject(aCx, global, result, iid, &v)) {

Don't we need to Throw() on aError if that fails?

It _might_ be nice to share code with nsXMLHttpRequest here (e.g. by having a templated utility method, or just a utility method that takes an nsIInterfaceRequestor* and an nsWrapperCache*).  I found both the above issues by comparing to that other method....

r=me with those fixed.
Attachment #8391155 - Flags: review?(bzbarsky) → review+
Blocks: 965153
Made an inline templated GetInterface method (which calls GetInterfaceImpl taking nsIInterfaceRequestor* and nsWrapperCache*).

http://hg.mozilla.org/integration/mozilla-inbound/rev/8aca83520a91
Comment on attachment 8391155 [details] [diff] [review]
v1

>+  nsCOMPtr<nsISupports> result;
>+  aError = GetInterface(*iid, getter_AddRefs(result));
In debug builds, ~nsGetterAddRefs calls Assert_NoQueryNeeded on the returned pointer, which will try to query to nsISupports, rather than iid, which might fail. You may want to use nsRefPtr instead.
(In reply to Boris Zbarsky [:bz] from comment #2)
> We need a "JSAutoCompartment ac(aCx, global);" after this line.

This actually seems wrong since we really want to wrap in the compartment of the caller, otherwise the security manager will block the wrapping (eg for non-WebIDL objects with no classinfo) if the caller is chrome but global is not.
Hmm.  Can we just pass a null scope then?  Or will that crash instead of using the current compartment of cx?

Basically, it seems really weird to me to be passing a scope and the JSContext in different compartments...
https://hg.mozilla.org/integration/mozilla-inbound/rev/39b430a304ca

(In reply to Boris Zbarsky [:bz] from comment #7)
> Hmm.  Can we just pass a null scope then?  Or will that crash instead of
> using the current compartment of cx?

We can't pass in null, but I guess we could pass in |JS::CurrentGlobalOrNull(aCx)| instead (bailing out if that returns null). What do you think?
Flags: needinfo?(bzbarsky)
> bailing out if that returns null

It better not return null!  This should only be called directly from the JS engine, no?

I'm ok passing CurrentGlobalOrNull(aCx).  That matches what I'm doing in bug 991742 anyway.  ;)
Flags: needinfo?(bzbarsky)
https://hg.mozilla.org/mozilla-central/rev/39b430a304ca
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.