Closed Bug 1461292 Opened 2 years ago Closed 2 years ago

Rename *AutoCompartment to *AutoRealm

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(4 files)

It would be nice to rename JSAutoCompartment to JS::AutoRealm, but we also have our internal js::AutoCompartment and merging these is not trivial. So I'll just rename to JSAutoRealm and js::AutoRealm.
The hardest part here is what to do with some of the comments mentioning "compartment". I changed the obvious ones to "realm", but I didn't touch the ones I was less confident about.

Boris, I expect you will have to audit these at some point anyway... Another option is to keep JSAutoCompartment as a typedef and try to limit my changes to js/src for now and leave the rest to you, but that might be more confusing to people.
Attachment #8975441 - Flags: review?(luke)
Attachment #8975441 - Flags: review?(bzbarsky)
Attachment #8975441 - Flags: review?(luke) → review+
Attachment #8975747 - Flags: review?(luke) → review+
Comment on attachment 8975448 [details] [diff] [review]
Part 2 - Rename JSAutoNullableCompartment to JSAutoNullableRealm

r=me
Attachment #8975448 - Flags: review?(bzbarsky) → review+
Comment on attachment 8975441 [details] [diff] [review]
Part 1 - Rename JSAutoCompartment to JSAutoRealm

> CustomElementRegistry::Define

There's all sorts of weirdness here that should just go away; I'll do that on top of your patch.  Filed bug 1461711 to track that.

>+++ b/dom/base/nsFrameMessageManager.cpp

The comments here about leaking until shutdown should probably s/compartment/global/ or s/compartment/realm/.

>+++ b/dom/base/nsObjectLoadingContent.cpp
>+  // NB: We need a JSAutoRealm because we can be called from nsPluginFrame

We can't be called from nsPluginFrame, and afaict the only caller is nsObjectLoadingContent::SetupProtoChain which already has cx in the obj realm.  So I think we can remove this comment and this JSAutoRealm.

>+++ b/dom/bindings/BindingUtils.cpp
>+  JSAutoRealm newAc(aCx, newParent);

newAr?

>+++ b/dom/bindings/CallbackObject.h
>+    // pop the JSContext. Though in practice we'll often manually order

There is no more JSContext stack.  This should probably say:

  // pop the script settings stack. Though in practice ....

>+++ b/dom/bindings/Codegen.py
>@@ -16679,27 +16679,27 @@ class CGMaplikeOrSetlikeHelperFunctionGe
>+            JSAutoRealm tempCompartment(cx, UnprivilegedJunkScopeOrWorkerGlobal());

tempRealm?

>+            JSAutoRealm reflectorCompartment(cx, obj);

reflectorRealm.

>+++ b/js/xpconnect/src/xpcprivate.h
>+    mozilla::Maybe<JSAutoRealm> mAutoCompartment;

mAutoRealm?

>+++ b/js/xpconnect/wrappers/XrayWrapper.cpp
>     // The expando lives in the target's compartment, so do our installation there.

"target's realm"?

r=me on "the bits outside js/" + js/xpconnect
Attachment #8975441 - Flags: review?(bzbarsky) → review+
Thanks for the reviews!

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #5)
> We can't be called from nsPluginFrame, and afaict the only caller is
> nsObjectLoadingContent::SetupProtoChain which already has cx in the obj
> realm.  So I think we can remove this comment and this JSAutoRealm.

I'll post a follow-up patch for this one to minimize risk.
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b6365bdb4c2
part 1 - Rename JSAutoCompartment to JSAutoRealm. r=bz,luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/1096b51e73ed
part 2 - Rename JSAutoNullableCompartment to JSAutoNullableRealm. r=bz
Keywords: leave-open
I also removed the now-unused |obj| argument.
Attachment #8976060 - Flags: review?(bzbarsky)
Attachment #8976060 - Flags: review?(bzbarsky) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf1b00c73d57
part 3 - Rename AutoCompartment to AutoRealm. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/f050b09ee268
part 4 - Remove unnecessary JSAutoRealm from GetPluginJSObject. r=bz
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/cf1b00c73d57
https://hg.mozilla.org/mozilla-central/rev/f050b09ee268
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.