653948 - Remove ContentInserted/ContentAppended duality

Status: NEW

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

Description

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

Can we kill the current ContentInserted/ContentAppended duality that we have? And instead have a single notification but which supports a range. Something like:

ContentInserted(nsIDocument *aDocument,
                nsIContent* aContainer,
                nsIContent* aStartChild,
                nsIContent* aEndChild,
                PRInt32 aIndexInContainer)

(where aIndexInContainer should eventually go away).

aEndChild is the first node that is *not* newly inserted.

Code that want to optimize for append operations can just check if aEndChild is false.

The advantage here is two-fold. First of all all listeners would only have to handle a single notification. Second, we can likely optimize things like range-insertions in the middle of a node.

The latter can happen with insertAdjecentHTML (which is likely to become more common once it becomes cross-browser) and XBL.

I saw that the layout code has been moving in this direction, though I'm not sure if it takes advantage of range insertions in the middle of a node yet.

Comment 1

[Timothy Nikkel (:tnikkel)]

(In reply to comment #0)
> I saw that the layout code has been moving in this direction, though I'm not
> sure if it takes advantage of range insertions in the middle of a node yet.

Layout will already convert multiple ContentInserted calls that are adjacent into a range insertion and perform it all in one batch (lazily).

Comment 2

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

Mounir said he'd take a stab at this.

Comment 3

[Boris Zbarsky [:bzbarsky]]

In general, I'm fine with this.  That said, there's one case where ContentAppended is faster: it knows that aContainer is non-null.  Unfortunately, for insertion as document kids we'd need to pass a null aContainer to the new method (or make aContainer an nsINode and add some AsContent() in places).

In the frame constructor, I wonder why ContentAppended doesn't just call ContentRangeInserted now...  Timothy, was there a reason for that other than just not wanting to rejigger the code too much?

Whoever works on this, I'd really appreciate a patch queue, esp if you want me to review the frame constructor changes.  ;)

Comment 4

[Timothy Nikkel (:tnikkel)]

There were some non-trivial differences in how appends and inserts were handled.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I guess we should sort those out, then...

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.