Closed
Bug 564432
Opened 15 years ago
Closed 15 years ago
[FIX]Give nsINode next/previous sibling pointers and a first child pointer
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a5
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
6.73 KB,
patch
|
jst
:
review+
sicking
:
superreview+
|
Details | Diff | Splinter Review |
22.46 KB,
patch
|
jst
:
review+
sicking
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•15 years ago
|
||
Attachment #444091 -
Flags: superreview?(jonas)
Attachment #444091 -
Flags: review?(jst)
Assignee | ||
Comment 2•15 years ago
|
||
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #444092 -
Flags: superreview?(jonas)
Attachment #444092 -
Flags: review?(jst)
Assignee | ||
Comment 3•15 years ago
|
||
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.
Assignee | ||
Comment 4•15 years ago
|
||
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.
Assignee | ||
Updated•15 years ago
|
Summary: Give nsINode next/previous sibling pointers and a first child pointer → [FIX]Give nsINode next/previous sibling pointers and a first child pointer
Assignee | ||
Comment 5•15 years ago
|
||
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+
Assignee | ||
Comment 8•15 years ago
|
||
> 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.
Updated•15 years ago
|
Attachment #444091 -
Flags: review?(jst) → review+
Comment 10•15 years ago
|
||
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+
Assignee | ||
Comment 11•15 years ago
|
||
Pushed:
http://hg.mozilla.org/mozilla-central/rev/e746b40974d1
http://hg.mozilla.org/mozilla-central/rev/1c6e9e4f3e09
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•15 years ago
|
Flags: in-testsuite-
Assignee | ||
Updated•15 years ago
|
Target Milestone: --- → mozilla1.9.3a5
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•