need unicode converter interfaces for script

RESOLVED FIXED in mozilla0.9.4

Status

()

P3
normal
RESOLVED FIXED
18 years ago
17 years ago

People

(Reporter: m_kato, Assigned: tetsuroy)

Tracking

Trunk
mozilla0.9.4
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(18 attachments)

IDL
1.75 KB, text/plain
Details
5.20 KB, text/plain
Details
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
(Reporter)

Description

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

Updated

18 years ago
Blocks: 41564

Updated

18 years ago
Status: UNCONFIRMED → NEW
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
(Reporter)

Comment 3

18 years ago
Created attachment 16160 [details]
IDL
(Reporter)

Comment 4

18 years ago
Created attachment 17245 [details]
mozilla/intl/uconv/src/nsScriptableUnicodeConverter.cpp
(Reporter)

Comment 5

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

Updated

18 years ago
Status: NEW → ASSIGNED

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!
(Reporter)

Comment 7

18 years ago
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

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

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
>cata@netscape.com 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 → ---
(Reporter)

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

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

Comment 13

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

Comment 14

18 years ago
Changing QA contact back to teruko@netscape.com.
QA Contact: andreasb → teruko

Comment 15

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

Comment 16

17 years ago
Created attachment 38437 [details] [diff] [review]
mozilla/intl/uconv/src/nsScriptableUnicodeConverter.h

Comment 17

17 years ago
Created attachment 38441 [details] [diff] [review]
diff mozilla/intl/uconv/src/makefile.win Makefile.in

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?
(Assignee)

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]
nsScriptableUnicodeConverter.cpp

Comment 24

17 years ago
Created attachment 39305 [details] [diff] [review]
nsScriptableUnicodeConverter.h

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]
nsScriptableUnicodeConverter.cpp

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?
Status: NEW → ASSIGNED

Comment 31

17 years ago
r=nhotta, please ask blizzard for a super review.
For detail, please check the page,
http://www.mozilla.org/hacking/reviewers.html
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

17 years ago
Created attachment 39924 [details] [diff] [review]
nsScriptableUnicodeConverter.h

Comment 34

17 years ago
Created attachment 39926 [details] [diff] [review]
nsScriptableUnicodeConverter.cpp

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 
nsScriptableUnicodeConverterConstructor

* InitConverter and SetCharset returns nsresult


Comment 37

17 years ago
more,

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
(Assignee)

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_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

17 years ago
Created attachment 43365 [details] [diff] [review]
nsScriptableUnicodeConverter.cpp

Comment 43

17 years ago
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

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
makefile.win Makefile.in - 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]
nsIScriptableUnicodeConverter.h

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/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

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

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.
Status: NEW → ASSIGNED
Whiteboard: /r=nhotta,/sr=blizzard
(Assignee)

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
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
(Assignee)

Comment 54

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

Comment 55

17 years ago
checked-in
Status: ASSIGNED → RESOLVED
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.