Skip to content

/ servo

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 JS<T> to Dom<T>, Root<T> to DomRoot<T>, and other things #18635

Merged
merged 9 commits into from Sep 26, 2017
Merged

Rename JS<T> to Dom<T>, Root<T> to DomRoot<T>, and other things #18635

merged 9 commits into from Sep 26, 2017

Conversation

@nox
Copy link
Member

nox commented Sep 26, 2017 •
edited by SimonSapin

This change is Reviewable
nox added 2 commits Sep 25, 2017
@nox
Fix some messages in the tidy script
dafcd82 
It's MutJS<T>, not MutJS<JS<T>>.
@nox
Rename dom::bindings::js to dom::bindings::root
0e3c54c
@highfive
Copy link

highfive commented Sep 26, 2017

Heads up! This PR modifies the following files:

  • @wafflespeanut: python/tidy/servo_tidy/tidy.py, python/tidy/servo_tidy_tests/ban.rs, python/tidy/servo_tidy_tests/test_tidy.py, python/tidy/servo_tidy_tests/ban-domrefcell.rs
  • @KiChjang: components/script/dom/request.rs, components/script/test.rs, components/script/dom/workerlocation.rs, components/script/dom/xmldocument.rs, components/script/dom/textencoder.rs and 298 more
  • @edunham: python/tidy/servo_tidy/tidy.py, python/tidy/servo_tidy_tests/ban.rs, python/tidy/servo_tidy_tests/test_tidy.py, python/tidy/servo_tidy_tests/ban-domrefcell.rs
  • @fitzgen: components/script/dom/request.rs, components/script/test.rs, components/script/dom/workerlocation.rs, components/script/dom/xmldocument.rs, components/script/dom/textencoder.rs and 298 more
  • @emilio: components/script/dom/webglframebuffer.rs, components/layout/wrapper.rs, components/script/dom/webglbuffer.rs, components/script/dom/webgl_extensions/wrapper.rs, components/script/dom/webgltexture.rs and 18 more
@highfive
Copy link

highfive commented Sep 26, 2017

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@emilio
emilio approved these changes Sep 26, 2017
View changes
Copy link
Member

emilio left a comment

rs=me, given you and @jdm discussed this yesterday, and this looks fine to me.

Kinda unfortunate that we have both Dom and DOM stuff though...
components/script/docs/JS-Servos-only-GC.md Outdated
@@ -164,8 +164,8 @@ relationship. The `Document` just has a pointer to a `Window`, one of many
pointers to that object, which can live in native DOM data structures or in
JavaScript objects. These are precisely the pointers we need to tell the
garbage collector about. We do this with a
[custom type for traced pointers: `JS<T>`][js] (for example, the `JS<Window>`
above). The implementation of `trace` for `JS<T>` is not auto-generated; this
[custom type for traced pointers: `Dom<T>`][js] (for example, the `Dom<Window>`

This comment has been minimized.

Sign in to view
@emilio

emilio Sep 26, 2017

Member

nit: Maybe change the link name too? (right now it's called js).

This comment has been minimized.

Sign in to view
@nox

nox Sep 26, 2017

Author Member

Done.
nox added 7 commits Sep 25, 2017
@nox
Rename JS<T> to Dom<T>
7be32fb
@nox
Rename MutJS<T> to MutDom<T>
d293350
@nox
Rename MutNullableJS<T> to MutNullableDom<T>
c52fd0a
@nox
Rename LayoutJS<T> to LayoutDom<T>
e2dac78
@nox
Rename OnceCellJS<T> to DomOnceCell<T>
9a7ba89 
Like DOMRefCell<T>.
@nox
Rename DOMRefCell<T> to DomRefCell<T>
5773707 
I don't want to do such a gratuitous rename, but with all the other types
now having "Dom" as part of their name, and especially with "DomOnceCell",
I feel like the other cell type that we already have should also follow
the convention. That argument loses weight though when we realise there
is still DOMString and other things.
@nox
Rename Root<T> to DomRoot<T>
Loading status checks…
f87c2a8 
In a later PR, DomRoot<T> will become a type alias of Root<Dom<T>>,
where Root<T> will be able to handle all the things that need to be
rooted that have a stable traceable address that doesn't move for the
whole lifetime of the root. Stay tuned.
@nox nox force-pushed the RENAME-ALL-THE-THINGS branch from bef6e1b to f87c2a8 Sep 26, 2017
@nox
Copy link
Member Author

nox commented Sep 26, 2017

@bors-servo r=emilio p=3
@bors-servo
Copy link
Contributor

bors-servo commented Sep 26, 2017

📌 Commit f87c2a8 has been approved by emilio
@highfive highfive assigned emilio and unassigned cbrewster Sep 26, 2017
@bors-servo
Copy link
Contributor

bors-servo commented Sep 26, 2017

⌛ Testing commit f87c2a8 with merge 1282e0d...
bors-servo added a commit that referenced this pull request Sep 26, 2017
@bors-servo
Auto merge of #18635 - servo:RENAME-ALL-THE-THINGS, r=emilio
Loading status checks…
1282e0d 
Rename JS<T> to Dom<T>, Root<T> to DomRoot<T>, and other things

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18635)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 26, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: emilio
Pushing 1282e0d to master...
@bors-servo bors-servo mentioned this pull request Sep 26, 2017
Merged
4 of 5 tasks complete
@bors-servo bors-servo merged commit f87c2a8 into master Sep 26, 2017
4 checks passed
4 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
dependency-ci Dependencies checked
Details
homu Test successful
Details
@bors-servo bors-servo mentioned this pull request Sep 26, 2017
Merged
4 of 4 tasks complete
@nox nox deleted the RENAME-ALL-THE-THINGS branch Sep 26, 2017
@bors-servo bors-servo mentioned this pull request Sep 26, 2017
Merged
3 of 5 tasks complete
This was referenced Sep 26, 2017
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilio emilio

Assignees

@emilio emilio

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@nox @highfive @bors-servo @emilio @cbrewster
You can’t perform that action at this time.