Closed Bug 485808 Opened 15 years ago Closed 15 years ago

[FIX]Create iterator class to iterate using GetChildArray

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(2 files)

A lot of the GetChildArray consumers really do just want a straight-through iteration of the child nodes.  I think it would make sense to have an iterator class they can use (which would also assert that no mutations happen during the iteration).
Either you forgot an attachment, or the title needs adjustment?
Attachment will come once I finish compiling it and such; probably tomorrow.  The title is just for tracking purposes for me...
This would also of course make it easier to switch to non-array child list storage... I'm Boris was secretly thinking of that :-).
It was in the back of my head, yes.  ;)
Attached patch PatchSplinter Review
Some review notes:

1)  There is one remaining consumer of GetChildArray: it's in nsCSSRuleProcessor, and iterates either forwards or backwards depending.  I considered creating a backwards iterator too, rewriting that code to use it, then making GetChildArray protected, but figured I'd start with the easy thing.

2)  I could stick nsMutationGuard into a separate header.

3)  I could add a SafeChildIterator that iterates via GetChildAt() and hence deals ok with ending up outside count due to mutations of the array during iteration.  We would need something like that in a number of cases where we iterate over child lists... Of course at that point it would make more sense to have an iterator that can keep its position correct during mutations.  Or something.

Thoughts?
Attachment #369942 - Flags: superreview?(jonas)
Attachment #369942 - Flags: review?(jonas)
Attachment #369942 - Flags: superreview?(jonas)
Attachment #369942 - Flags: superreview+
Attachment #369942 - Flags: review?(jonas)
Attachment #369942 - Flags: review+
Comment on attachment 369942 [details] [diff] [review]
Patch

Could you make Next() return a PRBool indicating if iteration is done? 

That would allow for things like:

nsINode::ChildIterator iter(aContainer, aNewIndexInContainer);
while (iter.Next()) {
  DoStuff(iter);
}

r/sr=me with that.
That would involve one extra compare in the common case, and I'm not sure when you'd have a pattern as described above...  That pattern implies not wanting to do anything with the node at index aNewIndexInContainer, no?

Unless you meant:

  do {
    DoStuff(iter);
  } while (iter.Next());

?  That would make sense for the append cases but not the ones that are iterating an existing tree...
Ok, you have me convinced. Looked at STL iterators and they work the way your class does.
Pushed http://hg.mozilla.org/mozilla-central/rev/ce4542e6cc72
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Blocks: 90906
Target Milestone: --- → mozilla1.9.2a1
Attachment #373488 - Flags: superreview?(jonas)
Attachment #373488 - Flags: review?(jonas)
Attachment #373488 - Flags: superreview?(jonas)
Attachment #373488 - Flags: superreview+
Attachment #373488 - Flags: review?(jonas)
Attachment #373488 - Flags: review+
Comment on attachment 373488 [details] [diff] [review]
(Bv1) Remove leftover 'i'
[Checkin: Comment 11]


http://hg.mozilla.org/mozilla-central/rev/bb8ba67896a1
Attachment #373488 - Attachment description: (Bv1) Remove leftover 'i' → (Bv1) Remove leftover 'i' [Checkin: Comment 11]
Component: Content → DOM
QA Contact: content → general
Depends on: 542040
No longer depends on: 542040
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: