[FIX]Content lists don't ignore anonymous nodes

RESOLVED FIXED in mozilla1.8alpha1

Status

()

Core
DOM: Core & HTML
P1
major
RESOLVED FIXED
14 years ago
10 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

({perf})

Trunk
mozilla1.8alpha1
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Created attachment 146400 [details] [diff] [review]
I think this should do it....
Attachment #146400 - Flags: superreview?(jst)
Attachment #146400 - Flags: review?(bugmail)
Priority: -- → P1
Summary: Content lists don't ignore anonymous nodes → [FIX]Content lists don't ignore anonymous nodes
Target Milestone: --- → mozilla1.8alpha
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?
(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.
Attachment #146400 - Flags: review?(bugmail) → review-
Created attachment 147285 [details] [diff] [review]
Patch with better comments and unneeded changes removed
Attachment #146400 - Attachment is obsolete: true
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 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+
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
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 10

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

12 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

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

Updated

10 years ago
Component: DOM: Core → DOM: Core & HTML
QA Contact: ian → general
You need to log in before you can comment on or make changes to this bug.