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.

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.

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.

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.

> 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.

Comment on attachment 444092 [details] [diff] [review] Add a first-child pointer on nsINode Looks good to me, r=jst

Pushed: http://hg.mozilla.org/mozilla-central/rev/e746b40974d1 http://hg.mozilla.org/mozilla-central/rev/1c6e9e4f3e09