need unicode converter interfaces for script

RESOLVED FIXED in mozilla0.9.4



18 years ago
17 years ago


(Reporter: m_kato, Assigned: tetsuroy)



Firefox Tracking Flags

(Not tracked)


(Whiteboard: /r=nhotta,/sr=blizzard)


(18 attachments)

1.75 KB, text/plain
5.20 KB, text/plain
6.95 KB, patch
Details | Diff | Splinter Review
5.50 KB, patch
Details | Diff | Splinter Review
1.86 KB, patch
Details | Diff | Splinter Review
1.14 KB, patch
Details | Diff | Splinter Review
1.18 KB, patch
Details | Diff | Splinter Review
5.33 KB, patch
Details | Diff | Splinter Review
1.87 KB, patch
Details | Diff | Splinter Review
5.42 KB, patch
Details | Diff | Splinter Review
1.73 KB, patch
Details | Diff | Splinter Review
4.81 KB, patch
Details | Diff | Splinter Review
1.39 KB, patch
Details | Diff | Splinter Review
5.16 KB, patch
Details | Diff | Splinter Review
4.58 KB, patch
Details | Diff | Splinter Review
3.51 KB, patch
Details | Diff | Splinter Review
1.51 KB, patch
Details | Diff | Splinter Review
13.05 KB, patch
Details | Diff | Splinter Review


18 years ago
I need unicode converter interfaces for script due to bug 41564, since Chatzilla
is 100% pure JavaScript and XUL application.


18 years ago
Blocks: 41564


18 years ago
Ever confirmed: true

Comment 1

18 years ago
Reassign to ftang, cc to cata.
Assignee: nhotta → ftang

Comment 2

18 years ago
hum... can you propose a idl for this ?
Assignee: ftang → cata
Target Milestone: --- → Future

Comment 3

18 years ago
Created attachment 16160 [details]

Comment 4

18 years ago
Created attachment 17245 [details]

Comment 5

18 years ago
Created attachment 17247 [details] [diff] [review]
patches of mozilla/intl


18 years ago

Comment 6

18 years ago
Thanks for your contribution. My comments below.

1) Are you sure this is the right solution for the problem? I think at this 
point you are the only customer for the interface, so you are the only one able 
to answer this. I can see at least 3 ways to fix it: a) make the converters 
interface itself be scriptable; b) have a bridge converter object; c) have 
convenience methods on an existing component (something like 
CharsetConverterManager::ConvertToUnicode(string, charset)). So, can you argue a 
little bit why you choosed solution b) and which are the benefits? Personally 
I don't have a strong opinion, but if anybody else will use this interface, 
another point of view would be great.

2) You should create the internal encoder/decoder objects *lazily*. Do not 
instantiate until needed! Really, those chinese encoders for example can get 
pretty big. Why create them if you will only decode in that session?

3) Please use the new nsICharsetConverterManager2 interface inside your object. 
The old one is being deprecated.

Thanks a lot!

Comment 7

18 years ago
Chatzilla is 100% pure XUL application.  So it cannot have convert object by 
So I want Converter method on nsICharsetConverterManager2 or other interface.
Cata, how do you think?

Comment 8

18 years ago
move all cata's bug to ftang
Assignee: cata → ftang

Comment 9

18 years ago
Chat related issue. reassign to nhotta-
nhotta- please help m_kato to check in the patch. I think yokoyama change the
uconv nsIModule code recently, so probably we need to redo that part.
Assignee: ftang → nhotta

Comment 10

18 years ago
> 2000-10-31 14:15
2) lazy creation, is that the case SetCharset is always followed by the
conversion calls (I mean for the chat case). I am not sure about general usage,
what would be the situation that charset conversion is needed in JS code?
3) please use nsICharsetConverterManager2 as Cata mentioned.
Assignee: nhotta → m_kato
Target Milestone: Future → ---

Comment 11

18 years ago
The socket stream code of ChartZilla is JS only.  So it need JS version of
charset convert code.
And I already rewrote to nsICharsetConvertManager2.  So I hope that you decide
which creating new interface (like my code) or adding new metod in

Comment 12

