243034 - Make nsWyciwygChannel suck less

Status: RESOLVED FIXED

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

Description

[Johnny Stenback (:jst)]

Currently we always convert document.write() data to UTF-8 when we write it to a
wycywyg channel. In stead of doing that, we could simply write it out as UTF-16
and avoid the conversion+copy. Patch coming up.

Comment 1

[Johnny Stenback (:jst)]

Attachment: Like so?

Comment 2

[Johnny Stenback (:jst)]

Comment on attachment 147994 [details] [diff] [review]
Like so?

bz, whaddayathink?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 147994 [details] [diff] [review]
Like so?

r+sr=bzbarsky assuming you tested this (create a page via document.write, then
go to some other page, then go back to the wyciwyg one, that sort of thing) and
it works.

Comment 4

[Johnny Stenback (:jst)]

Yup, that works. And I inspected things in the debugger as well to make sure the
parser got the character conversion right with this change.

Comment 5

[Johnny Stenback (:jst)]

FIXED.

Comment 6

[Alfred Kayser]

Question about this:
   static NS_NAMED_LITERAL_STRING(new_line, "\n");
   static NS_NAMED_LITERAL_STRING(empty, "");
   const nsAString  *term = aNewlineTerminate ? &new_line : ∅
 
   const nsAutoString text = aText + *term;

Could it be rewritten as, to prevent string copying, and adding an 'empty' string:

   static NS_NAMED_LITERAL_STRING(new_line, "\n");
   const nsAString  text = aNewlineTerminate ? aText + new_line : aText;

Or are the string classes allready smart enough to prevent string copying in
case of adding an empty string?

Comment 7

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

(In reply to comment #6)
> Question about this:
>    static NS_NAMED_LITERAL_STRING(new_line, "\n");
>    static NS_NAMED_LITERAL_STRING(empty, "");

Hmm, static literal strings? aren't those bad?
And what about EmptyString()?

hm, that code wasn't touched by the patch....

>    const nsAString  text = aNewlineTerminate ? aText + new_line : aText;

nsAString is an abstract class, and can't be instantiated...

Comment 8

[Boris Zbarsky [:bzbarsky]]

(In reply to comment #6)
>    const nsAString  *term = aNewlineTerminate ? &new_line : ∅

Assuming you meant nsAString&, that doesn't compile on at least some of the
platforms we want to support (HP-UX) and crashes on others (some gcc versions on
Linux).

Basically, you can't use ?: with strings in a sane way.

Comment 9

[Johnny Stenback (:jst)]

Right, and because of that, and the fact that our string code isn't smart enough
to not copy when doing string + EmptyString(), it might be worth not doing the
concatenation in the cases where we don't need to. I'll attach a patch that does
that...

Comment 10

[Johnny Stenback (:jst)]

Attachment: Don't concatenate strings with empty strings.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 148219 [details] [diff] [review]
Don't concatenate strings with empty strings.

r+sr=bzbarsky

Comment 12

[Johnny Stenback (:jst)]

Fix checked in.

Comment 13

[Johnny Stenback (:jst)]

This caused bug 244178.