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

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

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

I tested my patch. It seems to work fine. Initialization before ConvertFromUnicode() isn't necessary.

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 ?

reassign back to kazhik@mozilla.gr.jp, 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)

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?

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

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

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

aSrc parameter isn't necessary for Finish().

The previous patch doesn't free the allocated buffer.

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.

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

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;

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

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.

Added a desciption about Finish().

The patch was checked in to the trunk.

Who is calling the Finish method? Should this bug be closed?

marking fixed based on comment #19. Thanks kazhik@mozilla.gr.jp for the patch !

Verified.