18 years ago
Please go ahead with a separate interface.
I think using converters in JS is not common (so no need to add to

Comment 13

18 years ago
Changed QA contact to for now.
QA Contact: teruko → andreasb

Comment 14

18 years ago
Changing QA contact back to
QA Contact: andreasb → teruko

Comment 15

17 years ago
Created attachment 38435 [details] [diff] [review]

Comment 16

17 years ago
Created attachment 38437 [details] [diff] [review]

Comment 17

17 years ago
Created attachment 38441 [details] [diff] [review]
diff mozilla/intl/uconv/src/

Comment 18

17 years ago
I modify nsScriptableUnicodeConverter with nsICharsetConvertManager2
IDL is same as original.

nsUConvDll.cpp is not used now, so I remove it from cpp/h.

I already confirm ConvertFromUnicode() on my environment.
(Windows2000[jp]/sp1 and VC++6.0/SP3)

Comment 19

17 years ago
Created attachment 38444 [details] [diff] [review]
mozilla/intl/uconv/src/nsUConvModule.cpp (I forgot one)

Comment 20

17 years ago
Reassigning to Furukawa-san.

Could somebody review the patch?
Assignee: m_kato → oliver
Target Milestone: --- → mozilla1.0

Comment 21

17 years ago
* I think InitConverter does not have to be called in the contructor (e.g.
ConvertToUnicode can check if mCharset is empty). It's not likely that the
"ISO-8859-1" converters are actually used.
* nsCOMPtr can be used for mEncoder and mDecoder.
* Indentation not seems to be rightt in InitConverter().
* Need modification for Macintosh project file (I can help for that after the
files are checked in).

Roy, could you review nsUConvModule.cpp?

Comment 22

17 years ago
I reviewed nsUConvModule.cpp (06/14/01 11:05) and it looks fine to me.
/r=yokoyama for the file.

Comment 23

17 years ago
Created attachment 39302 [details] [diff] [review]

Comment 24

17 years ago
Created attachment 39305 [details] [diff] [review]

Comment 25

17 years ago
I modified and attach 2 files. 
* change type of mEncoder/mDecoder
* remove NS_IF_RELEASE
* remove AssignWithConversion() and InitConverter() from constructor
* re-format InitConverter()

Hotta-san, could you check?

I can't modify Macintosh project file. (I don't have Mac)

Comment 26

17 years ago
I found a couple of places which need change, sorry I did not catch them before.
* In GetCharset(), you can use ToNewUnicode().
* In case of conversion failure (both decode and encode), the buffer is not
freed, please fix it.

Comment 27

17 years ago
I don't know where is a point of your suggestion.
How can I fix ?
I think nsMemory::Free is used if the convertion failed.

>    rv = mEncoder->Convert(aSrc, &inLength, *_retval, &outLength);
>    if (NS_SUCCEEDED(rv))
>    {
>      (*_retval)[outLength] = '\0';
>      return NS_OK;
>    }
>    nsMemory::Free(*_retval);

Comment 28

17 years ago
You're right, my mistake.
For encoder, please call SetOutputErrorBehavior. Without this, the conversion
stops when it encounters a character which the encoder cannot convert. That
situation may happen because mozilla's widgets allow input in multiple scripts.

Comment 29

17 years ago
Created attachment 39523 [details] [diff] [review]

Comment 30

17 years ago
I modified and attach nsScriptableUnicodeConverter.cpp
* use mCharset.ToNewUnicode() in GetCharset
* add mEncoder->SetOutputErrorBehavior() in InitConverter()

Is this reflect your suggestion?

Comment 31

17 years ago
r=nhotta, please ask blizzard for a super review.
For detail, please check the page,
Can't you use NS_GENERIC_FACTORY_CONSTRUCTOR() for this with
nsModuleComponentInfo and NS_IMPL_NSGETMODULE()?

Also, what happens if someone calls SetCharset on the same object more than
once?  It won't work but it won't return an error either.  It looks like it
would be trivial to change it so that InitConverter() could reset the charset

Also, InitConverter() should return an error in case something goes wrong in
that function so it can be passed to the caller of SetCharset().

Comment 33

17 years ago
Created attachment 39924 [details] [diff] [review]

Comment 34

17 years ago
Created attachment 39926 [details] [diff] [review]

Comment 35

17 years ago
Created attachment 39928 [details] [diff] [review]
mozilla/intl/uconv/src/nsUConvModule.cpp  diff

Comment 36

17 years ago
Thanks, Blizzard.

I modified and attach 3 files.
Hotta-san, could you review again?

* add NS_GENERIC_FACTORY_CONSTRUCTOR(nsScriptableUnicodeConverter),
  remove NewScriptableUnicodeConverter(),
  and replace NS_NewScriptableUnicodeConverter  to 

* InitConverter and SetCharset returns nsresult

Comment 37

17 years ago

Does Blizzard said me to rewrite nsUConvModule.cpp with NS_IMPL_NSGETMODULE?
Many lines of nsUConvModule.cpp will be changed...?

