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 upRadioNodeList liveness #25435
RadioNodeList liveness #25435
Conversation
highfive
commented
Jan 5, 2020
|
Heads up! This PR modifies the following files:
|
|
There are still more test failures than I'd expect. It might be better to leave this one unmerged until I've looked into them more. |
|
Just making sure, is there any overlap between this PR and #22264? |
|
I was unaware of #22264, @KiChjang. I've been treating the different subcases of live NodeLists as different problems and have been working on them in this, #25424, and #25428. It looks like I've been following a very different strategy from you, and #25424 passes some tests that #22264 doesn't. I understand that you might feel you have priority on this and that I should step away from it to let you keep working on your version. If you do feel that way, let me know and I'll back off; I didn't intend to invalidate anyone else's work. |
|
Related issues that this change from static to live doesn't solve: #25429 (an element is staying gettable too long because it's not leaving the past names map), #21495 (an element with a |
|
It occurs to me that, since this is always looking at form.controls and the past names map and never at the DOM tree directly, it could be optimized with a cursor like #22264 without even needing a mutation observer. It would just need to invalidate or repair the cursor on every code path that changes form.controls or the past names map. |
|
This push fixes the one bug I had that is within the purview of the list-getting itself (I wasn't excluding input type=image on the check that had to exclude it); it does not take into account the idea of using a cursor for performance. |
|
|
|
r? @Manishearth |
| } | ||
| } | ||
|
|
||
| pub fn iter<'a>(&'a self) -> impl Iterator<Item = DomRoot<Node>> + 'a { | ||
| let len = self.Length(); | ||
| // There is room for optimization here in non-simple cases, | ||
| // as calling Item repeatedly on a live list can involve redundant work. |
This comment has been minimized.
This comment has been minimized.
|
We should probably have a followup filed to move everything to mutation observers |
| } | ||
| if candidates_length == 0 { | ||
| candidates = RadioNodeList::new_images(&window, self, name.clone()); | ||
| candidates_length = candidates.Length(); |
This comment has been minimized.
This comment has been minimized.
Manishearth
Jan 10, 2020
Member
we could use a separate function that just returns if there are zero, 1, or more (returning the element if it's one)
|
We should probably have a followup filed to move everything to mutation observers |
pshaughn
force-pushed the
pshaughn:radio
branch
2 times, most recently
from
1f42647
to
22513ac
|
|
…rformance optimization
|
@bors-servo r=Manishearth |
|
|
RadioNodeList liveness When the named getter of a form's .elements gets a list, that list is now a live view into the form, passing a WPT test that removes a node and then looks at the list again. --- <!-- 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 #25415 (GitHub issue number if applicable) <!-- 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. -->
|
|
pshaughn commentedJan 5, 2020
•
edited
When the named getter of a form's .elements gets a list, that list is now a live view into the form, passing a WPT test that removes a node and then looks at the list again.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors