Closed Bug 224231 Opened 21 years ago Closed 21 years ago

Need AppendASCIItoUTF16

Categories

(Core :: XPCOM, defect, P4)

defect

Tracking

()

RESOLVED FIXED
mozilla1.6beta

People

(Reporter: peterv, Assigned: peterv)

Details

Attachments

(1 file, 1 obsolete file)

There's several places in the tree that could use this. Patch upcoming.
Attached patch v1 (obsolete) — Splinter Review
Attachment #134535 - Flags: superreview?(dbaron)
Attachment #134535 - Flags: review?(jst)
Do we want to add LossyCopyUTF16toASCII( const PRUnichar* aSource, nsACString&
aDest ) and CopyASCIItoUTF16( const char* aSource, nsAString& aDest ) for
consistency?
Status: NEW → ASSIGNED
Priority: -- → P4
Target Milestone: --- → mozilla1.6beta
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);
>+  }
Yeah, my build finished and I just figured out the same thing.
Attached patch v1.1Splinter Review
Attachment #134535 - Attachment is obsolete: true
Attachment #134535 - Flags: superreview?(dbaron)
Attachment #134535 - Flags: review?(jst)
Attachment #134539 - Flags: superreview?(dbaron)
Attachment #134539 - Flags: review?(jst)
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.
Attachment #134539 - Flags: superreview?(dbaron) → superreview+
Comment on attachment 134539 [details] [diff] [review]
v1.1

Nice! r=jst
Attachment #134539 - Flags: review?(jst) → review+
Checked in except the WebBrowserChrome.cpp changes, I think they want
UTF8toUTF16 so I'll move that to a patch for bug 209699.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
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.
It seems to be limited to btek though.
I'll try to figure out what's going on. Either AppendASCIItoUTF16 is slower or
I've slowed down CopyASCIItoUTF16.
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.
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...
Component: String → XPCOM
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: