Closed
Bug 999881
Opened 11 years ago
Closed 11 years ago
Use CharsetMenu instead of the old charsetOverlay in Thunderbird
Categories
(Thunderbird :: Mail Window Front End, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 31.0
People
(Reporter: jcranmer, Assigned: jcranmer)
References
Details
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
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...
Attachment #8410719 -
Flags: review?(mconley)
Assignee | ||
Comment 1•11 years ago
|
||
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.
Assignee | ||
Comment 4•11 years ago
|
||
This includes the charsetMenu.dtd fix that Henri noticed, but should otherwise be identical to the last patch.
Attachment #8410719 -
Attachment is obsolete: true
Attachment #8410856 -
Attachment is obsolete: true
Attachment #8410719 -
Flags: review?(mconley)
Attachment #8413290 -
Flags: review?(mconley)
Assignee | ||
Comment 5•11 years ago
|
||
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.
Attachment #8413291 -
Flags: review?(mconley)
Assignee | ||
Comment 6•11 years ago
|
||
(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.
tracking-thunderbird31:
--- → ?
Comment 7•11 years ago
|
||
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.
Attachment #8413290 -
Flags: review?(mconley) → review+
Comment 8•11 years ago
|
||
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.
Attachment #8413291 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 9•11 years ago
|
||
(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.]
Assignee | ||
Comment 10•11 years ago
|
||
https://hg.mozilla.org/comm-central/rev/416056a645fe
https://hg.mozilla.org/comm-central/rev/7f010544c325
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 31.0
Updated•11 years ago
|
Comment 12•10 years ago
|
||
(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.
tracking-thunderbird31:
? → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•