868633 - Remove JS_GetGlobalObject from caps

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Comment 1

[Bobby Holley (:bholley)]

Attachment: Remove JS_GetGlobalObject from caps. v1

This code is an nsDOMClassInfo callback that doesn't care about the cx per se,
and just wants to operate in the scope of the active global of the
nsIScriptContext. We have a more direct way of doing that.

Comment 2

[Gabor Krizsanits (INACTIVE)]

I'm not sure why you want to get away from using AutoPushJSContext here... I'm glad to see the enter compartment, and not too scared from using AutoJSContext either, just want to understand the reason of that change, simply out of curiosity.

Comment 3

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #2)
> I'm not sure why you want to get away from using AutoPushJSContext here...
> I'm glad to see the enter compartment, and not too scared from using
> AutoJSContext either, just want to understand the reason of that change,
> simply out of curiosity.

Well, from my reading of the function, there's no dependence on the JSContext per se. The only reason we currently need to push here is so that we end up with the NativeGlobal() from that cx, which we then retrieve using JS_GetGlobalObject(cx). But we can just as easily get that global in a more direct manner.

AutoPushJSContext means "make sure that this JSContext is at the top of the stack", which is the kind of thing we should try to remove as we converge on SingleCX. AutoJSContext says "just give me whatever cx is already stack-top," which is totally safe as long as we enter a compartment afterwards.

Comment 4

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 745408 [details] [diff] [review]
Remove JS_GetGlobalObject from caps. v1

Review of attachment 745408 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Bobby Holley (:bholley) from comment #3)
> which is the kind of thing we should try to remove

Yeah I get that part. I was just wondering why starting it in this patch, but I guess the sooner the better...

Comment 5

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/60201d176dd2

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/60201d176dd2