Closed Bug 868633 Opened 7 years ago Closed 6 years ago

Remove JS_GetGlobalObject from caps

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(1 file)

No description provided.
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.
Attachment #745408 - Flags: review?(gkrizsanits)
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.
(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 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...
Attachment #745408 - Flags: review?(gkrizsanits) → review+
https://hg.mozilla.org/mozilla-central/rev/60201d176dd2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.