Bug 120248 – Element child and descendant iterators

• Component: DOM
• Status: RESOLVED FIXED

[Antti Koivisto]

Better approach to iterating.

[Antti Koivisto]

Created attachment 209551 [details]
maybe something along these lines

[Andreas Kling]

Comment on attachment 209551 [details]
maybe something along these lines

I think this would be absolutely magical with C++11 range-based for loops.

I also think get() should return an Element& and we can have something like getPtr() for Element*. (This to encourage ourselves to use references.)

[Darin Adler]

Will look nice in C++11 for loops.

When we use element pointers, it’s well defined what happens if the document changes while we are iterating. For iterators, we’d want to define what happens in that case. What our hash tables do with iterators is that in debug versions they invalidate iterators when changes are made to the underlying data structure. Maybe we could do some variation on that.

[Darin Adler]

(In reply to comment #2)
> I also think get() should return an Element& and we can have something like getPtr() for Element*. (This to encourage ourselves to use references.)

C++ iterators normally use *iterator, not iterator.get().

[Andreas Kling]

Comment on attachment 209551 [details]
maybe something along these lines

View in context: https://bugs.webkit.org/attachment.cgi?id=209551&action=review

> Source/WebCore/dom/ElementTraversal.h:107
> +    bool operator!=(const ElementDescendantIterator& other) const { return m_current != other.m_current; }

This method could assert that both iterators have the same root.

[Antti Koivisto]

(In reply to comment #3)
> Will look nice in C++11 for loops.
> 
> When we use element pointers, it’s well defined what happens if the document changes while we are iterating. For iterators, we’d want to define what happens in that case. What our hash tables do with iterators is that in debug versions they invalidate iterators when changes are made to the underlying data structure. Maybe we could do some variation on that.

Yeah. I think we should assert that there are no tree mutations during iteration. Not just in the current subtree, in the entire tree.

[Darin Adler]

(In reply to comment #6)
> Yeah. I think we should assert that there are no tree mutations during iteration. Not just in the current subtree, in the entire tree.

Should probably go further and also assert that no DOM events were dispatched. An event handler could have mutated the tree; just because it didn’t actually happen does not mean the code is OK.

[Antti Koivisto]

Created attachment 209595 [details]
patch

[Antti Koivisto]

Created attachment 209596 [details]
another

[Early Warning System Bot]

Comment on attachment 209596 [details]
another

Attachment 209596 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1564277

[Early Warning System Bot]

Comment on attachment 209596 [details]
another

Attachment 209596 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1561584

[EFL EWS Bot]

Comment on attachment 209596 [details]
another

Attachment 209596 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1544952

[EFL EWS Bot]

Comment on attachment 209596 [details]
another

Attachment 209596 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1562424

[kov's GTK+ EWS bot]

Comment on attachment 209596 [details]
another

Attachment 209596 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1564290

[kov's GTK+ EWS bot]

Comment on attachment 209596 [details]
another

Attachment 209596 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1559658

[Antti Koivisto]

Created attachment 209603 [details]
yet another

[Sam Weinig]

Comment on attachment 209603 [details]
yet another

View in context: https://bugs.webkit.org/attachment.cgi?id=209603&action=review

> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj:-1
> -<?xml version="1.0" encoding="utf-8"?>

Removing this was probably a mistake.

[Antti Koivisto]

https://trac.webkit.org/r154581