Closed Bug 1466121 Opened 2 years ago Closed 2 years ago

Rename JSCompartment to JS::Compartment, split vm/JSCompartment.h/cpp

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(7 files)

At some point we should rename JSCompartment to JS::Compartment (then we'll have JS::Realm, JS::Compartment, JS::Zone).

We should also split vm/JSCompartment.* in vm/Realm.* and vm/Compartment.*
Mostly very mechanical, except I fixed up some formatting and changed some comments to say realm instead of compartment.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8984065 - Flags: review?(luke)
For consistency with JS::Realm, make JS::Compartment and JS::Zone classes instead of structs.
Attachment #8984070 - Flags: review?(luke)
I think the next step (after these patches land) is to:

* merge vm/Realm.cpp into vm/JSCompartment.cpp
* hg mv vm/JSCompartment.cpp/h => vm/Compartment.cpp/h
* hg cp vm/Compartment.cpp/h => vm/Realm.cpp/h
  and remove the Compartment code from Realm.cpp/h and remove the Realm code from Compartment.cpp/h
Attachment #8984065 - Flags: review?(luke) → review+
Comment on attachment 8984070 [details] [diff] [review]
Part 2 - Make Compartment and Zone classes instead of structs

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

Nice!
Attachment #8984070 - Flags: review?(luke) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b640dc9b8998
part 1 - Rename JSCompartment to JS::Compartment. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3b458961e04
part 2 - Make Compartment and Zone classes instead of structs. r=luke
Keywords: leave-open
Taking one step back, so we can take two steps forward...
Attachment #8984376 - Flags: review?(luke)
Just a |hg cp| and then I removed code from both files.

The #includes are the hard part because it's not clear which ones we need where. I fixed the obvious ones.
Attachment #8984379 - Flags: review?(luke)
I'll probably land each of these parts separately because renaming/adding files can have weird side effects due to unified builds...
Attachment #8984376 - Flags: review?(luke) → review+
Attachment #8984377 - Flags: review?(luke) → review+
Comment on attachment 8984379 [details] [diff] [review]
Part 5 - Split Compartment.h from Realm.h

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

Nice job not losing the history.
Attachment #8984379 - Flags: review?(luke) → review+
Attachment #8984380 - Flags: review?(luke) → review+
Attachment #8984381 - Flags: review?(luke) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4db98fad2f4e
part 3 - Merge Realm.cpp into JSCompartment.cpp to simplify later patches. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac87103cdf38
part 4 - Rename vm/JSCompartment* to vm/Realm*. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/af265d75fc00
part 5 - Split Compartment.h from Realm.h. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/45e231683dbf
part 6 - Split Compartment.cpp from Realm.cpp. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/cceb75ca1a1d
part 7 - Split Compartment-inl.h from Realm-inl.h. r=luke
(In reply to Jan de Mooij [:jandem] from comment #12)
> I'll probably land each of these parts separately because renaming/adding
> files can have weird side effects due to unified builds...

I decided not to do this because the stack of patches was green on Try and splitting them up now is probably a bigger risk. Also if there is some weird Talos regression, it's easy enough to bisect on Try because separate revisions.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.