224231 - Need AppendASCIItoUTF16

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P4)

Description

[Peter Van der Beken [:peterv]]

There's several places in the tree that could use this. Patch upcoming.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 2

[Peter Van der Beken [:peterv]]

Do we want to add LossyCopyUTF16toASCII( const PRUnichar* aSource, nsACString&
aDest ) and CopyASCIItoUTF16( const char* aSource, nsAString& aDest ) for
consistency?

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 134535 [details] [diff] [review]
v1


>+NS_COM
>+void
>+AppendASCIItoUTF16( const nsACString& aSource, nsAString& aDest )
>+  {
>+      // right now, this won't work on multi-fragment destinations
>+    aDest.SetLength(aDest.Length() + aSource.Length());
>+
>+    nsACString::const_iterator fromBegin, fromEnd;
>+
>+    nsAString::iterator toEnd;
>+    LossyConvertEncoding<char, PRUnichar> converter(aDest.EndWriting(toEnd).get());

I'm surprised this works.  Since you've already done |SetLength|, shouldn't
|EndWriting| give you the new end instead of the old end?  The existing Append
functions save the old length, and do BeginWriting followed by advance.

>+
>+    copy_string(aSource.BeginReading(fromBegin), aSource.EndReading(fromEnd), converter);
>+  }

Comment 4

[Peter Van der Beken [:peterv]]

Yeah, my build finished and I just figured out the same thing.

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 134539 [details] [diff] [review]
v1.1

sr=dbaron, although maybe the "right now, this won't work..." comment belongs
before the definition of the |converter| variable since that *really* doesn't
work.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 134539 [details] [diff] [review]
v1.1

Nice! r=jst

Comment 8

[Peter Van der Beken [:peterv]]

Checked in except the WebBrowserChrome.cpp changes, I think they want
UTF8toUTF16 so I'll move that to a patch for bug 209699.

Comment 9

[Brian Ryner (not reading)]

I think this may have increased pageload time a bit on btek.  It went from
between 965 and 970 before the checkin to between 975 and 983 (or so) after the
checkin.  I don't see anything else during the checkin window.

Comment 10

[Peter Van der Beken [:peterv]]

It seems to be limited to btek though.

Comment 11

[Peter Van der Beken [:peterv]]

I'll try to figure out what's going on. Either AppendASCIItoUTF16 is slower or
I've slowed down CopyASCIItoUTF16.

Comment 12

[Peter Van der Beken [:peterv]]

Backing out the changes to CopyASCIItoUTF16 didn't help. Backing out the changes
to mozilla/content/html/content/src/nsAttributeContent.cpp,
mozilla/content/base/src/nsGenericDOMDataNode.cpp and
mozilla/netwerk/streamconv/converters/nsIndexedToHTML.cpp didn't help either.
Most of the other changes are either not on linux, DEBUG-only or in
serialization so I don't think they'll affect Tp. Not sure what's going on there.

Comment 13

[Johnny Stenback (:jst)]

Maybe we need to inline some of this code to eliminate the extra function call
that we're now doing? I.e. move the code in AppendASCIItoUTF16 into a new local
static inlined function that's called by copy and append? Or something along
those lines, at least... Not sure if that'll help enough tho...