Closed Bug 577438 Opened 12 years ago Closed 11 years ago

Make NodeIterator and TreeWalker not use GetChildAt


(Core :: DOM: Core & HTML, defect)

Not set





(Reporter: sicking, Assigned: craig.topper)


(Whiteboard: [approved-patches-landed])


(3 files, 2 obsolete files)

Both these classes can probably be significantly simplified, and be made more performant, by using the new GetFirstChild/GetNextSibling functions on nsINode, rather than the old GetChildAt/GetChildCount functions.

We might even be able to use nsINode::GetNextNode, though there is no equivalent for iterating backwards.
We can easily add a backwards version.  Backwards is actually simpler than forwards, since backwards is just "get previous sibling; if there is one, drill down its GetLastChild chain and if not return our parent" or so, right?
Yep, that sounds right. Might also need to compare |this| to |aRoot| and return null if they are the same.
Need to add previous sibling so that we can stop using aIndexInContainer in the mutation observer in nsNodeIterator.
Attachment #456523 - Flags: review?(jonas)
Attachment #456523 - Flags: review?(jonas) → review+
Attachment #456540 - Flags: review?(jonas)
Attachment #456540 - Flags: review?(jonas)
Attachment #456540 - Flags: review?(jonas)
Comment on attachment 456540 [details] [diff] [review]
Part 2: NodeIterator

It'd be nice to get rid of NodePointer::MoveForward and MoveBackward and just use nsINode::GetNextNode (and something similar for backwards iteration). However it does seem tricky given what we need in AdjustAfterRemoval.

r=me in any case. Thanks a lot for doing this!
Attachment #456540 - Flags: review?(jonas) → review+
Recoded to not use GetChildAt and also removed recursion.
Attachment #456772 - Flags: review?(jonas)
Comment on attachment 456772 [details] [diff] [review]
Part 3: TreeWalker

r=me, but would love to see some tests!
Attachment #456772 - Flags: review?(jonas) → review+
Keywords: checkin-needed
Seems I forgot to rev the IID on nsIMutationObserver.
Attachment #458887 - Attachment is obsolete: true
This needs approval2.0 to land.
Keywords: checkin-needed
So this should be "RESOLVED FIXED", right?
Flags: in-testsuite-
Well I left it open cause I was going to write some tests as requested.
I'd suggest resolving this bug and filing a new one for tests.
Whiteboard: [approved-patches-landed]
Resolving as suggested.
Closed: 11 years ago
Resolution: --- → FIXED
Component: DOM: Traversal-Range → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.