Bug 943252 removed nsCharsetMenu. This made some pop-up menus in prefs appear empty. At least the incoming and outgoing encoding pop-up for email need fixing and the per-server encoding setting for NNTP needs fixing. Attachment 8410848 [details] [diff] contains an addition of nsCharsetMenu to c-c. However, I recommend taking the opportunity for getting rid of RDF badness and instead hardcoding the pref pop-ups in XUL in the manner of http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/fonts.xul#264

<http://dxr.mozilla.org/comm-central/search?q=rdf%3Acharset-menu&case=true> has a more complete list of affected menus.

Working on the /mail -and related /mailnews code-. When I finish with that, I will post here the patch so someone else could try with the /suite dir.

Is going mailnews.send_default_charset to be deprecated?

(In reply to Javi Rueda from comment #3) > Is going mailnews.send_default_charset to be deprecated? No. At least not until we can convince the jp locale to suck it up and change their default from ISO-2022-JP to UTF-8. Hell, they insisted that we added in code to make silently falling back to UTF-8 difficult for the Japanese locale (effectively), see bug 448842.

Some Japanese people insist ISO-2022-JP because of RFC 1468. We should lobby IETF to make it HISTORICAL.

(In reply to Masatoshi Kimura [:emk] from comment #5) > Some Japanese people insist ISO-2022-JP because of RFC 1468. We should lobby > IETF to make it HISTORICAL. I doubt IETF is going to make that change, since ISO-2022-JP is still a charset in active use. RFC 6532 makes it very clear that UTF-8 is by far the most preferred charset for the changes. [As a side note, is it now feasible to remove the mailnews.disable_fallback_to_utf8 gunk and unconditionally fallback to UTF-8 silently?]

(In reply to Joshua Cranmer [:jcranmer] from comment #6) > [As a side note, is it now feasible to remove the > mailnews.disable_fallback_to_utf8 gunk and unconditionally fallback to UTF-8 > silently?] Mac OS X Mail turns charset to UTF-8 silently, but people install the LetterFix add-on to avoid mojibake. So no, I don't think we can remove the mailnews.disable_fallback_to_utf8 pref yet :(

Comment on attachment 8424525 [details] [diff] [review] Fix for the Mail Display preference Most of the changes are explained on the code itself. I have deleted preference intl.charset.default as it wasn't used. mailnews.(send/view)_default_charset are instantApply=true now. The problem was that if it was =false, when preference was on its default value, it was needed to select 2 times a new character encoding in order to change the item selected on the menulist pop-up. Once it wasn't "default", the selected item would be changed as expected. Preference value is changed, though.

This is a merged patch with changes from previous patch sent. It fixes also the problem on the Account Settings dialog box for NNTP accounts and the Folder Properties charset field on the folder properties dialog box. Previous patch removed int.charset.default preference on fonts.xul as it was unused. Now, as that is another problem (or, maybe not at all), this change has been removed and I shall create a new bug filing. All three parts are fixed almost the same way. Nevertheless, a difference in the quantity of items in the menulist for the folderProps and am-server files. On my tests, I learnt that this field can contain "ISO-8859-1", which wasn't included on the code in http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/fonts.xul#264, so I added it. Doing a search, I found that ISO-8859-1 charset corresponds to Latin. No JS code needed for am-server part. On the folderProps.js, I added some debug code. In the case that a folder's charset was unknown -i.e. there is no localized menu for it-, its value will be used, and menu-item show, as expected, an empty string. A debug message will be logged into Error Console so, if someone complains about it, we may add it, as the message will contain which charset to add. I have changed the reviewer to Magnus, as it seems he has less work on his queue. Could you take a look into this, please? Thank you.

Comment on attachment 8427418 [details] [diff] [review] Merged bug with remaining changes for /mail and /mailnews Review of attachment 8427418 [details] [diff] [review]: ----------------------------------------------------------------- The fact that you are copying names for charset labels to several different places is a distinct anti-pattern, and I cannot endorse that approach.

I am moving all charset entities into a unique new DTD file and referencing it from the XUL files instead, Joshua.

I have just found new file charsetTitles.properties that was introduced with bug 1006498. May I use this one, instead of using those labels?

Comment on attachment 8427418 [details] [diff] [review] Merged bug with remaining changes for /mail and /mailnews Review of attachment 8427418 [details] [diff] [review]: ----------------------------------------------------------------- I don't like how the menuitems are hardcoded and duplicated in each menulist and also all the strings in each .dtd file. But we can't argue about the perfect fix for too long while TB and SM is broken. If this is decided to be just a quick temporary measure to fix c-c we can make it better later (e.g. making a common widget/binding for it like the folder picker is or have a JS module generate the menuitems). ::: mail/components/preferences/fonts.js @@ +138,4 @@ > { > + var Ci = Components.interfaces; > + var prefs = Components.classes["@mozilla.org/preferences-service;1"] > + .getService(Ci.nsIPrefBranch); Please use Services.prefs in /mail/* files .

(In reply to Javi Rueda from comment #13) > I have just found new file charsetTitles.properties that was introduced with > bug 1006498. May I use this one, instead of using those labels? That should be sufficient.

Comment on attachment 8427418 [details] [diff] [review] Merged bug with remaining changes for /mail and /mailnews Review of attachment 8427418 [details] [diff] [review]: ----------------------------------------------------------------- Note that Firefox's fallback encoding UI is designed for *incoming data* in the *Web* context. So just reusing that is wrong in the *email* context and even more wrong when dealing with *outgoing* data. ::: mail/components/preferences/fonts.xul @@ +289,2 @@ > <menupopup id="sendDefaultCharset-menupopup"> > + <menuitem label="&languages.customize.Fallback.auto;" value=""/> The setting for *sending* (but not the setting for *receiving*) should have UTF-8 as the easiest-to-choose option. The magic value "" makes sense in the context of the Firefox code you copied from but does it make sense here? Why do you have the "auto" item? @@ +290,5 @@ > + <menuitem label="&languages.customize.Fallback.auto;" value=""/> > + <menuitem label="&languages.customize.Fallback.arabic;" value="windows-1256"/> > + <menuitem label="&languages.customize.Fallback.baltic;" value="windows-1257"/> > + <menuitem label="&languages.customize.Fallback.ceiso;" value="ISO-8859-2"/> > + <menuitem label="&languages.customize.Fallback.cewindows;" value="windows-1250"/> Is there any evidence that it would be useful to support the above 4 encodings as the default for outgoing email? @@ +291,5 @@ > + <menuitem label="&languages.customize.Fallback.arabic;" value="windows-1256"/> > + <menuitem label="&languages.customize.Fallback.baltic;" value="windows-1257"/> > + <menuitem label="&languages.customize.Fallback.ceiso;" value="ISO-8859-2"/> > + <menuitem label="&languages.customize.Fallback.cewindows;" value="windows-1250"/> > + <menuitem label="&languages.customize.Fallback.simplified;" value="gbk"/> Is it intentional that this menu doesn't allow Thunderbird to be configured to send gb18030? (Note that in Gecko, gbk *decodes* exactly like gb18030, but the encoders are different.) @@ +292,5 @@ > + <menuitem label="&languages.customize.Fallback.baltic;" value="windows-1257"/> > + <menuitem label="&languages.customize.Fallback.ceiso;" value="ISO-8859-2"/> > + <menuitem label="&languages.customize.Fallback.cewindows;" value="windows-1250"/> > + <menuitem label="&languages.customize.Fallback.simplified;" value="gbk"/> > + <menuitem label="&languages.customize.Fallback.traditional;" value="Big5"/> Is there any evidence that it's useful to provide this option? Traditional Chinese Thunderbird already defaults to UTF-8. @@ +293,5 @@ > + <menuitem label="&languages.customize.Fallback.ceiso;" value="ISO-8859-2"/> > + <menuitem label="&languages.customize.Fallback.cewindows;" value="windows-1250"/> > + <menuitem label="&languages.customize.Fallback.simplified;" value="gbk"/> > + <menuitem label="&languages.customize.Fallback.traditional;" value="Big5"/> > + <menuitem label="&languages.customize.Fallback.cyrillic;" value="windows-1251"/> Is there any evidence that it's useful to provide this option? Russian Thunderbird defaults to UTF-8 already. @@ +295,5 @@ > + <menuitem label="&languages.customize.Fallback.simplified;" value="gbk"/> > + <menuitem label="&languages.customize.Fallback.traditional;" value="Big5"/> > + <menuitem label="&languages.customize.Fallback.cyrillic;" value="windows-1251"/> > + <menuitem label="&languages.customize.Fallback.greek;" value="ISO-8859-7"/> > + <menuitem label="&languages.customize.Fallback.hebrew;" value="windows-1255"/> Is there any evidence that it's useful to provide this option? (As opposed to using UTF-8 for Hebrew.) @@ +296,5 @@ > + <menuitem label="&languages.customize.Fallback.traditional;" value="Big5"/> > + <menuitem label="&languages.customize.Fallback.cyrillic;" value="windows-1251"/> > + <menuitem label="&languages.customize.Fallback.greek;" value="ISO-8859-7"/> > + <menuitem label="&languages.customize.Fallback.hebrew;" value="windows-1255"/> > + <menuitem label="&languages.customize.Fallback.japanese;" value="Shift_JIS"/> Even though Shift_JIS makes sense in the Web context, in the email context, the Japanese legacy encoding is ISO-2022-JP. @@ +298,5 @@ > + <menuitem label="&languages.customize.Fallback.greek;" value="ISO-8859-7"/> > + <menuitem label="&languages.customize.Fallback.hebrew;" value="windows-1255"/> > + <menuitem label="&languages.customize.Fallback.japanese;" value="Shift_JIS"/> > + <menuitem label="&languages.customize.Fallback.korean;" value="EUC-KR"/> > + <menuitem label="&languages.customize.Fallback.thai;" value="windows-874"/> Is there any evidence that it's useful to provide this option? (As opposed to using UTF-8 for Thai.) @@ +299,5 @@ > + <menuitem label="&languages.customize.Fallback.hebrew;" value="windows-1255"/> > + <menuitem label="&languages.customize.Fallback.japanese;" value="Shift_JIS"/> > + <menuitem label="&languages.customize.Fallback.korean;" value="EUC-KR"/> > + <menuitem label="&languages.customize.Fallback.thai;" value="windows-874"/> > + <menuitem label="&languages.customize.Fallback.turkish;" value="windows-1254"/> Is there any evidence that it's useful to provide this option? (As opposed to using UTF-8 for Turkish.) @@ +300,5 @@ > + <menuitem label="&languages.customize.Fallback.japanese;" value="Shift_JIS"/> > + <menuitem label="&languages.customize.Fallback.korean;" value="EUC-KR"/> > + <menuitem label="&languages.customize.Fallback.thai;" value="windows-874"/> > + <menuitem label="&languages.customize.Fallback.turkish;" value="windows-1254"/> > + <menuitem label="&languages.customize.Fallback.vietnamese;" value="windows-1258"/> Is there any evidence that it's useful to provide this option? (As opposed to using UTF-8 for Vietnamese.) Previously, Thunderbird didn't provide this encoding in its message compose menu. @@ +301,5 @@ > + <menuitem label="&languages.customize.Fallback.korean;" value="EUC-KR"/> > + <menuitem label="&languages.customize.Fallback.thai;" value="windows-874"/> > + <menuitem label="&languages.customize.Fallback.turkish;" value="windows-1254"/> > + <menuitem label="&languages.customize.Fallback.vietnamese;" value="windows-1258"/> > + <menuitem label="&languages.customize.Fallback.other;" value="windows-1252"/> Labeling windows-1252 "Other" doesn't make sense in the context of outgoing email. (It makes sense in the Web context for incoming legacy data.) @@ +317,2 @@ > <menupopup id="viewDefaultCharset-menupopup"> > + <menuitem label="&languages.customize.Fallback.auto;" value=""/> Does the magic value "" make sense given how this pref works? @@ +324,5 @@ > + <menuitem label="&languages.customize.Fallback.traditional;" value="Big5"/> > + <menuitem label="&languages.customize.Fallback.cyrillic;" value="windows-1251"/> > + <menuitem label="&languages.customize.Fallback.greek;" value="ISO-8859-7"/> > + <menuitem label="&languages.customize.Fallback.hebrew;" value="windows-1255"/> > + <menuitem label="&languages.customize.Fallback.japanese;" value="Shift_JIS"/> Should be ISO-2022-JP in the email context.

Comment on attachment 8427418 [details] [diff] [review] Merged bug with remaining changes for /mail and /mailnews Review of attachment 8427418 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/preferences/fonts.js @@ +138,4 @@ > { > + var Ci = Components.interfaces; > + var prefs = Components.classes["@mozilla.org/preferences-service;1"] > + .getService(Ci.nsIPrefBranch); Services.prefs yes, everywhere possible @@ +144,5 @@ > + // so "" menuitem on the menulist must be selected. > + if (!prefs.prefHasUserValue("mailnews.send_default_charset")) > + return ""; > + else > + return undefined; no else after return (here and elsewhere) if (foo) return ""; return bar; ::: mailnews/base/content/folderProps.js @@ +109,5 @@ > var folderCharsetList = document.getElementById("folderCharsetList"); > + > + // Log to the Error Console the charset value for the folder > + // if it is unknown to us. Value will be preserved by the menu-item. > + if (folderCharsetList.selectedIndex==-1) space around == @@ +117,5 @@ > + .getService(Ci.steelIApplication); > + var considerStr = "Charset encoding for folder is unknown to us. "; > + var considerStr2 = "Please consider filing a bug. (gMsgFolder.charset = "; > + var wholeStr = considerStr + considerStr2 + gMsgFolder.charset + ")"; > + Application.console.log(wholeStr); Components.utils.reportError("Unknown folder encoding; folder=" + gMsgFolder.name + ", charset=" + gMsgFolder.charset) (And Application is instantly usable, no need to set it up that way, which is even wrong for suite) But, do this actually ever even happen?

Please don't forget that if you make strings changes to files under mailnews you need add strings to SeaMonkey as well as Thunderbird. Thanks.

(In reply to Henri Sivonen (:hsivonen) from comment #16) > > ::: mail/components/preferences/fonts.xul > @@ +289,2 @@ > > <menupopup id="sendDefaultCharset-menupopup"> > > + <menuitem label="&languages.customize.Fallback.auto;" value=""/> > > The setting for *sending* (but not the setting for *receiving*) should have > UTF-8 as the easiest-to-choose option. > > The magic value "" makes sense in the context of the Firefox code you copied > from but does it make sense here? Why do you have the "auto" item? > Auto item represents the "Default" value for active locale. UTF encoding is not the default for all supported locales http://mxr.mozilla.org/l10n-central/search?string=mailnews.send_default_charset. Many of them use UTF-8, but some of them use different ones. That is the reason why it exists the empty value on the menu item. When it is synchronized from the preference, we will select the empty item and then, as it is not a legal value, will reset it when user presses OK.

Attachment 8431640 [details] [diff] is a reworked patch. Now it is using strings from charsetTitles.properties. Some labels have been removed from the menulist as previous comments asked for. I have tentatively requested a review from Merlin, but I think that another one from :mconley or :IanN would be needed, as it also tries to fix the problem on folderProps (which is bug 1006342).

Comment on attachment 8431640 [details] [diff] [review] 1003716.patch Review of attachment 8431640 [details] [diff] [review]: ----------------------------------------------------------------- I'm pretty happy with this, but i keep getting an exception in the console Timestamp: 31.05.2014 21.30.13 Error: preference.value is null Source File: chrome://messenger/content/preferences/fonts.js Line: 127 ::: mail/components/preferences/fonts.js @@ +170,2 @@ > { > + var prefs = Services.prefs; I wouldn't bother declaring this locally..

(In reply to Magnus Melin from comment #22) > Comment on attachment 8431640 [details] [diff] [review] > 1003716.patch > > Review of attachment 8431640 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm pretty happy with this, but i keep getting an exception in the console > > Timestamp: 31.05.2014 21.30.13 > Error: preference.value is null > Source File: chrome://messenger/content/preferences/fonts.js > Line: 127 > That exception appears also when patch is not applied. When testing my patch, I looked into Error Console and saw it. Then I mach clobber the obj dir. It was also appearing. So, I think it is unrelated to the patch attached here. > ::: mail/components/preferences/fonts.js > @@ +170,2 @@ > > { > > + var prefs = Services.prefs; > > I wouldn't bother declaring this locally.. If you mean using Services.prefs directly instead of declaring locally to the function, line length on the declaration of (send/view)CharsetStr will exceed 80 columns, which I have tried to do whenever I was able to.

Note to comment 23: Where it says "Then I mach clobber the obj dir" it should say "Then I mach clobber the obj dir and unapply the patch".

Ok, I didn't realize it was there already. You can always wrap it differently, e.g. var sendCharsetStr = Services.prefs.getComplexValue( "mailnews.send_default_charset", Ci.nsIPrefLocalizedString).data);

Previous patch with changes from comments by reviewer. Maybe a review by :squib for both folderProps.js and am-server.js is needed

Comment on attachment 8432250 [details] [diff] [review] Update character encoding-related pref UI not to use the nsCharsetMenu RDF data source I'm not a good reviewer for this; I don't know much about this part of the code.

Comment on attachment 8432250 [details] [diff] [review] Update character encoding-related pref UI not to use the nsCharsetMenu RDF data source Passing to Joshua, as he's already had his head in this area of the code and I'm unlikely to get to it for a while.

I won't be able to get to this review before next Friday at the earliest.

I was trying to do something about /suite. As the browser part could take a similar way of fixing it as bug 910192, I cloned it on Seamonkey/Preference bugzilla component. Then I found that there already exists bug 934492. So I have closed my duplicated one and will stop working on the /suite part.

Comment on attachment 8432250 [details] [diff] [review] Update character encoding-related pref UI not to use the nsCharsetMenu RDF data source Review of attachment 8432250 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch, it looks good to me! r=mkmelin ::: mail/components/preferences/fonts.js @@ +172,3 @@ > > + var sendCharsetStr = Services.prefs.getComplexValue( > + "mailnews.send_default_charset", Ci.nsIPrefLocalizedString).data; nit: id prefer the second line to be indented by two space past var on the upper line. @@ +175,3 @@ > > + var viewCharsetStr = Services.prefs.getComplexValue( > + "mailnews.view_default_charset", Ci.nsIPrefLocalizedString).data; same thing here

Comment on attachment 8432250 [details] [diff] [review] Update character encoding-related pref UI not to use the nsCharsetMenu RDF data source Review of attachment 8432250 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/preferences/fonts.js @@ +27,5 @@ > + { > + var viewCharsetsValues = ["UTF-8", "windows-1256", "windows-1257", "ISO-8859-2", > + "windows-1250", "gbk", "Big5", "windows-1251", > + "ISO-8859-7", "windows-1255", "ISO-2022-JP", "EUC-KR", > + "windows-874", "windows-1258", "windows-1252", "ISO-8859-1"]; I'm guessing the order here is based on the order of the labels in the en-US locale, which could look very strange to people in other locales. The natural solution is to sort them [except for keeping UTF-8 at the front] based on the sort of their charset labels. ::: mailnews/base/content/folderProps.js @@ +111,5 @@ > + // Log to the Error Console the charset value for the folder > + // if it is unknown to us. Value will be preserved by the menu-item. > + if (folderCharsetList.selectedIndex == -1) > + { > + Components.utils.reportError("Unknown folder encoding; folder=" + gMsgFolder.name + ", charset=" + gMsgFolder.charset); Nit: pretty sure this violates the maximum line length.

(In reply to Joshua Cranmer [:jcranmer] from comment #32) > > + var viewCharsetsValues = ["UTF-8", "windows-1256", "windows-1257", "ISO-8859-2", > > + "windows-1250", "gbk", "Big5", "windows-1251", > > + "ISO-8859-7", "windows-1255", "ISO-2022-JP", "EUC-KR", > > + "windows-874", "windows-1258", "windows-1252", "ISO-8859-1"]; windows-1252 and ISO-8859-1 don't make semse as two items in this menu. It only affects the charset declaration in the reply, but doesn't affect how stuff decodes or encodes for replies. Users shouldn't be exposed to the compat issue of what label to put in the headers for replies.

(In reply to Henri Sivonen (:hsivonen) from comment #33) > windows-1252 and ISO-8859-1 don't make semse as two items in this menu. It > only affects the charset declaration in the reply, but doesn't affect how > stuff decodes or encodes for replies. Users shouldn't be exposed to the > compat issue of what label to put in the headers for replies. Should ISO-8859-1 be kept and windows-1252 removed?

Also, unless there is a really good reason to have UTF-8 in the incoming menu, it should be in the outgoing menu only. (It'a not a legacy encoding.)

Re ISO-8859-1 in the view menu: given how much ISO-8859-1 is used in standards, it may be better recognized than windows-1252. Oh well... Re UTF-8 in incoming: why does it matter if it's a legacy encoding or not? That encoding option is used as a fallback if the mail (or news) message doesn't have a charset declared. That may be rare, but when it happens, UTF-8 may an ok choice.

(In reply to Magnus Melin from comment #36) > Re ISO-8859-1 in the view menu: given how much ISO-8859-1 is used in > standards, it may be better recognized than windows-1252. Oh well... Do you mean recognized by users or by receiving email programs? If the former, that's why Firefox these days doesn't show the name of the encoding in the UI. If the latter, I think it's wrong to bother the user with the problem. Instead, the Thunderbird developers should figure out the compatibility issue on the users' behalf. Note that if you are worrying about ISO-8859-1 being better recognized than windows-1252 in email headers, it would make sense to also be worried about TIS-620 vs. windows-874. Anyway, I think intentionally mislabeling windows-1252 as ISO-8859-1 or windows-874 as TIS-620 in outgoing headers for supposed compatibility with receiving applications belongs in the MIME header generation code and not here. (So to answer comment 34, I'd remove ISO-8859-1 here and keep windows-1252.) For example, TIS-620 is on track to going away as a mozilla-central-supported Gecko-canonical encoding name. The reason why ISO-8859-1 is staying in m-c for now even though it's no longer exposed to the Web is that it's harder to remove all references from the code base, but that doesn't mean it's a good idea to land new code that treats ISO-8859-1 as a valid Gecko-canonical name. > Re UTF-8 in incoming: why does it matter if it's a legacy encoding or not? In order to avoid facilitating the creation of a new generation of broken email sending software that uses the non-legacy encoding but still doesn't send the encoding label. > That encoding option is used as a fallback if the mail (or news) message > doesn't have a charset declared. That may be rare, but when it happens, > UTF-8 may an ok choice. If it's rare, why does it make sense as a pref (as opposed to a View menu item)?

(In reply to Henri Sivonen (:hsivonen) from comment #37) > (In reply to Magnus Melin from comment #36) > > Re ISO-8859-1 in the view menu: given how much ISO-8859-1 is used in > > standards, it may be better recognized than windows-1252. Oh well... > > Do you mean recognized by users or by receiving email programs? I meant recognized by users. > > Re UTF-8 in incoming: why does it matter if it's a legacy encoding or not? > > In order to avoid facilitating the creation of a new generation of broken > email sending software that uses the non-legacy encoding but still doesn't > send the encoding label. Our market share is small enough not to have any impact on that, unfortunately. > > That encoding option is used as a fallback if the mail (or news) message > > doesn't have a charset declared. That may be rare, but when it happens, > > UTF-8 may an ok choice. > > If it's rare, why does it make sense as a pref (as opposed to a View menu > item)? My take on it is that for e-mail it's virtually non-existent, and we should indeed show it only for news folders. Nobody just had the guts to do it yet. I think Joshua had data that it's a large enough problem to care about for news.

While a decision is taken about which encoding should be kept on the menulist, I have a few questions. Some code on the repository is using Services.strings instead of <stringbundle>. I have replaced it on my patch. I copy&paste here for some feedback: (the following code is also part of the code pasted for Joshua feedback, if you wanted a more general point of view). var charsetBundle = Services.strings.createBundle("chrome://messenger/locale/charsetTitles.properties"); aMenuStrings.forEach(function(item) { var strCharset = charsetBundle.GetStringFromName(item.toLowerCase() + ".title"); On the other side, that is my new code for sorting menu-items. Some feedback here, Joshua? Thank you. Furthermore, I assume the same should be used for the other menulists. createDefaultCharsetMenus: function() { var viewCharsetsValues = ["windows-1256", "windows-1257", "ISO-8859-2", "windows-1250", "gbk", "Big5", "windows-1251", "ISO-8859-7", "windows-1255", "ISO-2022-JP", "EUC-KR", "windows-874", "windows-1258", "ISO-8859-1"]; var sendCharsetsValues = ["UTF-8", "gbk", "gb18030", "ISO-8859-7", "ISO-2022-JP", "EUC-KR", "windows-1252", "ISO-8859-1"]; var viewMenuList = document.getElementById("viewDefaultCharsetList"); var sendMenuList = document.getElementById("sendDefaultCharsetList"); this.populateCharsetMenu(viewMenuList, this.sortCharsetLabels(viewCharsetsValues)); this.populateCharsetMenu(sendMenuList, this.sortCharsetLabels(sendCharsetsValues)); // Select appropiate menu item. var preference = document.getElementById( viewMenuList.getAttribute("preference")); viewMenuList.value = preference.value; preference = document.getElementById( sendMenuList.getAttribute("preference")); sendMenuList.value = preference.value; }, sortCharsetLabels: function(aMenuStrings) { var menuLabels = []; var charsetBundle = Services.strings.createBundle("chrome://messenger/locale/charsetTitles.properties"); aMenuStrings.forEach(function(item) { var strCharset = charsetBundle.GetStringFromName(item.toLowerCase() + ".title"); menuLabels.push({label: strCharset, value: item}); }); menuLabels.sort(function(a, b) { if (a.value == "UTF-8" || a.label < b.label) return -1; if (a.label > b.label) return 1; return 0; }); return menuLabels; }, populateCharsetMenu: function(aMenuList, aMenuStrings) { aMenuStrings.forEach(function(item) { aMenuList.appendItem(item.label, item.value); }); },

This patch is a draft for the implementation of the same previous patch but done as a XUL/XBL widget. Please, note that I was developing it at the same time I worked on patches which has been reviewed here. Then I qfolded both of them. That is the explanation about the fact that original code in some files is not what it is on c-c repo. Some explanation: * A new XUL widget is defined and implemented in file charsetList.xml * It can be used by adding a <menulist> into XUL and additional CSS for the XBL binding * <menulist> type attribute must be "charset" * "subset" attribute makes it easy to tell widget what are the character set we need to be shown a the popup. * "subset" could be either "viewing" or "sending" * Most the code for the implementation is the same as previously reviewed. Furthermore, menu-items are sorted alphabetically, keeping UTF-8 at the top-most item. Latest changes about what charsets should be included have not been integrated yet. This is not a final patch, but just a draft. Feedback, please. Thank you.

Same as attachment 8443042 [details] [diff] [review] but with sorting algorithm on fonts.xul. It includes UTF-8 and windows-1252 again.

Comment on attachment 8447749 [details] [diff] [review] patch Review of attachment 8447749 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/preferences/fonts.js @@ +32,5 @@ > + "windows-1258", "windows-1252", "ISO-8859-1"]; > + > + var sendCharsetsValues = ["UTF-8", "gbk", "gb18030", "ISO-8859-7", > + "ISO-2022-JP", "EUC-KR", "windows-1252", > + "ISO-8859-1"]; I'd off-hand prefer these lists to be sorted by charset value, leaving "UTF-8" at the front, so, e.g., sendCharsetsValues = ["UTF-8", "EUC-KR", "gbk", "gb18030", "ISO-2022-JP", "ISO-8859-1", "ISO-8859-7", "windows-1252"]; It makes it clearer to see what's supported and what's not supported, rather than having an ordering that's merely based off the predominant language names in English. Actually, come to think of it, why are ISO-8859-1 and Windows-1252 both in this list? As far as our charset backend is concerned, they're the same charset. @@ +66,5 @@ > + menuLabels.push({label: strCharset, value: item}); > + }); > + > + menuLabels.sort(function(a, b) { > + if (a.value == "UTF-8" || a.label < b.label) return -1; This sort function isn't quite right, since it's not checking for b.value == "UTF-8" as well and returning 1 in that scenario.

(In reply to Henri Sivonen (:hsivonen) from comment #37) > > Note that if you are worrying about ISO-8859-1 being better recognized than > windows-1252 in email headers, it would make sense to also be worried about > TIS-620 vs. windows-874. Anyway, I think intentionally mislabeling > windows-1252 as ISO-8859-1 or windows-874 as TIS-620 in outgoing headers for > supposed compatibility with receiving applications belongs in the MIME > header generation code and not here. (So to answer comment 34, I'd remove > ISO-8859-1 here and keep windows-1252.) mailnews.view_default_charset preference default vañue is ISO-8859-1.

Fixed sort function. It has been also applied into folderProps.js and am-server.js. Both Western charsets has been kept, as there hasn't been taken any decision about it.

Comment on attachment 8451924 [details] [diff] [review] Patch v1.1 Review of attachment 8451924 [details] [diff] [review]: ----------------------------------------------------------------- I'd still like to see the ISO-8859-1/Windows-1252 duplication cleaned up, but at this point, that's a follow-on bug. I haven't tested this patch, but I don't realistically have time in the near future to give this patch the testing it defers, so I'll just mark f+ and let mkmelin fulfill that role in the review process.

Comment on attachment 8451924 [details] [diff] [review] Patch v1.1 Review of attachment 8451924 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for fixing this! I'm happy with this, just a couple of style nits. r=mkmelin ::: mailnews/base/content/folderProps.js @@ +112,5 @@ > + // if it is unknown to us. Value will be preserved by the menu-item. > + if (folderCharsetList.selectedIndex == -1) > + { > + Components.utils.reportError("Unknown folder encoding; folder=" > + + gMsgFolder.name + ", charset=" + gMsgFolder.charset); style nit: please put the + on the end of the previous line @@ +333,5 @@ > + }); > + > + menuLabels.sort(function(a, b) { > + if (a.value == "UTF-8" || a.label < b.label) return -1; > + if (b == "UTF-8" || a.label > b.label) return 1; and put the returns on new lines ::: mailnews/base/prefs/content/am-server.js @@ +410,5 @@ > + }); > + > + menuLabels.sort(function(a, b) { > + if (a.value == "UTF-8" || a.label < b.label) return -1; > + if (b == "UTF-8" || a.label > b.label) return 1; same here

Comment on attachment 8443063 [details] [diff] [review] charsetpicker widget please remember to obsolete old patch versions

To the person who make the check in into the repository: Review and feedback on comments 47-48.

return statements on fonts.js haven't been moved to a new line.

Please use the leave-open keyword, rather than don't close, to ensure a bug stays open.

Please, see comment 41 for explanation. This one, however now implements the charset strings sorting and removes those which had been removed on previous patch. I hoped there was a better way to call substPrefTokens function but it seems XBL 1.0 doesn't allow to import scripts from outside :( While using a XBL widget, all that repeated code will be unneeded. That is the reason I have just re-implemented it again this way.

Comment on attachment 8489665 [details] [diff] [review] XBL widget implementation Review of attachment 8489665 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/jar.mn @@ +65,5 @@ > content/messenger/phishingDetector.js (content/phishingDetector.js) > content/messenger/mail-offline.js (content/mail-offline.js) > content/messenger/aboutDialog.css (content/aboutDialog.css) > content/messenger/messenger.css (content/messenger.css) > + content/messenger/charsetList.css (content/charsetList.css) need this somewhere in suite/ too (mailnews is shared with seamonkey) ::: mailnews/base/content/charsetList.xml @@ +56,5 @@ > + > + // Selecting appropiate menu item corresponding to preference stored > + // value. > + if (this.hasAttribute("prefstring")) { > + window.setTimeout(function(){ // Wait until attribute has been added. spacing between () and { however, arbitrary timeouts are not a good idea @@ +62,5 @@ > + aMenuList.value = Services.prefs.getCharPref(preference); > + }, 50, []); > + } > + else if (this.hasAttribute("preference")) { > + var Ci = Components.interfaces; const not var and in this (new) file there's a mix if var and let. please use let consistently @@ +74,5 @@ > + <parameter name="aStr"/> > + <parameter name="aElement"/> > + <body><![CDATA[ > + // Note: This function has been stolen from the one on mailnews/base/prefs/content/am-prefs.js > + let tokenpat = /%(\w+)%/; please inline this @@ +75,5 @@ > + <parameter name="aElement"/> > + <body><![CDATA[ > + // Note: This function has been stolen from the one on mailnews/base/prefs/content/am-prefs.js > + let tokenpat = /%(\w+)%/; > + let token; no need to declare it up here @@ +83,5 @@ > + /* here's a little loop that goes through > + each part of the string separated by a dot, and > + if any parts are of the form %string%, it will replace > + them with the value of the attribute of that name from > + the xul object */ Please use // style code comments, and make it proper sentences (capitalization, period) @@ +86,5 @@ > + them with the value of the attribute of that name from > + the xul object */ > + for (let i = 0; i < prefPartsArray.length; i++) { > + token = prefPartsArray[i].match(tokenpat); > + if (token) { /* we've got a %% match */ // style comments please. here and elsewhere the style you used not nice for commenting out larger chunks of code later

It includes some of the corrections from previous review. While debugging this patch I found that the preference was updated even my patch didn't work at all (an exception was throw). Finally, I removed code relative to the substitution of "prefstring" for servers of type=nntp, and everything was working as expected. Account Settings or Manager should be taking care about this and my code was trying to do it again. So, my previous code was wrong. So, this new version of the widget will only take care of "preference" when that was the attribute that appears. Code has been moved into mailnews/ and checked so what was working on TB is still working. For SeaMonkey, I have only verified it builds.

Comment on attachment 8503305 [details] [diff] [review] Corrected and simplified patch (XBL) Review of attachment 8503305 [details] [diff] [review]: ----------------------------------------------------------------- Thx Javier, looks good to me! r=mkmelin with the indention fixed ::: mailnews/base/content/charsetList.xml @@ +39,5 @@ > + let strCharset = charsetBundle.GetStringFromName( > + item.toLowerCase() + ".title"); > + > + menuLabels.push({label: strCharset, value: item}); > + }); the two lines above are wrongly indented, and everything after it should be indented two more spaces

let menuLabels = []; charsetValues.forEach(function(item) { let strCharset = charsetBundle.GetStringFromName( item.toLowerCase() + ".title"); This line are the arguments for GetStringFromName(). Keeping them at the same line implied surpassing line length to more than 80. charsetValues.forEach(...) line was offset by 1 char. Corrected with new patch. Now everything seems ok to me. Good to check-in, Magnus?

I think so!