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 upMake labelable element .labels a live list in tree order #25424
Merged
Conversation
highfive
commented
Jan 3, 2020
|
Heads up! This PR modifies the following files:
|
|
@bors-servo try=wpt |
bors-servo
added a commit
that referenced
this pull request
Jan 3, 2020
Make labelable element .labels a live list in tree order This is not the highest-performance solution possible but it's visibly spec-aligned in a way a faster-performing implementation would be harder to verify, and I don't expect label-getting to deal with more than a few nodes at once in practice. I added a macro by analogy with some of the existing make_XXX_getter! macros; I will change it if it doesn't seem right. Remaining test failures are because keygen, shadow DOM, and ElementInternals are unimplemented. Shadow DOM should already be handled by the existing code when it is implemented, and keygen should just be able to add a labels_node_list and use the macro like the other labelable elements. ElementInternals labels are slightly different and might need another NodeList case. --- <!-- 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 #25391 <!-- Either: --> - [X] There are tests for these changes, although there's room for more (see web-platform-tests/wpt#21028) <!-- 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. -->
|
|
|
The only failures are three separate instances of #24726. |
bors-servo
added a commit
that referenced
this pull request
Jan 4, 2020
make Document.getElementsByName a live collection Another new case for NodeList; this and the labels live collection in #25424 are in the pipeline simultaneously, so one of them will need a merge resolution when the other one lands. Iterating over many same-named elements is potentially slower than it has to be, since getting the nth element from the live view always starts from the start of the tree. --- <!-- 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 #25147 <!-- 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. -->
|
Seems like Firefox uses a mutation observer to make the live list work efficiently. We do support them (I think), so this should be possible, but perhaps not necessary in this first pass. |
|
@bors-servo r+ |
|
|
bors-servo
added a commit
that referenced
this pull request
Jan 6, 2020
Make labelable element .labels a live list in tree order This is not the highest-performance solution possible but it's visibly spec-aligned in a way a faster-performing implementation would be harder to verify, and I don't expect label-getting to deal with more than a few nodes at once in practice. I added a macro by analogy with some of the existing make_XXX_getter! macros; I will change it if it doesn't seem right. Remaining test failures are because keygen, shadow DOM, and ElementInternals are unimplemented. Shadow DOM should already be handled by the existing code when it is implemented, and keygen should just be able to add a labels_node_list and use the macro like the other labelable elements. ElementInternals labels are slightly different and might need another NodeList case. --- <!-- 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 - [ ] `./mach test-tidy` does not report any errors - [X] These changes fix #25391 <!-- Either: --> - [X] There are tests for these changes, although there's room for more (see web-platform-tests/wpt#21028) <!-- 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. -->
|
(sorry about the double-force-push, I forgot test-tidy the first time and I had tabs) |
|
@bors-servo r=Manishearth |
|
|
bors-servo
added a commit
that referenced
this pull request
Jan 6, 2020
Make labelable element .labels a live list in tree order This is not the highest-performance solution possible but it's visibly spec-aligned in a way a faster-performing implementation would be harder to verify, and I don't expect label-getting to deal with more than a few nodes at once in practice. I added a macro by analogy with some of the existing make_XXX_getter! macros; I will change it if it doesn't seem right. Remaining test failures are because keygen, shadow DOM, and ElementInternals are unimplemented. Shadow DOM should already be handled by the existing code when it is implemented, and keygen should just be able to add a labels_node_list and use the macro like the other labelable elements. ElementInternals labels are slightly different and might need another NodeList case. --- <!-- 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 #25391 <!-- Either: --> - [X] There are tests for these changes, although there's room for more (see web-platform-tests/wpt#21028) <!-- 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 retry |
bors-servo
added a commit
that referenced
this pull request
Jan 6, 2020
Make labelable element .labels a live list in tree order This is not the highest-performance solution possible but it's visibly spec-aligned in a way a faster-performing implementation would be harder to verify, and I don't expect label-getting to deal with more than a few nodes at once in practice. I added a macro by analogy with some of the existing make_XXX_getter! macros; I will change it if it doesn't seem right. Remaining test failures are because keygen, shadow DOM, and ElementInternals are unimplemented. Shadow DOM should already be handled by the existing code when it is implemented, and keygen should just be able to add a labels_node_list and use the macro like the other labelable elements. ElementInternals labels are slightly different and might need another NodeList case. --- <!-- 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 #25391 <!-- Either: --> - [X] There are tests for these changes, although there's room for more (see web-platform-tests/wpt#21028) <!-- 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 was referenced Jan 6, 2020
bors-servo
added a commit
that referenced
this pull request
Feb 28, 2020
make Document.getElementsByName a live collection Another new case for NodeList; this and the labels live collection in #25424 are in the pipeline simultaneously, so one of them will need a merge resolution when the other one lands. Iterating over many same-named elements is potentially slower than it has to be, since getting the nth element from the live view always starts from the start of the tree. --- <!-- 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 #25147 <!-- 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 pull request
Feb 28, 2020
make Document.getElementsByName a live collection Another new case for NodeList; this and the labels live collection in #25424 are in the pipeline simultaneously, so one of them will need a merge resolution when the other one lands. Iterating over many same-named elements is potentially slower than it has to be, since getting the nth element from the live view always starts from the start of the tree. --- <!-- 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 #25147 <!-- 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
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.
pshaughn commentedJan 3, 2020
•
edited
This is not the highest-performance solution possible but it's visibly spec-aligned in a way a faster-performing implementation would be harder to verify, and I don't expect label-getting to deal with more than a few nodes at once in practice.
I added a macro by analogy with some of the existing make_XXX_getter! macros; I will change it if it doesn't seem right.
Remaining test failures are because keygen, shadow DOM, and ElementInternals are unimplemented. Shadow DOM should already be handled by the existing code when it is implemented, and keygen should just be able to add a labels_node_list and use the macro like the other labelable elements. ElementInternals labels are slightly different and might need another NodeList case.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors