54857 - need unicode converter interfaces for script

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P3)

Description

[Makoto Kato [:m_kato]]

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

Comment 1

[nhottanscp]

Reassign to ftang, cc to cata.

Comment 2

[Frank Tang]

hum... can you propose a idl for this ?

Comment 3

[Makoto Kato [:m_kato]]

Attachment: IDL

Comment 4

[Makoto Kato [:m_kato]]

Attachment: mozilla/intl/uconv/src/nsScriptableUnicodeConverter.cpp

Comment 5

[Makoto Kato [:m_kato]]

Attachment: patches of mozilla/intl

Comment 6

[cata]

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

[Makoto Kato [:m_kato]]

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

Comment 8

[Frank Tang]

move all cata's bug to ftang

Comment 9

[Frank Tang]

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.

Comment 10

[nhottanscp]

>[e-mail] 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.

Comment 11

[Makoto Kato [:m_kato]]

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

Comment 12

[nhottanscp]

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

Comment 13

[Teruko Kobayashi]

Changed QA contact to [e-mail] for now.

Comment 14

[Andreas Becker]

Changing QA contact back to [e-mail].

Comment 15

[Ryoichi Furukawa]

Attachment: mozilla/intl/uconv/src/nsScriptableUnicodeConverter.cpp

Comment 16

[Ryoichi Furukawa]

Attachment: mozilla/intl/uconv/src/nsScriptableUnicodeConverter.h

Comment 17

[Ryoichi Furukawa]

Attachment: diff mozilla/intl/uconv/src/makefile.win Makefile.in

Comment 18

[Ryoichi Furukawa]

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

[Ryoichi Furukawa]

Attachment: mozilla/intl/uconv/src/nsUConvModule.cpp (I forgot one)

Comment 20

[Koike Kazuhiko]

Reassigning to Furukawa-san.

Could somebody review the patch?

Comment 21

[nhottanscp]

* 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

[Roy Yokoyama]

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

Comment 23

[Ryoichi Furukawa]

Attachment: nsScriptableUnicodeConverter.cpp

Comment 24

[Ryoichi Furukawa]

Attachment: nsScriptableUnicodeConverter.h

Comment 25

[Ryoichi Furukawa]

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

[nhottanscp]

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

[Ryoichi Furukawa]

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

[nhottanscp]

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

[Ryoichi Furukawa]

Attachment: nsScriptableUnicodeConverter.cpp

Comment 30

[Ryoichi Furukawa]

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

Is this reflect your suggestion?

Comment 31

[nhottanscp]

r=nhotta, please ask blizzard for a super review.
For detail, please check the page,
http://www.mozilla.org/hacking/reviewers.html

Comment 32

[Christopher Blizzard (:blizzard)]

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

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

[Ryoichi Furukawa]

Attachment: nsScriptableUnicodeConverter.h

Comment 34

[Ryoichi Furukawa]

Attachment: nsScriptableUnicodeConverter.cpp

Comment 35

[Ryoichi Furukawa]

Attachment: mozilla/intl/uconv/src/nsUConvModule.cpp diff

Comment 36

[Ryoichi Furukawa]

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 
nsScriptableUnicodeConverterConstructor

* InitConverter and SetCharset returns nsresult

Comment 37

[Ryoichi Furukawa]

more,

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

Comment 38

[nhottanscp]

cc to blizzard

Comment 39

[Roy Yokoyama]

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

[nhottanscp]

r=nhotta for 06/25/01 08:38 and 06/25/01 08:39.

Comment 41

[Christopher Blizzard (:blizzard)]

You define this more than once:

#define NS_ISCRIPTABLEUNICODECONVERTER_CONTRACTID
"@mozilla.org/nsIScripatableUnicodeConverter;1"
#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

[Ryoichi Furukawa]

Attachment: nsScriptableUnicodeConverter.cpp

Comment 43

[Ryoichi Furukawa]

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

* Remove 2 lines
   NS_ISCRIPTABLEUNICODECONVERTER_CONTRACTID
   NS_ISCRIPTABLEUNICODECONVERTER_CID

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

Comment 44

[Ryoichi Furukawa]

Attachment: nsScriptableUnicodeConverter.cpp (07/24/01 07:24 is mistake)

Comment 45

[nhottanscp]

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
makefile.win Makefile.in - 06/14/01 10:52
also need mac project file change

Comment 46

[Ryoichi Furukawa]

Yes, a patch list is correct.

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

Comment 47

[nhottanscp]

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

[Ryoichi Furukawa]

Attachment: proposed patch under mozilla/intl/uconv

Comment 49

[Ryoichi Furukawa]

Attachment: nsIScriptableUnicodeConverter.h

Comment 50

[Ryoichi Furukawa]

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/Makefile.in
+ mozilla/intl/uconv/idl/makefile.win
+ mozilla/intl/uconv/src/Makefile.in
+ mozilla/intl/uconv/src/makefile.win
+ mozilla/intl/uconv/src/nsUConvModule.cpp

and,

I put nsScriptableUnicodeConverter.h [07/26/01 06:03] 
I removed definitions
  NS_ISCRIPTABLEUNICODECONVERTER_CONTRACTID and
  NS_ISCRIPTABLEUNICODECONVERTER_CID
(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

[nhottanscp]

Roy will help checking in the files.

Comment 52

[Roy Yokoyama]

accepting. I'll check into the trunk as soon as the tree is open.
Thanks Furukawa-san for putting the patch together.

Comment 53

[Roy Yokoyama]

I had a compiler error saying can't find NS_ISCRIPTABLEUNICODECONVERTER_CID.
 
nsIScriptableUnicodeConverter.idl [10/04/00 15:37] has
typo. 
#define NS_SCRIPTABLEUNICODECONVERTER_CID 
should read
#define NS_ISCRIPTABLEUNICODECONVERTER_CID

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

Comment 54

[Roy Yokoyama]

Attachment: all-in-one patch (including new files)

Comment 55

[Roy Yokoyama]

checked-in

Comment 56

[Teruko Kobayashi]

Kato-san, please verify this.  Thanks.

Comment 57

[Koike Kazuhiko]

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