1372899 - Provide a facility to encode/decode UTF-7 in JS so tests disabled in bug 1363281 can be reinstated

Status: RESOLVED FIXED

(MailNews Core :: Internationalization, enhancement)

Description

[Jorg K (CEST = GMT+2)]

+++ This bug was initially created as a clone of Bug #1363281 +++

Some tests related to UTF-7 encoding were remove here:
https://hg.mozilla.org/comm-central/rev/f2532fa2989b44536653261e53c179edf27c5400
https://hg.mozilla.org/comm-central/rev/322986aa2dbabda0ba031d76ba2dc959c7cc9f7a
since UTF-7 can no longer be decoded via the nsIScriptableUnicodeConverter.

We should provide a facility to do this decoding in JS so we can enable the tests again.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1372899-scriptable-utf-7.patch (v1).

With this patch, this test works again:
mozilla/mach xpcshell-test mailnews/imap/test/unit/test_mailboxes.js

Let's send this for a try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=585d225c00f72bcb9d1a9adf70a76aa2a0587df7

Joshua, why the this.fullName.replace(/([\\"])/g, '\\$1') ?
It's from here:
https://hg.mozilla.org/comm-central/rev/a8d4959143e5#l5.276

I'd like to add a comment.

Comment 2

[Joshua Cranmer [:jcranmer]]

(In reply to Jorg K (GMT+2) from comment #1)
> Joshua, why the this.fullName.replace(/([\\"])/g, '\\$1') ?
> It's from here:
> https://hg.mozilla.org/comm-central/rev/a8d4959143e5#l5.276

That's a poor man's string escape. The regex is saying "replace every instance of \ and " with itself prepended by \".

Comment 3

[Jorg K (CEST = GMT+2)]

Yes, that's what the regex says, but why is it necessary?
How about a comment:
// Escape \ and " with another \ since ...

Comment 4

[Jorg K (CEST = GMT+2)]

BTW, https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=585d225c00f72bcb9d1a9adf70a76aa2a0587df7 is green.

Comment 5

[:aceman]

Comment on attachment 8878215 [details] [diff] [review]
1372899-scriptable-utf-7.patch (v1).

Review of attachment 8878215 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/imap/test/unit/test_mailboxes.js
@@ +5,5 @@
>  load("../../../resources/logHelper.js");
>  load("../../../resources/asyncTestUtils.js");
>  load("../../../resources/messageGenerator.js");
>  
> +const folderName1 = "I18N box\u00E1";

Please add a comment that the names are in modified UTF-7 encoding.

::: mailnews/intl/nsCharsetConverterManager.cpp
@@ +16,5 @@
>  
>  #include "nsComponentManagerUtils.h"
>  #include "nsISupportsPrimitives.h"
>  #include "nsServiceManagerUtils.h"
> +#include "../base/util/nsMsgI18N.h"

Is the path needed? It's uncommon with other includes.

::: mailnews/intl/nsICharsetConverterManager.idl
@@ +61,5 @@
>      nsIAtom getCharsetLangGroup(in string aCharset);
>      nsIAtom getCharsetLangGroupRaw(in string aCharset);
> +
> +    /**
> +     * MUTF-7 Support.

Please describe what the M means.

::: mailnews/test/fakeserver/imapd.js
@@ +301,5 @@
>             this.name;
>    },
>    get displayName() {
> +    let manager = Cc['@mozilla.org/charset-converter-manager;1']
> +                    .getService(Ci.nsICharsetConverterManager);

How often do we call this displayName in the tests? May this be expensive to get the Manager every time here?

Comment 6

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #5)
> How often do we call this displayName in the tests? May this be expensive to
> get the Manager every time here?
Before it got the scriptable Unicode encoder there:
https://hg.mozilla.org/comm-central/rev/f2532fa2989b44536653261e53c179edf27c5400
but since it doesn't handle UTF-7 any more I had to kill that code and now it's coming back.

Comment 7

[:aceman]

OK.

Comment 8

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #5)
> > +const folderName1 = "I18N box\u00E1";
> Please add a comment that the names are in modified UTF-7 encoding.
Actually, no, this is simply "I18N boxá". Without an MUTF-7 encoder I can't represent this name. That's why I dumbed the names down here:
https://hg.mozilla.org/comm-central/rev/322986aa2dbabda0ba031d76ba2dc959c7cc9f7a#l1.12

> > +#include "../base/util/nsMsgI18N.h"
> Is the path needed? It's uncommon with other includes.
Because this is not a published interface and it's in a different directory. Uncommon, but used:
https://dxr.mozilla.org/comm-central/rev/72afbd97238900cf27f9bbe59d7b238c5bd7b2e0/mailnews/base/util/nsMsgI18N.cpp#29
https://dxr.mozilla.org/comm-central/rev/72afbd97238900cf27f9bbe59d7b238c5bd7b2e0/mailnews/imap/src/nsImapService.cpp#72

Comment 9

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/df37b1887f4dc4170cc657bf4317f5870209485c

Landed with a few tweaked comments.