372086 - Should ranges observe the common ancestor container instead?

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Right now ranges in a document observe the document.  This means that if you have a lot of ranges, all sorts of DOM manipulations become very slow.

For example, the large testcase in bug 352367 ends up with one range per textfield (the editor's selection).  As a result, a partial profile (I killed the app when the log file was nearing a gig) shows that 10% of the time is spent assigning document observers (most of them ranges) into nsCOMPtrs while notifying.  Given N textfields, this behavior makes the load O(N^2).  The constant is small, but that testcase seems to be big enough that this does not matter anymore...

Now we should work on lazy editor init for sure, but at the same time, it seems like it would make more sense for ranges to observe their common ancestor container, not the document...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Unfortunately, we pass mRoot to nsContentUtils::ContentIsDescendantOf in nsRange::ComparePoint.  We also expose it via GetRoot().  Which means we have to notice when it goes away....  :(

I just realized that lots of editor is not the only thing that produces lots of ranges in common browsing.  Spellcheck on a large textarea will do the same thing.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Nevermind.  That wouldn't work well at all -- we want ranges to notice when an ancestor of an endpoint is being removed.

So let me try that in a different way.  ;)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Maybe like so

After some consideration, perhaps we should do this.  I tested with <https://bugzilla.mozilla.org/attachment.cgi?id=143479> and selection with anon content is already broken enough that this won't really break it more.  And it makes a lot more sense than what we do now, imo.

Comment 4

[Boris Zbarsky [:bzbarsky]]

With that change, bug 236957 will become more or less moot, I think...

Comment 5

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

Comment on attachment 256771 [details] [diff] [review]
Maybe like so

Lets try it and see if anything breaks

Comment 6

[Boris Zbarsky [:bzbarsky]]

Checked in.  Still need to write tests...

Comment 7

[[e-mail]]

(In reply to comment #5)
>(From update of attachment 256771 [details] [diff] [review])
>Lets try it and see if anything breaks
void
nsRange::DoSetRange(nsINode* aStartN, PRInt32 aStartOffset,
                    nsINode* aEndN, PRInt32 aEndOffset,
                    nsINode* aRoot)
{
  NS_PRECONDITION((aStartN && aEndN && aRoot) ||
                  (!aStartN && !aEndN && !aRoot),
                  "Set all or none");
  NS_PRECONDITION(!aRoot ||
                  (!aRoot->GetNodeParent() &&
                   (aRoot->IsNodeOfType(nsINode::eDOCUMENT) ||
                    aRoot->IsNodeOfType(nsINode::eATTRIBUTE) ||
                    aRoot->IsNodeOfType(nsINode::eDOCUMENT_FRAGMENT))),
                  "Bad root");

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Oops. Good point.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Checked in the assertion check.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Mochitest

Comment 11

[Boris Zbarsky [:bzbarsky]]

Checked that in.