Closed
Bug 577438
Opened 14 years ago
Closed 13 years ago
Make NodeIterator and TreeWalker not use GetChildAt
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
People
(Reporter: sicking, Assigned: craig.topper)
Details
(Whiteboard: [approved-patches-landed])
Attachments
(3 files, 2 obsolete files)
9.80 KB,
patch
|
sicking
:
review+
sicking
:
approval2.0+
|
Details | Diff | Splinter Review |
28.36 KB,
patch
|
sicking
:
review+
sicking
:
approval2.0+
|
Details | Diff | Splinter Review |
36.96 KB,
patch
|
sicking
:
approval2.0+
|
Details | Diff | Splinter Review |
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.
Comment 1•14 years ago
|
||
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?
Reporter | ||
Comment 2•14 years ago
|
||
Yep, that sounds right. Might also need to compare |this| to |aRoot| and return null if they are the same.
Assignee | ||
Comment 3•14 years ago
|
||
Need to add previous sibling so that we can stop using aIndexInContainer in the mutation observer in nsNodeIterator.
Attachment #456523 -
Flags: review?(jonas)
Reporter | ||
Updated•14 years ago
|
Attachment #456523 -
Flags: review?(jonas) → review+
Assignee | ||
Comment 4•14 years ago
|
||
Attachment #456540 -
Flags: review?(jonas)
Assignee | ||
Updated•14 years ago
|
Attachment #456540 -
Flags: review?(jonas)
Assignee | ||
Updated•14 years ago
|
Attachment #456540 -
Flags: review?(jonas)
Reporter | ||
Comment 5•14 years ago
|
||
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+
Assignee | ||
Comment 6•14 years ago
|
||
Recoded to not use GetChildAt and also removed recursion.
Attachment #456772 -
Flags: review?(jonas)
Reporter | ||
Comment 7•14 years ago
|
||
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+
Assignee | ||
Comment 8•14 years ago
|
||
Attachment #456523 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 9•14 years ago
|
||
Seems I forgot to rev the IID on nsIMutationObserver.
Attachment #458887 -
Attachment is obsolete: true
Reporter | ||
Updated•14 years ago
|
Attachment #456540 -
Flags: approval2.0+
Reporter | ||
Updated•14 years ago
|
Attachment #456772 -
Flags: approval2.0+
Reporter | ||
Updated•14 years ago
|
Attachment #458925 -
Flags: approval2.0+
Assignee | ||
Comment 11•14 years ago
|
||
Jonas pushed these, but here are the links http://hg.mozilla.org/mozilla-central/rev/7ff4f4a95e64 http://hg.mozilla.org/mozilla-central/rev/36772409fcb4 http://hg.mozilla.org/mozilla-central/rev/aed2287bf3ba
Assignee | ||
Comment 13•14 years ago
|
||
Well I left it open cause I was going to write some tests as requested.
Comment 14•13 years ago
|
||
I'd suggest resolving this bug and filing a new one for tests.
Whiteboard: [approved-patches-landed]
Assignee | ||
Comment 15•13 years ago
|
||
Resolving as suggested.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Component: DOM: Traversal-Range → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•