77585 - nsTextNode should implement AppendData()

Status: VERIFIED FIXED

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

Description

[Chris Waterson]

(cf. bug 77540.) nsTextNode ends up relying on the nsGenericDOMDataNode's 
implementation of AppendData(), which fights with nsTextNode's attempts to keep 
data stored as single-byte when possible.

Comment 1

[Johnny Stenback (:jst)]

Waterson, what do you suggest we do about this?

Comment 2

[Chris Waterson]

The specific problem is that whenever the content sink calls AppendData() on a
text node (instead of SetText(), e.g.), we end up having a worst-case scenario
where the existing single-byte data is needlessly re-inflated to double-byte,
forcing a re-allocation of the buffer.

IIRC, bug 77540 highlighted this: it was a long flat text doc that caused us to
oscillate back and forth between SetText() and AppendData() due to the buffer
size in the content sink. This ended up causing us to inflate and deflate the
text repeatedly.

I guess I'm proposing that nsTextNode should implement AppendData() itself,
instead of forwarding to mInner. (nsTextNode could probably make use of some of
the new string fu, too.)

Alternatively, we could

- Give up on storing data as ASCII, and just use nsGenericDOMDataNode's
  storage management. This would sort of suck, because we'd have to deflate
  to deflate to measure text in nsTextFrame.  

- Move the ASCII handling out of nsTextNode into nsGenericDOMDataNode.

I think I might like the latter alternative best of all...

Comment 3

[Chris Waterson]

jst, any interest in doing this (soon)? If not, please reassign back to me and
I'll do it.

Comment 4

[Johnny Stenback (:jst)]

Chris, feel free to work on this, I got my plate more than full at the moment...

Comment 5

[Jeff Qiu]

Attachment: patch for nsTextNode to implement its own AppendData()

As Chris's suggestion, the existing single-byte data is no longer needlessly
re-inflated to double-byte.

Comment 6

[Johnny Stenback (:jst)]

Jeff, in stead of adding an AppendData() method to nsTextNode that is more
efficient than the one in nsGenericDOMDataNode, why not simply speed up the one
in nsGenericDOMDataNode? I.e. take the code you wrote in nsTextFragment and move
it into nsGenericDOMDataNode::AppendData(const nsAString&).

Let me know if you don't have time to work on that and I'll take over (unless
someone else volunteers).

Comment 7

[Jeff Qiu]

Attachment: As jst's suggestion, do it in nsGenericDOMDataNode::AppendData

Thank you, jst.
Anyone who has the right, please obsolete patch 73179.

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 78515 [details] [diff] [review]
As jst's suggestion, do it in nsGenericDOMDataNode::AppendData

sr=jst

Comment 9

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

Comment on attachment 78515 [details] [diff] [review]
As jst's suggestion, do it in nsGenericDOMDataNode::AppendData

+  void AppendTo(nsCString& aCString) const;

Can you use nsACString? (ideally you'd also change the nsString of the other
AppendTo function to nsAString)

r=biesi with at least the nsCString->nsACString change

Comment 10

[Johnny Stenback (:jst)]

Unfortunately we can't use nsACString there since that would have performance
implications in the implementation of that method. We'd end up having to use
NS_Convert* in the implementation which would cost us extra string copying and
potentially memory allocation/freeing.

Comment 11

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

ah yeah...

ignore my comment, r=biesi on the current patch

Comment 12

[Johnny Stenback (:jst)]

Taking bug.

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 78515 [details] [diff] [review]
As jst's suggestion, do it in nsGenericDOMDataNode::AppendData

a=[e-mail] for drivers.  Please check into both branch and trunk.

Comment 14

[Johnny Stenback (:jst)]

Fixed on the trunk.

Comment 15

[Heikki Toivonen (remove -bugzilla when emailing directly)]

The checkin had an interesting (beneficial) side effect as discovered by
[e-mail]: bug  121841 is almost gone. It is one of the most duped
hang bugs we have. Note that this patch does not fix it, this merely makes it
less likely to happen and hides it in most cases, it seems.

Comment 16

[Stewart Campbell]

Comment 13 shows drivers approval on Trunk & Branch, so why has this made it
into Trunk only?

Comment 17

[Johnny Stenback (:jst)]

Us Netscape people need even more approval before checking in on the branch,
approval request sent...

Comment 18

[Chris Waterson]

It's probably worth explicitly commenting on the naked nsCString usage: I would
have made the same mistake that biesi did. (And we don't want someone to `clean
this up' later.)

Comment 19

[Johnny Stenback (:jst)]

This patch has lived on the trunk and gotten some testing there already, this
change affects any page with text chunks that are more than 4096 characters
long, i.e. most plain text files out there. If there'd be problems with this
patch I would've expected to see them really early, and none have shown up so far.

Comment 20

[Johnny Stenback (:jst)]

Comment added on the trunk.

Comment 21

[Paul Wyskoczka]

adt1.0.0+ on behalf of the adt.  Please check into the branch today and add
fixed1.0.0 in the keyword field.

Comment 22

[Johnny Stenback (:jst)]

Fixed on the 1.0 branch too.

Comment 23

[Prashant Desale]

Johnney is there any testcase I can use to test this ?
or could you please verify this one ?
thnx.

Comment 24

[Johnny Stenback (:jst)]

Verified.