Consider adding some generic ancestor iterators.
Categories
(Core :: DOM: Core & HTML, task, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox75 | --- | fixed |
People
(Reporter: emilio, Assigned: emilio)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
This will be a bit cleaner than bug 1613830, IMO, and they also allow asserting that the DOM isn't mutated while in use, which is nice.
Assignee | ||
Comment 1•5 years ago
|
||
Assignee | ||
Comment 2•5 years ago
|
||
Comment on attachment 9128028 [details]
Bug 1617084 - Add some simple DOM ancestor iterators. r=smaug
I wonder what do y'all think about something like this? Not necessarily against bug 1613830 and co, but I'm moderately sure this could be useful too on top.
Assignee | ||
Comment 3•5 years ago
|
||
Not necessarily a fan either... I guess it could be argued that if you don't know the DOM terminology it may be a bit harder to reason about what's going on... :/
Assignee | ||
Comment 4•5 years ago
|
||
(Though the mutation guard may be a worth benefit in exchange? idk)
Comment 5•5 years ago
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #0)
This will be a bit cleaner than bug 1613830, IMO, and they also allow asserting that the DOM isn't mutated while in use, which is nice.
bug 1613830 is super clean and clear :)
Iterating using GetParentNode() feels more natural given that it is a normal DOM method.
And in cases like
auto* element : InclusiveLightTreeAncestorsOfType<nsGenericHTMLElement>(*this)
it isn't super clear what the type is. I guess that is more about just the normal issue about using 'auto'
(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)
Not necessarily a fan either... I guess it could be argued that if you don't know the DOM terminology it may be a bit harder to reason about what's going on... :/
Or rather, if one knows the old and clear parentNode terminology, this may make the code harder to understand.
And lifetime management gets hidden inside the iterator. This is the biggest worry I have.
But then, I can see this being quite nice in some cases. Though the examples in the patch don't really get better.
I don't have strong opinion on this one.
Comment 6•5 years ago
|
||
Comment on attachment 9128028 [details]
Bug 1617084 - Add some simple DOM ancestor iterators. r=smaug
feedback given :)
I need to think still some more lifetime management .
Comment 7•5 years ago
|
||
Comment on attachment 9128028 [details]
Bug 1617084 - Add some simple DOM ancestor iterators. r=smaug
I like the general idea, and in particular the automatic mutation-guard aspect.
I'm not a big fan of the verbosity. I wonder whether we can do better there. And yes, I realize the verbosity matches spec language....
Comment 8•5 years ago
|
||
Comment on attachment 9128028 [details]
Bug 1617084 - Add some simple DOM ancestor iterators. r=smaug
In editor module, we need to climb up the DOM tree from a content node and some loops are not simple because of historical reasons (I meant some of them don't use modern API yet). Additionally, the mutation guard in debug build looks nice because there are some loops which collect nodes to be handled with comments, but the wrong usage will be detectable with automated tests. So, looks nice to me except the issue I mentioned in bug 1613830 comment 11. In editor module, some loops forgot checking editing host and that cause some editing operation changed non-editable nodes, so, it's greater if there is an optional argument to stop the iteration when it reaches the node.
Updated•5 years ago
|
Assignee | ||
Comment 9•5 years ago
|
||
This is legal with C++17. It's not too important for the ancestor iterators
because they're just a pointer anyway, but it's nice for
ShadowIncludingTreeIterator, which has an AutoTArray and what not.
Depends on D63594
Comment 10•5 years ago
|
||
Comment 11•5 years ago
|
||
Comment 12•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/28caac15f153
https://hg.mozilla.org/mozilla-central/rev/a1d9de6e2675
Description
•