235255 - Using AppendData to append text in the sink notifies

Status: RESOLVED WORKSFORME

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

Description

[Boris Zbarsky [:bzbarsky]]

See bug 235022 comment 7.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: A start

Thsi makes the necessary changes to the content sink.  I got this far, then
took a look at nsGenericDOMDataNode and wanted to run away screaming.  Can't we
kill off some of that code duplication?  And what's the deal with AppendData
being all different?

sicking, any chance you could pick this up from here and handle the domdatanode
bit?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Of course in light of bug 17191 I am starting to question this whole "save the
last text node" thing.....

Comment 3

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

how else would we do it? If we don't save the last textnode and append data to
it we won't be able to 'stream' in textpages or pages with long textcontent.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Do it like the XML sink does and create a new textnode for each AppendText call,
perhaps?  Depends on how much the parser will pass through at a time (this is
well-worth looking into -- we should make the max size the parser passes
somewhat comparable to the max size we put into a single textnode).

Comment 5

[Johnny Stenback (:jst)]

This whole splitting text up into multiple text nodes is in violation with the
DOM spec, and indeally we wouldn't need to do that. And I don't know of any
reasons to do that, other than perhaps performance reasons in dealing with the
string data, or does layout have issues with multi-megabyte textnodes? :-)

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

well, layout does have issues with large blocks of text even if they're in
separate textnodes, that's for sure ;)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Well.. any time you change a textnode, at the moment, layout is likely to
remeasure all the text in that textnode.  So yes, multi-megabyte textnodes would
probably not sit well with it.  ;)

So back to the topic at hand -- having a way to easily append text to a text
content object that preferably satisfies the following criteria:

1)  Takes a PRUnicha* + length or an nsDependentSubstring
2)  Uses realloc instead of the current mess with temp strings on the stack
3)  Allows not notifying.

Comment 8

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

I'm with jst here, we shouldn't 'break' the DOM in order to make up for
suboptimal code elsewhere.

The first two points in the previous comment sounds good. However i do think
that we still need to send off some sort of notification. Otherwise we won't be
able to stream data onto the screen as a textdocument loads.

How about adding an argument like 'aAppendStartIndex' to
nsIDocumentObserver::CharacterDataChanged. That way layout would only have to
remeasure that part.
An alternative is breaking CharacterDataChanged into two separate functions like
CharacterDataChanged and CharacterDataAppended.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> However i do think that we still need to send off some sort of notification

Did you read the patch I attached?

ccing people who know way more about the textframe stuff than I do...

Comment 10

[Boris Zbarsky [:bzbarsky]]

This also came up as an issue in bug 145425 -- we spend ridiculous amounts of
time there looking for the primary frame for textnodes because of this bug.

Comment 11

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

Is my patch in bug 330872 enough to mark this fixed? Or should we try to notify even less.

The scenario where I think we could try to notify less is this:

1. We get a chunk of text data
2. We create a textnode and insert it.
3. We get two more chunks of text data

With my patch in bug 330872 we in 3 only notify for the new text if we notified in 2 (for whatever reason). However we do notify twice still in 3. It would be possible to only notify once if we're clever enough about it.

Comment 12

[Boris Zbarsky [:bzbarsky]]

We should probably reprofile the bugs this bugs once bug 330872 lands.

Comment 13

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

What is really left to do here? Just the perf stuff I mentioned in comment 11?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Well, to fix bug 394540 we'd want to not notify synchronously from the text append at all, and instead notify later, when it's safe to do so...  So yeah, right now we have one extra notify and fire two extra mutation events (if someone is listening); we really don't want to be doing the latter, imo.

Comment 15

[Henri Sivonen (:hsivonen)]

Resolving WFM on the theory that the code this bug was reported about is no longer relevant.