Closed Bug 564432 Opened 10 years ago Closed 10 years ago

[FIX]Give nsINode next/previous sibling pointers and a first child pointer

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files)

Like the summary says: we want direct pointers to siblings and a pointer to the first child.  We could put the latter on Element if we want later, but for now just stick on nsINode.
Blocks: 513169
Attachment #444091 - Flags: superreview?(jonas)
Attachment #444091 - Flags: review?(jst)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #444092 - Flags: superreview?(jonas)
Attachment #444092 - Flags: review?(jst)
Note on part 2: perhaps I should have broken out the refactoring of do*ChildAt into a separate diff.  Let me know if you would prefer that?  I needed to make it a member method so it can write to mFirstChild.

Another note:  I made GetNextNode inline because it was much faster on my getElementsByTagName tests.  We could make it out-of-line but NS_FASTCALL for a bit of hit but smaller codesize if desired.  Making it inline does have the extra benefit of it being callable from outside gklayout.
Blocks: domperf
Blocks: 564435
One other note.  If we're ok with not firing ContentAppended for appends to the
end of a document fragment, then we can nix that one virtual IsNodeOfType check
too and replace it with an IsElement() check.  I would somewhat prefer that,
actually, since then we can also move on making ContentAppended take Element
for the container.
Blocks: 564574
Blocks: 564575
Summary: Give nsINode next/previous sibling pointers and a first child pointer → [FIX]Give nsINode next/previous sibling pointers and a first child pointer
And one more thing.  I could switch over the nsINode::ChildIterator over to the new API if people prefer we keep that abstraction layer.  Should I?
(In reply to comment #5)
> And one more thing.  I could switch over the nsINode::ChildIterator over to the
> new API if people prefer we keep that abstraction layer.  Should I?

The idea is to migrate all code to not use the child array right? And use the new API instead. Seems like ChildIterator needs to be converted at some point as part of that, no?

In other words: Go for it :)
Comment on attachment 444091 [details] [diff] [review]
Part 1.  Add sibling pointers

>+nsAttrAndChildArray::SetChildAtPos(void** aPos, nsIContent* aChild,
>+                                   PRUint32 aIndex, PRUint32 aChildCount)
>+{
>+  *aPos = aChild;
>+  NS_ADDREF(aChild);
>+  if (aIndex != 0) {
>+    nsIContent* previous = static_cast<nsIContent*>(*(aPos - 1));
>+    aChild->mPreviousSibling = previous;
>+    previous->mNextSibling = aChild;
>+  }
>+  if (aIndex != aChildCount) {
>+    nsIContent* next = static_cast<nsIContent*>(*(aPos + 1));
>+    aChild->mNextSibling = next;
>+    next->mPreviousSibling = aChild;
>+  }
>+}

Wanna assert that aChild has a null next and previous sibling when you enter this function? Or is that asserted elsewhere?

sr=me with that.
Attachment #444091 - Flags: superreview?(jonas) → superreview+
> Seems like ChildIterator needs to be converted at some point as part of that,
> no?

Unless we drop ChildIterator altogether, right?

> Wanna assert that aChild has a null next and previous sibling when you enter
> this function? 

Will do.
(In reply to comment #8)
> > Seems like ChildIterator needs to be converted at some point as part of that,
> > no?
> 
> Unless we drop ChildIterator altogether, right?

Oh, true. Yeah, might not make sense to keep ChildIterator so no need to spend time on it.
Attachment #444091 - Flags: review?(jst) → review+
Comment on attachment 444092 [details] [diff] [review]
Add a first-child pointer on nsINode

Looks good to me, r=jst
Attachment #444092 - Flags: review?(jst) → review+
Attachment #444092 - Flags: superreview?(jonas) → superreview+
Pushed:
  http://hg.mozilla.org/mozilla-central/rev/e746b40974d1
  http://hg.mozilla.org/mozilla-central/rev/1c6e9e4f3e09
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
Blocks: 564979
Depends on: 565125
Depends on: 564974
Target Milestone: --- → mozilla1.9.3a5
Blocks: 651120
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.