Change nsITextTransform::Change to take const PRUnichar* as an input

VERIFIED FIXED in mozilla0.9.9

Status

()

Core
Internationalization
VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: nhottanscp, Assigned: nhottanscp)

Tracking

({intl})

Trunk
mozilla0.9.9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

17 years ago
nsITextTransform::Change takes an input as nsString& which may require the
client an extra allocation.
I think the input does not need to change, so we can use const PRUnichar* as an
input.

http://lxr.mozilla.org/seamonkey/source/intl/unicharutil/public/nsITextTransform.h

Comment 1

17 years ago
add dependency.  set TM to 0.9.9.
Blocks: 73403
Target Milestone: --- → mozilla0.9.9
(Assignee)

Comment 2

17 years ago
Created attachment 69112 [details] [diff] [review]
Added an method to take PRUnichar* as an input string.

Comment 3

17 years ago
Comment on attachment 69112 [details] [diff] [review]
Added an method to take PRUnichar* as an input string.

I think we should pass in the
length. so in case the caller
already know the length, we don't
need to call strlen again here.
(Assignee)

Comment 4

17 years ago
Created attachment 69181 [details] [diff] [review]
Changed to pass a string length.
Attachment #69112 - Attachment is obsolete: true

Comment 5

17 years ago
Comment on attachment 69181 [details] [diff] [review]
Changed to pass a string length.

r=ftang
Attachment #69181 - Flags: review+
Comment on attachment 69181 [details] [diff] [review]
Changed to pass a string length.

sr=blizzard
Attachment #69181 - Flags: superreview+
(Assignee)

Comment 7

17 years ago
The patch was checked in, remove the dependency.
Keep the bug open for the client side change.
No longer blocks: 73403
Status: NEW → ASSIGNED

Updated

17 years ago
Keywords: intl
(Assignee)

Comment 8

17 years ago
Created attachment 69684 [details] [diff] [review]
Client side change, in mail code, changed to use the non nsString function, plus clean up.
Comment on attachment 69684 [details] [diff] [review]
Client side change, in mail code, changed to use the non nsString function, plus clean up.

is nsIAtom::GetUnicode create a new unicode string? if yes, you are leaking
charsetName! in that case I would propose you to use an nsXPIDLCString to solve
the problem. Else, R=ducarroz
Attachment #69684 - Flags: review+
(Assignee)

Comment 10

17 years ago
 51 void GetUnicode([shared, retval] out wstring aResult);
http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsIAtom.idl#51

The implementation does not allocate memory, it returns a pointer of the
internal buffer.
http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsAtomTable.cpp#241

So, charsetName is not leaked.


1)

+ !nsCRT::strcmp(charsetName, NS_LITERAL_STRING("ISO-8859-1").get()) ?

why not do something like:

!strcmp(NS_LossyConvertUCS2toASCII(charsetName).get(), "ISO-8859-1") ?

2)

+  if (!nsCRT::strcmp(charsetName, NS_LITERAL_STRING("ISO-2022-JP").get())) {

why not do something like:

if (!strcmp(NS_LossyConvertUCS2toASCII(charsetName).get(), "ISO-2022-JP")) {

standard libc strcmp of two c strings is faster than nsCRT::strcmp() of two UCS2
strings, right?

3)

+        res = textTransform->Change(inString, nsCRT::strlen(inString), mapped);

can't you do:

inString.Length()?

address those 3 items, and then sr=sspitzer
(Assignee)

Comment 12

17 years ago
1) & 2)
I thought the conversion by NS_LossyConvertUCS2toASCII takes more time than
strlen for PRUnichar*, note charset names are usually short like "ISO-8859-1".

3) inString in PRUnichar*, so cannot do that.
> I thought the conversion by NS_LossyConvertUCS2toASCII takes more time than
> strlen for PRUnichar*, note charset names are usually short like "ISO-8859-1".

I'm not sure it matters how slow strlen is, maybe you mean strcmp.

NS_LossyConvertUCS2toASCII() does look to be kind of heavy weight.  I guess the
question is which is worse:  

NS_LossyConvertUCS2toASCII() and native strcmp() 
or a hand rolled nsCRT::strcmp().

looking at NS_LossyConvertUCS2toASCII() and nsCRT::strcmp(), you are right. 
your way is faster.

(general question, why are charsets not char strings?)

> inString in PRUnichar*, so cannot do that.

oops, I was looking at other inStrings in nsMsgI18N.cpp

sr=sspitzer
(Assignee)

Comment 14

17 years ago
checked in

>(general question, why are charsets not char strings?)
uconv uses nsIAtom for charset and its nsIAtom is PRUnichar* base.
Because charset name is always ASCII, we can use char* until we come to the
point where we need unicode conversion (or need a canonicalized name). After
that, we can use the charset name in const PRUnichar* from nsIAtom which needs
no memory allocation and no need to strcasecmp (because of the canonicalization).
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 15

17 years ago
Should IQA be involved in this bug?
(Assignee)

Comment 16

17 years ago
verified by lxr
Status: RESOLVED → VERIFIED
QA Contact: ruixu → nhotta
You need to log in before you can comment on or make changes to this bug.