1617084 - Consider adding some generic ancestor iterators.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, task, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1617084 - Add some simple DOM ancestor iterators. r=smaug

Comment 2

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

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... :/

Comment 4

[Emilio Cobos Álvarez (:emilio)]

(Though the mutation guard may be a worth benefit in exchange? idk)

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

(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

[Olli Pettay [:smaug][[e-mail]]]

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

[Boris Zbarsky [:bzbarsky]]

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

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1617084 - Return nullptr_t instead of a full iterator from end(). r=bzbarsky

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/28caac15f153
Add some simple DOM ancestor iterators. r=smaug

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a1d9de6e2675
Return nullptr_t instead of a full iterator from end(). r=bzbarsky

Comment 12

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/28caac15f153
https://hg.mozilla.org/mozilla-central/rev/a1d9de6e2675