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

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[nhottanscp]

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

[Takayuki Tei]

add dependency.  set TM to 0.9.9.

Comment 2

[nhottanscp]

Attachment: Added an method to take PRUnichar* as an input string.

Comment 3

[Frank Tang]

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.

Comment 4

[nhottanscp]

Attachment: Changed to pass a string length.

Comment 5

[Frank Tang]

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

r=ftang

Comment 6

[Christopher Blizzard (:blizzard)]

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

sr=blizzard

Comment 7

[nhottanscp]

The patch was checked in, remove the dependency.
Keep the bug open for the client side change.

Comment 8

[nhottanscp]

Attachment: Client side change, in mail code, changed to use the non nsString function, plus clean up.

Comment 9

[Jean-Francois Ducarroz]

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

Comment 10

[nhottanscp]

 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.

Comment 11

[(not reading, please use [e-mail] instead)]

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

Comment 12

[nhottanscp]

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.

Comment 13

[(not reading, please use [e-mail] instead)]

> 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

Comment 14

[nhottanscp]

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).

Comment 15

[Rui Xu]

Should IQA be involved in this bug?

Comment 16

[nhottanscp]

verified by lxr