Closed
Bug 224231
Opened 21 years ago
Closed 21 years ago
Need AppendASCIItoUTF16
Categories
(Core :: XPCOM, defect, P4)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla1.6beta
People
(Reporter: peterv, Assigned: peterv)
Details
Attachments
(1 file, 1 obsolete file)
30.77 KB,
patch
|
jst
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
There's several places in the tree that could use this. Patch upcoming.
Assignee | ||
Comment 1•21 years ago
|
||
Assignee | ||
Updated•21 years ago
|
Attachment #134535 -
Flags: superreview?(dbaron)
Attachment #134535 -
Flags: review?(jst)
Assignee | ||
Comment 2•21 years ago
|
||
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); >+ }
Assignee | ||
Comment 4•21 years ago
|
||
Yeah, my build finished and I just figured out the same thing.
Assignee | ||
Comment 5•21 years ago
|
||
Attachment #134535 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #134535 -
Flags: superreview?(dbaron)
Attachment #134535 -
Flags: review?(jst)
Assignee | ||
Updated•21 years ago
|
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 7•21 years ago
|
||
Comment on attachment 134539 [details] [diff] [review] v1.1 Nice! r=jst
Attachment #134539 -
Flags: review?(jst) → review+
Assignee | ||
Comment 8•21 years ago
|
||
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
Comment 9•21 years ago
|
||
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.
Assignee | ||
Comment 10•21 years ago
|
||
It seems to be limited to btek though.
Assignee | ||
Comment 11•21 years ago
|
||
I'll try to figure out what's going on. Either AppendASCIItoUTF16 is slower or I've slowed down CopyASCIItoUTF16.
Assignee | ||
Comment 12•21 years ago
|
||
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•21 years ago
|
||
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...
Updated•3 years ago
|
Component: String → XPCOM
You need to log in
before you can comment on or make changes to this bug.
Description
•