240851 - [FIX]Content lists don't ignore anonymous nodes

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: I think this should do it....

Comment 2

[Johnny Stenback (:jst)]

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

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

+      nsIContent* newContent = aContainer->GetChildAt(aNewIndexInContainer);
+      while (newContent && IsContentAnonymous(newContent)) {

why is this needed? Aren't nodes returned by GetChildAt always un-anonymous?

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch with better comments and unneeded changes removed

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 147285 [details] [diff] [review]
Patch with better comments and unneeded changes removed

jst, does that comment look ok?

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 147285 [details] [diff] [review]
Patch with better comments and unneeded changes removed

r=me

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 147285 [details] [diff] [review]
Patch with better comments and unneeded changes removed

Yeah, looks good to me.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Fixed.  I'm not sure it's worth trying for branch with this; in any case it
needs to bake on the trunk first.

Comment 10

[[e-mail]]

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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> 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

[[e-mail]]

(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

[[e-mail]]

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