Closed
Bug 240851
Opened 20 years ago
Closed 20 years ago
[FIX]Content lists don't ignore anonymous nodes
Categories
(Core :: DOM: Core & HTML, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla1.8alpha1
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
(Keywords: perf)
Attachments
(1 file, 1 obsolete file)
7.64 KB,
patch
|
sicking
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
I was reprofiling the testcase in bug 237735 now that bug 240291 is fixed, and discovered something very interesting. The part of the profile dealing with nsDocument::ContentAppended looks like this: 66253 nsDocument::ContentAppended(nsIContent*, int) 37436 nsContentList::ContentAppended(nsIDocument*, nsIContent*, int) 27853 PresShell::ContentAppended(nsIDocument*, nsIContent*, int) So the short of it is that nsContentList::ContentAppended is taking up a _lot_ if time (about 1/4 of the total!). So I poked about at what's going on. Seems like for every textnode or br node appended to the textarea we actually go and call CompareDocumentPosition on the last thing in the content list. That's not cool for two reasons: 1) It's slow. 2) If we have a list that matches <br> nodes we'll end up with anonymous content in the list.
Assignee | ||
Comment 1•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #146400 -
Flags: superreview?(jst)
Attachment #146400 -
Flags: review?(bugmail)
Assignee | ||
Updated•20 years ago
|
Priority: -- → P1
Summary: Content lists don't ignore anonymous nodes → [FIX]Content lists don't ignore anonymous nodes
Target Milestone: --- → mozilla1.8alpha
Comment 2•20 years ago
|
||
Comment on attachment 146400 [details] [diff] [review] I think this should do it.... +PRBool +nsContentList::IsContentAnonymous(nsIContent* aContent) { Opening brace on its own line! :-) + // aContent is anonymous if it's got a different bindingParent than + // our root. If our root has no bindingParent, then aContent better + // have none either. If we have no root, that means we correspond + // to a list gotten off the "document" object, so will always + // contain only nodes with null bindingParent. You might want to explain a bit more what's going on here (as discussed on IRC). + if (mRootContent) { + return mRootContent->GetBindingParent() != aContent->GetBindingParent(); + } else { + return aContent->GetBindingParent() != nsnull; + } else-after-return sr=jst
Attachment #146400 -
Flags: superreview?(jst) → superreview+
+ nsIContent* newContent = aContainer->GetChildAt(aNewIndexInContainer); + while (newContent && IsContentAnonymous(newContent)) { why is this needed? Aren't nodes returned by GetChildAt always un-anonymous?
Assignee | ||
Comment 4•20 years ago
|
||
(In reply to comment #3) > + nsIContent* newContent = aContainer->GetChildAt(aNewIndexInContainer); > + while (newContent && IsContentAnonymous(newContent)) { > > why is this needed? Aren't nodes returned by GetChildAt always un-anonymous? Hmm... They are from the point of view of aContainer, yes. Which means they're actual kids of aContainer. So given that we check whether aContainer is anonymous, you're right. This isn't needed. Neither are most of the other changes to ContentAppended.
Assignee | ||
Updated•20 years ago
|
Attachment #146400 -
Flags: review?(bugmail) → review-
Assignee | ||
Comment 5•20 years ago
|
||
Attachment #146400 -
Attachment is obsolete: true
Assignee | ||
Comment 6•20 years ago
|
||
Comment on attachment 147285 [details] [diff] [review] Patch with better comments and unneeded changes removed jst, does that comment look ok?
Attachment #147285 -
Flags: superreview?(jst)
Attachment #147285 -
Flags: review?(bugmail)
Comment on attachment 147285 [details] [diff] [review] Patch with better comments and unneeded changes removed r=me
Attachment #147285 -
Flags: review?(bugmail) → review+
Comment 8•20 years ago
|
||
Comment on attachment 147285 [details] [diff] [review] Patch with better comments and unneeded changes removed Yeah, looks good to me.
Attachment #147285 -
Flags: superreview?(jst) → superreview+
Assignee | ||
Comment 9•20 years ago
|
||
Fixed. I'm not sure it's worth trying for branch with this; in any case it needs to bake on the trunk first.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Comment 10•18 years ago
|
||
Could this possibly have broken tabbrowser's context menu? Its call to aPopup.getElementsByAttribute("tbattr", "tabbrowser-multiple") is returning a zero-length list. Note that document.getAnonymousElementByAttribute(this, "tbattr", "tabbrowser-multiple") does return the first such element, but then it manually walks the anonymous content subtree.
Assignee | ||
Comment 11•18 years ago
|
||
> Could this possibly have broken tabbrowser's context menu?
"Not likely". This bug only changed behavior on dynamic DOM changes.
Please file a bug on the problem you're seeing, with steps to reproduce? I'll be happy to look into it next week.
Comment 12•18 years ago
|
||
(In reply to comment #11) >Please file a bug on the problem you're seeing, with steps to reproduce? I'll >be happy to look into it next week. Actually it's now looking like an XBL regression, I'll file a bug on it.
Comment 13•18 years ago
|
||
(In reply to comment #12) >Actually it's now looking like an XBL regression, I'll file a bug on it. To be precise, it's a regression in our XBL front end caused by bug 288986.
You need to log in
before you can comment on or make changes to this bug.
Description
•