114923 - Unicode converter doesn't work well for the second time

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Koike Kazuhiko]

nsScriptableUnicodeConverter::ConvertFromUnicode() doesn't work well
for the second time.

ChatZilla(0.8.5-rc5) calls ConvertFromUnicode() for outgoing messages.
The first string in iso-2022-jp is converted correctly, but the second
string isn't converted. See bug 41564.

I think ConvertFromUnicode() should call mEncoder->Finish()
after mEncoder->Convert().

Comment 1

[nhottanscp]

KOIKE san, I think you are right, need Finish() call. 
Can anyone in Mozilla JA work on this?

Comment 2

[Koike Kazuhiko]

Attachment: patch

I added mEncoder->Finish() after mEncoder->Convert(),
like ConvertFromUnicode() in nsMsgI18N.cpp. (Why does similar function exist?)

I haven't tested this patch yet. CVS build can't connect to IRC server.
"Connection to moznet (irc.mozilla.org:6667) closed with status (null)."

Comment 3

[Koike Kazuhiko]

Attachment: Remove unnecessary initialization in ChatZilla

I tested my patch. It seems to work fine.

Initialization before ConvertFromUnicode() isn't necessary.

Comment 4

[Frank Tang]

the reason we have Finish method is because we are aussumming working under a
STREAM base environment- which mean we may not have every data we need untill
the Finish got called. We do NOT work to switch back to the ASCII mode too
eariler. For this particular usage, I am not 100% sure this is the right way to
solve it. Should we add a Finish method to the interface ?

Comment 5

[Frank Tang]

reassign back to [e-mail], I think you should propose a patch and
ask nhotta to r= it, and then you can ask super reviewer to sr= it and check in
that fix. (if you do not have cvs check in right, then ask someone who have to
check in for you. and eventually ask for cvs check in right)

Comment 6

[Koike Kazuhiko]

I think we should add stateless ConvertFromUnicode() along with
stateful ConvertFromUnicode(). Stateless ConvertFromUnicode()
has initializing and finishing functions in it. It's simpler
than current stateful ConvertFromUnicode() and enough for Chatzilla.

Hotta-san, how do you think?

Comment 7

[nhottanscp]

Adding another conversion methods might be confusing. Is it possible to add
Finish method like ftang mentioned in comment #4?

Comment 8

[Koike Kazuhiko]

Here's my patch I'm testing now.
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=1340&action=edit

Comment 9

[nhottanscp]

>+  string Finish([const] in string aSrc);
In the patch, what is supposed to be passed for the input aSrc?

Comment 10

[Koike Kazuhiko]

Attachment: patch

aSrc parameter isn't necessary for Finish().

Comment 11

[Koike Kazuhiko]

Attachment: patch

The previous patch doesn't free the allocated buffer.

Comment 12

[nhottanscp]

The other way is make Finish() to take memory/length since
nsIUnicodeEncoder::GetMaxLength includes the terminal characters. Then the
caller can pass the same buffer used for Convert() with adding an offset of the
consumed length, so Finish() does not need to allocate memory and estimate the
finish buffer length. But I realized that that is probably not good for this
scriptable interface.

About the memory allocation in the patch, it does not need to allocate buffer
twice. I think it is okay to leave some extra bytes allocated.

Comment 13

[Koike Kazuhiko]

Attachment: patch

I don't know how many bytes should be allocated for the buffer.
Is there constant defined for that?

Comment 14

[nhottanscp]

Comment on attachment 106136 [details] [diff] [review]
patch

That is something I mentioned in my last comment, we cannot get the length
estimation without the context of the last conversion.
I think 10 is fine that should cover a terminator plus possible data remains in
the internal buffer but you may increase to 32 (or larger) just to make sure.

Please change +  if (!NS_SUCCEEDED(rv))
to something like,

if (NS_SUCCEEDED(rv))
  (*_retval)[finLength] = '\0';
else
  nsMemory::Free(*_retval);

return rv;

Comment 15

[Koike Kazuhiko]

Attachment: patch

Comment 16

[Koike Kazuhiko]

Adding Shanjian Li and Boris Zbarsky. Please super-review the patch.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 106145 [details] [diff] [review]
patch

Please add comments to the interface explaining what this function is and when
it should or should not be used (eg after ConvertFromUnicode?).

Oh, and use the request tracker instead of ccing people; that's what it's for.

Comment 18

[Koike Kazuhiko]

Attachment: patch

Added a desciption about Finish().

Comment 19

[nhottanscp]

The patch was checked in to the trunk.

Comment 20

[Daniel Bratell]

Who is calling the Finish method?

Should this bug be closed?

Comment 21

[Matthias Versen [:Matti]]

marking fixed based on comment #19.
Thanks [e-mail] for the patch !

Comment 22

[Teruko Kobayashi]

Verified.