Cache charset for nsMsgI18NGetDefaultMailCharset

RESOLVED FIXED in mozilla0.9.7

Status

RESOLVED FIXED
17 years ago
10 years ago

People

(Reporter: nhottanscp, Assigned: nhottanscp)

Tracking

Trunk
mozilla0.9.7
x86
Windows 2000

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

17 years ago
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.
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.7
(Assignee)

Comment 1

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


(Assignee)

Comment 2

17 years ago
Created attachment 58165 [details] [diff] [review]
Cache default charset to the class.
(Assignee)

Comment 3

17 years ago
Created attachment 58169 [details] [diff] [review]
For search, cache default charset to the class. For eudora, changed to get a pref directly.
Attachment #58165 - Attachment is obsolete: true

Comment 4

17 years ago
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.
Attachment #58169 - Flags: review+
(Assignee)

Comment 5

17 years ago
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.
(Assignee)

Comment 6

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

Looks good. R=ducarroz
Attachment #58404 - Flags: review+

Comment 8

17 years ago
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.
(Assignee)

Comment 9

17 years ago
Created attachment 59964 [details] [diff] [review]
Revised 58404 with bienvenu's comment.
Attachment #58404 - Attachment is obsolete: true

Comment 10

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

sr=bienvenu
Attachment #59964 - Flags: superreview+

Comment 11

17 years ago
+  if (!m_defaultCharset)

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

Comment 12

17 years ago
Created attachment 60021 [details] [diff] [review]
Revised 58169 with bienvenu's comment.
Attachment #58169 - Attachment is obsolete: true
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.
(Assignee)

Comment 14

17 years ago
Right, they cannot be inlined, thanks for the fix.

Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 15

17 years ago
Naoki, could you help verifying this? thanks.
QA Contact: ji → nhotta
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.