Peter Van der Beken [:peterv] Assignee
	Description • 10 years ago

      No description provided.

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 10 years ago

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.

	Peter Van der Beken [:peterv] Assignee
	Comment 3 • 10 years ago

Made an inline templated GetInterface method (which calls GetInterfaceImpl taking nsIInterfaceRequestor* and nsWrapperCache*).

http://hg.mozilla.org/integration/mozilla-inbound/rev/8aca83520a91

	neil@parkwaycc.co.uk
	Comment 4 • 10 years ago

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.

	Peter Van der Beken [:peterv] Assignee
	Comment 5 • 10 years ago

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/3b1a35bd4e3f.

	Peter Van der Beken [:peterv] Assignee
	Comment 6 • 10 years ago

(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.

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 10 years ago

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...

	Peter Van der Beken [:peterv] Assignee
	Comment 8 • 10 years ago

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?

	Boris Zbarsky [:bzbarsky]
	Comment 9 • 10 years ago

> 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.  ;)

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/39b430a304ca

	Carsten Book [:Tomcat]
	Comment 11 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/d7b7dbe3c9d3