I tested this only on Linux, so it might be slightly problematic. By my notes, this should remove all uses of the deprecated global/charsetOverlay.xul (yay, see <http://dxr.mozilla.org/comm-central/search?q=charsetOverlay.xul&case=true>). Next task is stopping our use of the RDF datasource...

One thing that's very hidden in the bugzilla UI is I hg copied suite/common/charsetOverlay.xul to mail/base/content/charsetOverlay.xul

Please make sure you don't depend on m-c toolkit DTDs to localize the c-c charsetOverlay.xul files.

(In reply to Henri Sivonen (:hsivonen) from comment #2) > Please make sure you don't depend on m-c toolkit DTDs to localize the c-c > charsetOverlay.xul files. See attached for a part-way fix.

This includes the charsetMenu.dtd fix that Henri noticed, but should otherwise be identical to the last patch.

Okay, this is a slightly more complicated patch: 1. The charset overlay normalizes ISO-8859-1 to windows-1252 instead. I think we always output windows-1252 anyways. Given that our default switched to UTF-8, the practical impact of this change should ultimately be minimal. 2. I tried to make deathly sure that there was no way we'd attempt to encode any charset other than the ones in the list. Most notably x-mac-croatian (which shouldn't exist). 3. I added a test to make sure we could properly handle replying to or editing forbidden charset messages. This includes making sure that the charset won't get mojibake'd (via UTF-7). I'm bad at mozmill tests, so that is 90% cribbing.

(In reply to Joshua Cranmer [:jcranmer] from comment #5) > 2. I tried to make deathly sure that there was no way we'd attempt to encode > any charset other than the ones in the list. Most notably x-mac-croatian > (which shouldn't exist). This part I think needs to go into TB 31.

Comment on attachment 8413290 [details] [diff] [review] Rough port of bug 943732 Review of attachment 8413290 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. Tested on OS X. Thanks jcranmer.

Comment on attachment 8413291 [details] [diff] [review] Use the new charset menu in compose Review of attachment 8413291 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Just a few nits - with those fixed (or compelling arguments against), r=me. ::: mail/components/compose/content/MsgComposeCommands.js @@ +1774,5 @@ > dump("### textEditor.wrapWidth exception text: " + e + " - failed\n"); > } > } > CompFields2Recipients(gMsgCompose.compFields); > + Nit - remove the extra newline. ::: mail/test/mozmill/composition/test-charset-edit.js @@ +30,5 @@ > +function setupModule(module) { > + for (let req of MODULE_REQUIRES) { > + collector.getModule(req).installInto(module); > + } > + if (!MailServices.accounts Looks like we're using rootFolder a lot. Let's just alias that before all of these, and clean this up a little. @@ +57,5 @@ > + if (!outboxFolder) > + throw new Error("outboxFolder not found"); > + > + // Ensure reply charset isn't UTF-8, otherwise there's no need to upgrade, > + // which is what this test tests. Nit - extra space at start @@ +61,5 @@ > + // which is what this test tests. > + let str = Components.classes["@mozilla.org/pref-localizedstring;1"] > + .createInstance(Components.interfaces.nsIPrefLocalizedString); > + str.data = "windows-1252"; > + Services.prefs.setComplexValue("mailnews.send_default_charset", We might want to stash any pre-existing values at this pref, and restore that instead of setting/overwriting and then deleting. @@ +70,5 @@ > + * Helper to get the full message content. > + * > + * @param aMsgHdr: nsIMsgDBHdr object whose text body will be read > + * @param aGetText: if true, return header objects. if false, return body data. > + * @return Dict(partnum -> message headers) Thanks for the documentation - though Dict isn't really right. A Map, no? @@ +130,5 @@ > + let draftMsg = select_click_row(1); > + assert_equals(getMsgHeaders(draftMsg).get("").charset, "windows-1252"); > + press_delete(mc); // Delete message > + > + // Edit the original message. Charset should be windows-1252 now. Nit - trailing ws. @@ +171,5 @@ > + if (!text.contains(nonASCII)) > + throw new Error("Expected to find " + nonASCII + " in " + text); > + press_delete(mc); // Delete message > + > + // Edit the original message. Charset should be UTF-8 now. Nit - trailing ws.

(In reply to Mike Conley (:mconley) from comment #8) > Looks like we're using rootFolder a lot. Let's just alias that before all of > these, and clean this up a little. [ FWIW, the boilerplate here was mostly copy-pasted from test-charset-upgrade.js] > We might want to stash any pre-existing values at this pref, and restore > that instead of setting/overwriting and then deleting. I copied this from test-charset-upgrade.js. We really shouldn't be modifying the prefs at all for the tests, but test-charset-upgrade needs to test non-UTF-8 to UTF-8 upgrade while test-charset-edit wanted to ensure that upgrading wouldn't play a role in its test. [FWIW, when I pushed the patch queue to try, it failed but not locally. It turns out that the way test-charset-upgrade.js cleans the outbox doesn't appear to actually be safe; when I copied from that to test-charset-edit, if test-charset-edit ran first, test-charset-upgrade failed. Since I don't need the oubox in test-charset-edit, I deleted that code and got it to work when run in that order.]

https://hg.mozilla.org/comm-central/rev/416056a645fe https://hg.mozilla.org/comm-central/rev/7f010544c325

See bug 1002468 comment 4.

(In reply to Joshua Cranmer [:jcranmer] from comment #6) > (In reply to Joshua Cranmer [:jcranmer] from comment #5) > > 2. I tried to make deathly sure that there was no way we'd attempt to encode > > any charset other than the ones in the list. Most notably x-mac-croatian > > (which shouldn't exist). > > This part I think needs to go into TB 31. This apparently was removed in TB 32 - I just confirmed via code search.