Comment 38

17 years ago
cc to blizzard

Comment 39

17 years ago
Ryoichi:  I have a couple of bugs to use the NS_GETMODULE().
74438 nsUconvModule - need to use NS_GETMODULE 
74439 nsLocaleModule - need to use NS_GETMODULE

>Many lines of nsUConvModule.cpp will be changed...?
Yes, but you don't need to change uConv right now. 
I'll take care of it after you add your modifications.

Comment 40

17 years ago
r=nhotta for 06/25/01 08:38 and 06/25/01 08:39.
You define this more than once:

#define NS_ISCRIPTABLEUNICODECONVERTER_CID { 0xEC42D9B0, 0xB269, 0x48e1, { 0xa7,
0x7a, 0x2e, 0x56, 0x24, 0xda, 0x1f, 0x67 } }

Please put those in a header file somewhere and use it from there.  As near as I
can tell removing them from nsScriptableUnicodeConverter.cpp should be fine.

Other than that you should be all set with an sr=blizzard

Comment 42

17 years ago
Created attachment 43365 [details] [diff] [review]

Comment 43

17 years ago
I attached nsScriptableUnicodeConverter.cpp (reflected Blizzard's review)

* Remove 2 lines

* change GetUnicode() to get() by  bug 88413

Comment 44

17 years ago
Created attachment 43367 [details] [diff] [review]
nsScriptableUnicodeConverter.cpp (07/24/01 07:24 is mistake)

Comment 45

17 years ago
I can help checking in the files.
Here is a list of files and creation dates of the patches which I think I can
check in. Are those correct?

nsIScriptableUnicodeConverter.idl - 10/04/00 15:37
nsScriptableUnicodeConverter.cpp - 07/24/01 07:38
nsScriptableUnicodeConverter.h - 06/25/01 08:38
nsUConvModule.cpp - 06/25/01 08:46 - 06/14/01 10:52
also need mac project file change

Comment 46

17 years ago
Yes, a patch list is correct.

hotta-san, could you check patches and check in?

Comment 47

17 years ago
Furukawa san, it would be very helpful if you could attach a patch which
contains all the changes (excluding the new files). Could you do that?

Comment 48

17 years ago
Created attachment 43655 [details] [diff] [review]
proposed patch under mozilla/intl/uconv

Comment 49

17 years ago
Created attachment 43656 [details] [diff] [review]

Comment 50

17 years ago
Hotta-san, I don't know how can I make a patch containing all patches.

I used 'cvs diff -p1 mozilla/intl/uconv'. Is it correct way?

a patch file [2001-07-26 06:02] is containing 
+ mozilla/intl/uconv/idl/
+ mozilla/intl/uconv/idl/
+ mozilla/intl/uconv/src/
+ mozilla/intl/uconv/src/
+ mozilla/intl/uconv/src/nsUConvModule.cpp


I put nsScriptableUnicodeConverter.h [07/26/01 06:03] 
I removed definitions
(nsIScriptableUnicodeConverter.idl has these definitions,
so compiler output warning.)

So, please use 4 files below.
nsIScriptableUnicodeConverter.idl [10/04/00 15:37]
nsScriptableUnicodeConverter.h    [07/26/01 06:03]
nsScriptableUnicodeConverter.cpp  [07/24/01 07:38]
patches under mozilla/intl/uconv  [07/26/01 06:02]

Comment 51

17 years ago
Roy will help checking in the files.
Assignee: oliver → yokoyama
Target Milestone: mozilla1.0 → mozilla0.9.4

Comment 52

17 years ago
accepting. I'll check into the trunk as soon as the tree is open.
Thanks Furukawa-san for putting the patch together.
Whiteboard: /r=nhotta,/sr=blizzard

Comment 53

17 years ago
I had a compiler error saying can't find NS_ISCRIPTABLEUNICODECONVERTER_CID.
nsIScriptableUnicodeConverter.idl [10/04/00 15:37] has
should read

I'll post the all-in-one patch in a minute and will check
in with mac project file

Comment 54

17 years ago
Created attachment 44274 [details] [diff] [review]
all-in-one patch (including new files)

Comment 55

17 years ago
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 56

17 years ago
Kato-san, please verify this.  Thanks.
QA Contact: teruko → m_kato

Comment 57

17 years ago
nsScriptableUnicodeConverter::ConvertFromUnicode() doesn't work well.
I posted bug 114923.

You need to log in before you can comment on or make changes to this bug.