112904 - MimeInlineText's charset need to be initialized before setting font

Status: VERIFIED FIXED

(MailNews Core :: Internationalization, defect)

Description

[Shanjian Li]

This bug was found by Jungshik when he was working with bug 102623. 
He wrote:
There's a very strange problem unrelated with my
patch that caused my patch not to work.  
In mimemoz2.cpp, charset is always set to "us-ascii"

1937 nsresult GetMailNewsFont(MimeObject *obj, PRBool styleFixed, char
*fontName, PRUint32 nameBuffSize, 
1938 PRInt32 *fontPixelSize, PRInt32 *fontSizePercentage)
1939 {
1940 nsresult rv = NS_OK;
1941 1942 nsIPref *aPrefs = GetPrefServiceManager(obj->options);
1943 if (aPrefs) {
1944 MimeInlineText *text = (MimeInlineText *) obj;
1945 nsCAutoString aCharset;
1946 PRUnichar *unicode = nsnull;
1947 nsCAutoString convertedStr;
1948 nsCAutoString variableFontType;   // serif, sans-serif
1949 
1950 // get a charset
1951 if (!text->charset || !(*text->charset))
1952 aCharset.Assign("us-ascii");
1953 else
1954 aCharset.Assign(text->charset);

It used to work fine, but somehow text->charset
is NULL for SC, J and K messages and later langGroup
is set to x-western because charset is 'us-ascii'.

Therefore, to see my code work, I have to hardcode
charset to 'gb2312', 'iso-2022-jp', and 'euc-kr'
in place of 'us-ascii'.

Comment 1

[Shanjian Li]

This bug was introduced with one of my mime charset change. I have to 
delay the initialization of charset after MimeInlineText_initialize
since some information is not available at that time (namely 'option').

Comment 2

[Shanjian Li]

Attachment: patch

Comment 3

[Shanjian Li]

ducarroz, seth,
Could you give r/sr? thanks.

Comment 4

[Jungshik Shin]

Shanjian,
Thanks again for the fix. 
As for your patch, how about adding if-statement like this:
 if (!text->initializeCharset)
in mimemoz2.cpp and moving it after 
'MimeInlineText  *text = (MimeInlineText *) obj;'
Just thought that may be safer (no difference right now,
but just in case in the future initialize_charset() is
called earlier in another place).

my 0.02 :-)

Comment 5

[Shanjian Li]

Attachment: updated patch

Comment 6

[Shanjian Li]

Jungshik, 
You are right. Thought it is not a problem at this time, it is a good 
idea to put the if statement there. Thanks for correcting me that. 
I updated my patch.

Comment 7

[Jean-Francois Ducarroz]

Comment on attachment 59946 [details] [diff] [review]
updated patch

R=ducarroz

Comment 8

[(not reading, please use [e-mail] instead)]

Comment on attachment 59946 [details] [diff] [review]
updated patch

sr=sspitzer

Comment 9

[Shanjian Li]

fix checked in. 

Comment 10

[ji]

Shanjian, could you help verifying this? Thanks a lot.

Comment 11

[Shanjian Li]

verified in debugger.