110127 - Cache charset for nsMsgI18NGetDefaultMailCharset

Status: RESOLVED FIXED

(MailNews Core :: Internationalization, defect)

Description

[nhottanscp]

The function gets a mail compose default charset from pref.
It can be cached and hook up notification for changes instead of read the value
everytime. We also want to change the function name to say explicitely this is
for compose default instead of view default, e.g.
nsMsgI18NGetDefaultMailCompseCharset.

Comment 1

[nhottanscp]

Currently there are three places which call the function. I think two of them
(search and eudora) use it incorrectly. What they need is view_default not
send_default.

/mailnews/base/search/src/nsMsgSearchAdapter.cpp, line 326
/mailnews/compose/src/nsMsgCompFields.cpp, line 82
/mailnews/import/eudora/src/nsEudoraCompose.cpp, line 623

My current plan is to remove nsMsgI18NGetDefaultMailCharset and add a function
to get send_default to nsIMsgCompose. By adding to nsIMsgCompose, .js files
which reads send_default from pref also take advantage of the cached value.
For Eudora, I think it just need to read view_default from the pref no cache is
needed.
For search, cache view_default in the search class.

cc to ducarroz

Comment 2

[nhottanscp]

Attachment: Cache default charset to the class.

Comment 3

[nhottanscp]

Attachment: For search, cache default charset to the class. For eudora, changed to get a pref directly.

Comment 4

[Navin Gupta]

Comment on attachment 58169 [details] [diff] [review]
For search, cache default charset to the class. For eudora, changed to get a pref directly.

r=naving

looks ok to me, but could 
you explain why we need
srcCharset and dstCharset
in SearchAdapter.

Comment 5

[nhottanscp]

For local search, UI charset is unique (from the pref default) but the target
charset may vary based on the folder charset (the user can assign different
charset per folder), so need to have two charsets.

Comment 6

[nhottanscp]

Attachment: Cache default charset in nsMsgCompFields, removed nsMsgI18NGetDefaultMailCharset.

Comment 7

[Jean-Francois Ducarroz]

Comment on attachment 58404 [details] [diff] [review]
Cache default charset in nsMsgCompFields, removed nsMsgI18NGetDefaultMailCharset.

Looks good. R=ducarroz

Comment 8

[David :Bienvenu]

I prefer using IsEmpty() for the following code with nsXPIDLCString:
+    if (charset)
+      m_DefaultCharacterSet.AssignWithConversion(charset);
+    else
+      m_DefaultCharacterSet.Assign("ISO-8859-1");

so if (charset.IsEmpty())
      m_DefaultCharacterSet.Assign("ISO-8859-1");
   else
      m_DefaultCharacterSet.AssignWithConversion(charset);

Other than that, looks OK.

Comment 9

[nhottanscp]

Attachment: Revised 58404 with bienvenu's comment.

Comment 10

[David :Bienvenu]

Comment on attachment 59964 [details] [diff] [review]
Revised 58404 with bienvenu's comment.

sr=bienvenu

Comment 11

[David :Bienvenu]

+  if (!m_defaultCharset)

change it to if (m_defaultCharset.IsEmpty()) and sr=bienvenu

Comment 12

[nhottanscp]

Attachment: Revised 58169 with bienvenu's comment.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 59964 [details] [diff] [review]
Revised 58404 with bienvenu's comment.

I checked in a fix for the OS/2 bustage following the checkin of this patch by
changing |inline nsresult| to |NS_IMETHODIMP| for
nsMsgCompFields::GetDefaultCharacterSet, which can't be inlined anyway.

Comment 14

[nhottanscp]

Right, they cannot be inlined, thanks for the fix.

Comment 15

[ji]

Naoki, could you help verifying this? thanks.