Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename compartment-related terminology to use realms instead #25581

Closed
jdm opened this issue Jan 23, 2020 · 13 comments
Closed

Rename compartment-related terminology to use realms instead #25581

jdm opened this issue Jan 23, 2020 · 13 comments

Comments

@jdm
Copy link
Member

@jdm jdm commented Jan 23, 2020

This is specifically about components/script/compartments.rs and names like AlreadyInCompartment. Everywhere we use the name compartment, we should use the name realm instead.

@highfive
Copy link

@highfive highfive commented Jan 23, 2020

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

@kunalmohan
Copy link
Collaborator

@kunalmohan kunalmohan commented Jan 23, 2020

@highfive: assign me

@highfive highfive added the C-assigned label Jan 23, 2020
@highfive
Copy link

@highfive highfive commented Jan 23, 2020

Hey @kunalmohan! Thanks for your interest in working on this issue. It's now assigned to you!

@kunalmohan
Copy link
Collaborator

@kunalmohan kunalmohan commented Jan 23, 2020

@jdm should

per_compartment: {

be renamed too?

@jdm
Copy link
Member Author

@jdm jdm commented Jan 24, 2020

It turns out that preference can be removed; we don't actually use it anywhere in servo.

@kunalmohan
Copy link
Collaborator

@kunalmohan kunalmohan commented Jan 24, 2020

Can you explain (or provide a resource) what role preference values play and why don't we need it anymore? Are they, in any way, related to user preferences and how users want servo to operate on their system? I don't have much idea what it means here at this point 😅 . I see that it is being enabled here

"js.mem.gc.per_compartment.enabled": true,

And in the above comment, we are talking about removing just the 'per_compartment', right?

@kunalmohan
Copy link
Collaborator

@kunalmohan kunalmohan commented Jan 24, 2020

Ok, I think I get it. prefs.json sets the values for various limits and functionalities that servo should follow/use by default, right? (I think I have put it in a very vague manner)

@jdm
Copy link
Member Author

@jdm jdm commented Jan 24, 2020

There are no uses of pref!(js.mem.gc.per_compartment.enabled) in the rest of the servo code. There is a use of pref!(js.mem.gc.per_zone.enabled), so I think this is just a preference that used to be valid in an older version of spidermonkey, then spidermonkey switch to per-zone garbage collection and we added a new preference for it instead of renaming the existing one.

@jdm
Copy link
Member Author

@jdm jdm commented Jan 24, 2020

And yes, the values in prefs.json are read during Servo's startup and used for configurable values in Servo's code.

@kunalmohan
Copy link
Collaborator

@kunalmohan kunalmohan commented Jan 24, 2020

Ok. I have renamed the variables, functions, structs, etc. in compartments.rs and any other local variables in other files. I have removed the preference and renamed compartments.rs to realms.rs.
I found inCompartment descriptors in codegen/Bindings.conf I guess those are to be renamed too.

@jdm
Copy link
Member Author

@jdm jdm commented Jan 24, 2020

Yes please! That will require changing https://github.com/servo/servo/blob/master/components/script/dom/bindings/codegen/CodegenRust.py#L3400-L3401 and

self.inCompartmentMethods = [name for name in desc.get('inCompartments', [])]
as well.

@kunalmohan
Copy link
Collaborator

@kunalmohan kunalmohan commented Jan 24, 2020

Yes I did that. Now the only references remaining are
abcd
I don't think anything can be done about the first one (except renaming the imports).
I'm not sure about the others.

@jdm
Copy link
Member Author

@jdm jdm commented Jan 24, 2020

Agreed. All of those need to stay.

@kunalmohan kunalmohan mentioned this issue Jan 24, 2020
3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.