1221415 - Locale message processing incompatibilities

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Kris Maglione [:kmag]]

The Chrome l10n documentation is impressively bad. I had to track down and read the code to figure out how it's meant to work, and our locale message processing has some incompatibilities:

* Messages that don't exist in the current locale should fall back to the default locale.

* This is not documented, but any number of consecutive dollar signs are replaced by the same number of dollar signs minus one. So, $$ -> $, $$$ -> $$, ...

* When the locale file is read, tokens matching /\$([a-z0-9_@]+)\$/i are replaced with the matching value from the string's "replacements" object. Again, this is not documented, but these substitutions happen prior to processing any /\$\d/ tokens in the message. We currently process /$\d/ tokens multiple times.

* When a locale string is used, tokens matching /\$\d+/ are replaced with the replacements passed to `getMessage`. Strangely, though, while the processor consumes all digits following the $ character, the `getMessage` API refuses to process any calls with more than 9 substitutions.

Taking this, since I wound up writing some code for it while I was investigating.

Comment 1

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1221415: [webext] Improve error checking and Chrome-compatibility of i18n API. r?billm

Bug 1221415: [webext] Improve error checking and Chrome-compatibility of i18n API. r?billm

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8687741 [details]
MozReview Request: Bug 1221415: [webext] Improve error checking and Chrome-compatibility of i18n API. r?billm

https://reviewboard.mozilla.org/r/25249/#review22891

::: toolkit/components/extensions/Extension.jsm:347
(Diff revision 1)
>    // Map(locale-name -> message-map)
>    // Contains a key for each loaded locale, each of which is a
> -  // JSON-compatible object with a property for each message
> +  // Map of message keys to their localized strings.

Might be better to write this as Map[locale-name -> Map[message-key -> localized-strings]].

::: toolkit/components/extensions/Extension.jsm:585
(Diff revision 1)
> +      if (!instanceOf(msg, "Object") || typeof msg.message != "string") {

Again, parens around the typeof.

::: toolkit/components/extensions/Extension.jsm:590
(Diff revision 1)
> +      let { replacements } = msg;

Do you mean placeholders here?
https://developer.chrome.com/extensions/i18n-messages

::: toolkit/components/extensions/Extension.jsm:596
(Diff revision 1)
> +        let replacement = replacements[name];

Based on the documentation I linked, the placeholder names seem to be case insensitive. So maybe we should convert everything to uppercase?

::: toolkit/components/extensions/Extension.jsm:617
(Diff revision 1)
> -    let messages = {};
> +    let messages = new Map;

Please use new Map() for consistency.

::: toolkit/components/extensions/Extension.jsm:853
(Diff revision 1)
> +    } else if (typeof script == "object") {

typeof(script) for consistency

::: toolkit/components/extensions/Extension.jsm:1007
(Diff revision 1)
> -    return ExtensionData.prototype.initLocale.call(this, locale);
> +    promises.push(ExtensionData.prototype.initLocale.call(this, locale));

Why are you able to call readLocaleFile but not initLocale directly?

Comment 3

[Kris Maglione [:kmag]]

(In reply to Bill McCloskey (:billm) from comment #2)
> ::: toolkit/components/extensions/Extension.jsm:590
> (Diff revision 1)
> > +      let { replacements } = msg;
> 
> Do you mean placeholders here?
> https://developer.chrome.com/extensions/i18n-messages
> 
> ::: toolkit/components/extensions/Extension.jsm:596
> (Diff revision 1)
> > +        let replacement = replacements[name];
> 
> Based on the documentation I linked, the placeholder names seem to be case
> insensitive. So maybe we should convert everything to uppercase?

Yeah, I missed that.

> (Diff revision 1)
> > -    return ExtensionData.prototype.initLocale.call(this, locale);
> > +    promises.push(ExtensionData.prototype.initLocale.call(this, locale));
> 
> Why are you able to call readLocaleFile but not initLocale directly?

Because initLocale is overridden by Extension, to default to the browser
locale rather than the extension default locale, and readLocaleFile isn't.

Although, now that I think about it again, the initLocale in ExtensionData
should probably be the one to load the default locale if it's called with
another locale.

Comment 4

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/4916558f00d5ccd519f1de94039fb417f0cf92be
Bug 1221415: [webext] Improve error checking and Chrome-compatibility of i18n API. r=billm

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4916558f00d5

Comment 6

[Tomislav Jovanovic :zombie]

Not much point in documenting incompatibilities fixed 4+ years ago.