Make name content attributes consistently atoms and put them in rare_data for fast access#25572
Merged
bors-servo merged 1 commit intoservo:masterfrom Feb 13, 2020
Merged
Conversation
|
Heads up! This PR modifies the following files:
|
highfive
added
the
S-awaiting-review
Collaborator
|
Opened new PR for upstreamable changes. Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#21332. |
Contributor
Author
|
I am sure this will have some idlharness test result changes, and I wouldn't be surprised if filling in the name setter has had other effects. |
Contributor
bors-servo
pushed a commit
that referenced
this pull request
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. -->
Contributor
|
💔 Test failed - status-taskcluster |
Contributor
Author
|
That one /css/css-transforms/animation/perspective-interpolation.html crash looks scary. Other new failures are probably because we were only "accidentally" passing before, but of course I'll have to look through them all to be sure. |
Contributor
Author
|
Oh, I was reading upwards from the bottom and missed the known-intermittents header, that one test already has an intermittency issue filed for it. Now to look at all the real new failures... |
Contributor
Author
|
Once I was reading the right part of the results, it was all new passes and completely consistent with what I changed. I notice the CI test harness does a better job of extracting results from a timed-out test than I get running on the command line; the _mozilla test I added is not actually necessary for CI purposes. |
Contributor
Author
|
I believe this is ready to roll out. |
Collaborator
|
Transplanted upstreamable changes to existing PR. Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#21332. |
Contributor
bors-servo
pushed a commit
that referenced
this pull request
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. -->
Contributor
|
💔 Test failed - status-taskcluster |
Member
|
That's #23290. |
4 tasks
bors-servo
pushed a commit
that referenced
this pull request
Jan 22, 2020
Add SupportedPropertyNames to Document (also fix iframe getting) Existing test of named-getting an iframe now succeeds. I added a new test for Object.getOwnPropertyNames(document) based on my understanding of the spec; that test could use a second opinion. UPDATE: This was trying to do too many things in one PR as originally submitted. It is now using #25572 as a base, and I suggest reviewing that PR before this one to avoid duplicating review effort. --- <!-- 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 #7273 for all implemented named getters, fix #25146, and fix the iframe case only of #25145. <!-- 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. -->
jdm
approved these changes
Feb 12, 2020
Member
jdm
left a comment
jdm
left a comment
There was a problem hiding this comment.
Thanks for doing this work! I'm impressed by how many places ended up needing to be modified.
jdm
added
S-needs-code-changes
Member
|
This needs a |
Collaborator
|
Transplanted upstreamable changes to existing PR. Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#21332. |
Member
|
@bors-servo r+ |
Contributor
|
📌 Commit 02a3e59 has been approved by |
highfive
added
S-awaiting-merge
Contributor
bors-servo
pushed a commit
that referenced
this pull request
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. -->
Contributor
Author
|
The spec sometimes explicitly excludes the empty-string from being a possible name and sometimes doesn't; there aren't WPT tests on the subject so I don't know what browsers do in various name="" cases, and empty-string handling here might not be great. |
Contributor
|
☀️ Test successful - status-taskcluster |
Contributor
|
👀 Test was successful, but fast-forwarding failed: 422 Update is not a fast forward |
Contributor
Author
|
I don't know what that means but it sounds like something a rebase might fix. |
highfive
added
S-awaiting-review
Collaborator
|
Transplanted upstreamable changes to existing PR. Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#21332. |
Member
|
@bors-servo r+ |
Contributor
|
📌 Commit f29e22f has been approved by |
highfive
added
S-awaiting-merge
Contributor
bors-servo
pushed a commit
that referenced
this pull request
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. -->
Contributor
|
☀️ Test successful - status-taskcluster |
highfive
removed
the
S-awaiting-merge
bors-servo
pushed a commit
that referenced
this pull request
Feb 13, 2020
Add SupportedPropertyNames to Document (also fix iframe getting) Existing test of named-getting an iframe now succeeds. I added a new test for Object.getOwnPropertyNames(document) based on my understanding of the spec; that test could use a second opinion. UPDATE: This was trying to do too many things in one PR as originally submitted. It is now using #25572 as a base, and I suggest reviewing that PR before this one to avoid duplicating review effort. --- <!-- 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 #7273 for all implemented named getters, fix #25146, and fix the iframe case only of #25145. <!-- 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
pushed a commit
that referenced
this pull request
Feb 14, 2020
Add SupportedPropertyNames to Document (also fix iframe getting) Existing test of named-getting an iframe now succeeds. I added a new test for Object.getOwnPropertyNames(document) based on my understanding of the spec; that test could use a second opinion. UPDATE: This was trying to do too many things in one PR as originally submitted. It is now using #25572 as a base, and I suggest reviewing that PR before this one to avoid duplicating review effort. --- <!-- 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 #7273 for all implemented named getters, fix #25146, and fix the iframe case only of #25145. <!-- 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. -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors