Locale message processing incompatibilities

RESOLVED FIXED in Firefox 45

Status

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: kmag, Assigned: kmag)

Tracking

(Blocks 1 bug, {dev-doc-needed})

unspecified
mozilla45
Dependency tree / graph
Bug Flags:
blocking-webextensions ?

Firefox Tracking Flags

(firefox45 fixed)

Details

(Whiteboard: [i18n])

Attachments

(1 attachment)

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.
Flags: blocking-webextensions?
Bug 1221415: [webext] Improve error checking and Chrome-compatibility of i18n API. r?billm
Attachment #8687741 - Flags: review?(wmccloskey)
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?
Attachment #8687741 - Flags: review?(wmccloskey) → review+
(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.
https://hg.mozilla.org/integration/fx-team/rev/4916558f00d5ccd519f1de94039fb417f0cf92be
Bug 1221415: [webext] Improve error checking and Chrome-compatibility of i18n API. r=billm
https://hg.mozilla.org/mozilla-central/rev/4916558f00d5
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Iteration: --- → 45.2 - Nov 30
Keywords: dev-doc-needed
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.