Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upHandle element names consistently #25570
Closed
Assignees
Labels
Comments
This was referenced Jan 21, 2020
|
https://github.com/pshaughn/servo/tree/atomnames is probably doing this, but it's not testable through WPT alone since it's an unobservable-unless-something-else-breaks internal representation. I have an idea for how to unit-test it, but since I've never had to write a Servo unit test before, this might take me a while. |
|
On closer reading, I can't really make a DOM object from a unit test without inventing a bunch of new test infrastructure, and I've thought of a way to do what I want with a debug_assert! and a WPT test. |
bors-servo
added a commit
that referenced
this issue
Jan 22, 2020
Make name content attributes consistently atoms and put them in rare_data for fast access <!-- Please describe your changes on the following line: --> All codepaths setting the name content attribute now use an atom, which is also stored in rare_data for direct lookup by a get_name method. Paralleling the get_name method, I added a get_id method, which makes some internal id-lookup cases nicer. A new test tests for a name setter on every HTML element type. In addition to its overt and upstreamable purpose of checking IDL property reflection semantics, for us this test also hits some Servo assertions that make sure the name is an atom in every case. If the test doesn't crash, even a failed test case still has the attribute as an atom rather than some other type. The failed cases are for elements that we have unimplemented or completely stubbed; I added a few missing name IDL properties to otherwise implemented elements. I also made a _mozilla copy of the working part of an existing but not-working test, as I explain in #25057 (comment) --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #25570 and make progress on #25057 <!-- Either: --> - [X] There are tests for these changes <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo
added a commit
that referenced
this issue
Jan 22, 2020
Make name content attributes consistently atoms and put them in rare_data for fast access <!-- Please describe your changes on the following line: --> All codepaths setting the name content attribute now use an atom, which is also stored in rare_data for direct lookup by a get_name method. Paralleling the get_name method, I added a get_id method, which makes some internal id-lookup cases nicer. A new test tests for a name setter on every HTML element type. In addition to its overt and upstreamable purpose of checking IDL property reflection semantics, for us this test also hits some Servo assertions that make sure the name is an atom in every case. If the test doesn't crash, even a failed test case still has the attribute as an atom rather than some other type. The failed cases are for elements that we have unimplemented or completely stubbed; I added a few missing name IDL properties to otherwise implemented elements. I also made a _mozilla copy of the working part of an existing but not-working test, as I explain in #25057 (comment) --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #25570 and make progress on #25057 <!-- Either: --> - [X] There are tests for these changes <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo
added a commit
that referenced
this issue
Feb 12, 2020
Make name content attributes consistently atoms and put them in rare_data for fast access <!-- Please describe your changes on the following line: --> All codepaths setting the name content attribute now use an atom, which is also stored in rare_data for direct lookup by a get_name method. Paralleling the get_name method, I added a get_id method, which makes some internal id-lookup cases nicer. A new test tests for a name setter on every HTML element type. In addition to its overt and upstreamable purpose of checking IDL property reflection semantics, for us this test also hits some Servo assertions that make sure the name is an atom in every case. If the test doesn't crash, even a failed test case still has the attribute as an atom rather than some other type. The failed cases are for elements that we have unimplemented or completely stubbed; I added a few missing name IDL properties to otherwise implemented elements. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #25570 and make progress on #25057 <!-- Either: --> - [X] There are tests for these changes <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo
added a commit
that referenced
this issue
Feb 13, 2020
Make name content attributes consistently atoms and put them in rare_data for fast access <!-- Please describe your changes on the following line: --> All codepaths setting the name content attribute now use an atom, which is also stored in rare_data for direct lookup by a get_name method. Paralleling the get_name method, I added a get_id method, which makes some internal id-lookup cases nicer. A new test tests for a name setter on every HTML element type. In addition to its overt and upstreamable purpose of checking IDL property reflection semantics, for us this test also hits some Servo assertions that make sure the name is an atom in every case. If the test doesn't crash, even a failed test case still has the attribute as an atom rather than some other type. The failed cases are for elements that we have unimplemented or completely stubbed; I added a few missing name IDL properties to otherwise implemented elements. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #25570 and make progress on #25057 <!-- Either: --> - [X] There are tests for these changes <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Some SetName methods are make_setter!, some are make_atomic_setter!, and HTMLIFrameElement has its own DomRefCell reflecting its name. This inconsistency is a contributing factor in #25057, and cleaning it up is necessary to make a #25548 that won't crash.
I had changed the Element parent class to make names atoms in #25548, reflecting them in rare_data for lookup speed, but I had neglected to consider the differences between different Element subclasses there, and now that I look at all the specific ways different elements use names, a sweep to unify them all seems to be in order.