Closed Bug 1357902 Opened 6 years ago Closed 6 years ago

WebExtensions I18n API should use the correct LocaleService data

Categories

(WebExtensions :: General, enhancement, P2)

Product:
WebExtensions
Component:
General
Type:
enhancement

Tracking

(firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: zbraniecki, Assigned: kmag)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [l10n], triaged)

Attachments

(1 file)

Bug 1357902 - Use improved locale service APIs for localization.
59 bytes, text/x-review-board-request
zbraniecki
: review+
Details 
At the moment WebExtensions i18n is using several APIs slightly incorrectly:

1) https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/i18n/LanguageCode

Not sure what is it currently using, since the only code in our codebase is minified, but I suspect it uses RequestLocale, not AppLocale.

It should be using AppLocale so that extensions stay in sync with UI.

2) https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/i18n/detectLanguage

There's code in our I18n module that does language detection for font guessing and spacing. I'm wondering if we should unify this to reduce code duplication and maximize the chance that a language detected by Gecko will be the same as detected by Gecko's WebExtension system. NI'ing :jfkthame and :m_kato.

3) https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/i18n/getMessage

The locale selection for messages should use LocaleService.negotiateLanguages Lookup strategy instead of a custom code I believe:

http://searchfox.org/mozilla-central/source/toolkit/modules/Locale.jsm#28
http://searchfox.org/mozilla-central/source/intl/locale/mozILocaleService.idl#104

5) https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/i18n/getUILanguage

Same as in the first one, we should use LocaleService::GetAppLocaleAsBCP47 (or AsLangtag) instead of RequestedLocale.
NI on :m_kato and :jfkthame on the language detection unification.

The reason why I'd like to update this code soon is that the more extensions we get that rely on the implementation details the harder it will be to change.
Flags: needinfo?(m_kato)
Flags: needinfo?(jfkthame)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #0)

> 2)
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/i18n/
> detectLanguage
> 
> There's code in our I18n module that does language detection for font
> guessing and spacing.

I'm not sure what code you're referring to here?
Flags: needinfo?(jfkthame)
Oh, I thought that we do use ICU charset detector[0] in our CJK font guessing.

Should we use it here?

[0] http://icu-project.org/apiref/icu4c/ucsdet_8h.html
Flags: needinfo?(jfkthame)
Or the custom charset detector that we have - http://searchfox.org/mozilla-central/source/intl/chardet
IIUC, the WebExtensions detectLanguage API is about *language* detection, given a chunk of (Unicode) text, which is quite different from charset detection (for an arbitrary chunk of bytes). There's not a lot of overlap here.

Charset detection in either ICU or intl/chardet/ is about figuring out what legacy encoding some random string of bytes is probably meant to represent, so that it can be mapped to Unicode appropriately; in the CJK case, the guessed charset may also provide a hint as to language (because Chinese and Japanese typically used different legacy encodings), but aside from that there's not much connection to language.
Flags: needinfo?(jfkthame)
Oh, right, I assumed character detection and language detection will overlap more severely.

Taking off NI's.

So, it's down to:

1) Use AppLocale, not RequestedLocale
2) Use LanguageNegotiation
Flags: needinfo?(m_kato)
Also, we should use same behavior of Chrome's extension API (https://developer.chrome.com/extensions/i18n).  This is from chrome extension and addon developer can use it without new knowledge.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #0)
> 1) https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/i18n/LanguageCode
>
> Not sure what is it currently using, since the only code in our codebase is
> minified, but I suspect it uses RequestLocale, not AppLocale.

This is just a type name. It doesn't correspond to any actual code.

> It should be using AppLocale so that extensions stay in sync with UI.
>
> 2) https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/i18n/detectLanguage
>
> There's code in our I18n module that does language detection for font
> guessing and spacing. I'm wondering if we should unify this to reduce code
> duplication and maximize the chance that a language detected by Gecko will
> be the same as detected by Gecko's WebExtension system. NI'ing :jfkthame and
> :m_kato.

Even if our detection code did the same as this API (which comment 5 suggests
it doesn't), we'd probably still want to continue using the cld2 API, since
that's also what Chrome uses for its implementation of this API, and it
guarantees us good compatibility with them.

> 3) https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/i18n/getMessage
>
> The locale selection for messages should use
> LocaleService.negotiateLanguages Lookup strategy instead of a custom code I
> believe:

I know that Locales.jsm is going away, but is there any reason not to update
findClosestLocale to use negotiateLocales in the mean time?

In any case, I agree that this should be updated.

> 5) https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/i18n/getUILanguage
>
> Same as in the first one, we should use LocaleService::GetAppLocaleAsBCP47
> (or AsLangtag) instead of RequestedLocale.

I'm just curious: how does this differ from getRequestedLocale()?

Also, I'm wondering if we should try to do some sort of more complex
negotiation based on the list of requested app locales and the list of locales
bundled with the extension. This topic has come up before, when talking about
distribution add-ons, and the best fallback to choose when the app build
locale differs from the currently active locale.
> I'm just curious: how does this differ from getRequestedLocale()?

If Firefox has resources in ['de', 'fr'] and the user requests ['de'], he ends up with:

availableLocales: ['de', 'fr']
requestedLocales: ['de']
appLocales: ['de']

If he requests ['it', 'de'], he ends up with:

availableLocales: ['de', 'fr']
requestedLocales: ['it', 'de']
appLocales: ['de']

So, if webextensions follow requested, they may end up in a locale that the UI is not, if they follow appLocales, they'll sync with the main product UI.

> Also, I'm wondering if we should try to do some sort of more complex
negotiation based on the list of requested app locales and the list of locales
bundled with the extension. This topic has come up before, when talking about
distribution add-ons, and the best fallback to choose when the app build
locale differs from the currently active locale.

Oh, that's great that you bring it up.
I just filed bug 1358628 which is about multi-negotiation and perfectly fits into the scope of the problem.

In the simplest approach, if you stick to appLocales, you're guaranteed that extension will not use a locale that UI doesn't have.
But if we'd ever want to make it a two-way binding, where Firefox UI doesn't pick a locale that extension doesn't have, we'll need what we'll come up with in bug 1358628 :)
We're also discussing a future "contexts" for locale negotiation and prototyped all APIs in LocaleService to handle a context ID argument in the future.

A simple use case is: You want Firefox in "de", but DevTools in "en-US".

In that case we would have two contexts 'main' and 'devtools' which would follow separate language negotiations and we'd keep updating your langpack for "main" Firefox, but keep your 'devtools' just in English.

I can imagine this idea extended to webextensions, but not sure how exactly that should work.
If you have a particular use case for this kind of feature, let's talk! :)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #9)
> In the simplest approach, if you stick to appLocales, you're guaranteed that
> extension will not use a locale that UI doesn't have.
> But if we'd ever want to make it a two-way binding, where Firefox UI doesn't
> pick a locale that extension doesn't have, we'll need what we'll come up
> with in bug 1358628 :)

OK. So, for a start, I think that, since this API requires that we only return
one locale, we should try to return the app locale that closest matches the
set of locales the extension has available. Then maybe we should consider
adding additional APIs that better match what's available in the locale
service.

So it seems like that would be something along the lines of:

  negotiateLanguages(extensionLocales, appLocalesAsBCP47)

Am I on the right track?
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #10)
> I can imagine this idea extended to webextensions, but not sure how exactly
> that should work.
> If you have a particular use case for this kind of feature, let's talk! :)

Yeah, that seems like it would apply very nicely to WebExtensions. I think I'd have to know more about how it would work for devtools before I can say more about how I think it should work, though.
(In reply to Kris Maglione [:kmag] from comment #11)
> (In reply to Zibi Braniecki [:gandalf][:zibi] from comment #9)
> > In the simplest approach, if you stick to appLocales, you're guaranteed that
> > extension will not use a locale that UI doesn't have.
> > But if we'd ever want to make it a two-way binding, where Firefox UI doesn't
> > pick a locale that extension doesn't have, we'll need what we'll come up
> > with in bug 1358628 :)
> 
> OK. So, for a start, I think that, since this API requires that we only
> return
> one locale, we should try to return the app locale that closest matches the
> set of locales the extension has available. Then maybe we should consider
> adding additional APIs that better match what's available in the locale
> service.
> 
> So it seems like that would be something along the lines of:
> 
>   negotiateLanguages(extensionLocales, appLocalesAsBCP47)

Reverse.

appLocalesAsBCP47 is your "requested" (because those are locales user requested filtered down to ones that we do have Firefox UI resources for), and extensionLocales is your "available".

But generally, yes :)

And since you only want to return one, you can just use Lookup strategy which gives you the best of the available based on requested.
(although I hope we'll move to lists, single locale is a deprecated concept!)

Then you can listen to "intl:app-locales-changed" event which will get fired when appLocales change, in case you'd like to renegotiate languages for yourself. (we're moving to the world where we will want to let user install langpacks and switch locales on fly, but it's a long shot, so  it's ok not to have it yet, but worth noting when you're doing refactors).

> Yeah, that seems like it would apply very nicely to WebExtensions. I think I'd have to know more about how it would work for devtools before I can say more about how I think it should work, though.

Cool, yeah. I expect to start playing with this more seriously soon and I'll CC you once we start prototyping.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #13)
> Reverse.
>
> appLocalesAsBCP47 is your "requested" (because those are locales user
> requested filtered down to ones that we do have Firefox UI resources for),
> and extensionLocales is your "available".

Well, there are two cases here:

1) We need to determine extension locale based on the requested locales of the
   browser.

2) We need to determine what to tell the extension is the UI locale, based on
   the list of app locales.

The second is what I'm after here. I'd like to return the app locale that
most closely matches the locales the extension has available, so the extension
locales are being requested, and the app locales are what's available.

But maybe I'm overthinking this, and I should just return getAppLocaleAsBCP47().

> Then you can listen to "intl:app-locales-changed" event which will get fired
> when appLocales change, in case you'd like to renegotiate languages for
> yourself. (we're moving to the world where we will want to let user install
> langpacks and switch locales on fly, but it's a long shot, so  it's ok not
> to have it yet, but worth noting when you're doing refactors).

Hm. Alas, I think we'd have to expose a new event to the extension, and have
them opt into it... I don't think it would work well if we suddenly started
returning strings from a different locale when they weren't expecting it. That
would definitely be nice to have, though.

We could probably get away with it for new loads of CSS files or extension
pages, though.

> > Yeah, that seems like it would apply very nicely to WebExtensions. I think
> > I'd have to know more about how it would work for devtools before I can
> > say more about how I think it should work, though.
>
> Cool, yeah. I expect to start playing with this more seriously soon and I'll
> CC you once we start prototyping.

Awesome. Thanks.
> But maybe I'm overthinking this, and I should just return getAppLocaleAsBCP47().

Yup, I guess you should just return that for now :)

if the extension is asking for the UI locale - getAppLocaleAsBCP47 is precisely that.
Priority: -- → P2
Whiteboard: [l10n], triaged
Blocks: 1362244
Comment on attachment 8864709 [details]
Bug 1357902 - Use improved locale service APIs for localization.

https://reviewboard.mozilla.org/r/136352/#review139674

::: toolkit/components/extensions/Extension.jsm:815
(Diff revision 1)
>          this.localeData.messages.set(locale, result);
>        });
>    }
>  
>    parseManifest() {
> -    return StartupCache.manifests.get([this.id, Locale.getLocale()],
> +    return StartupCache.manifests.get([this.id, Services.locale.getRequestedLocale()],

Why requestedLocales, instead of appLocales?

::: toolkit/components/extensions/Extension.jsm:949
(Diff revision 1)
>    async initLocale(locale = undefined) {
>      if (locale === undefined) {
>        let locales = await this.promiseLocales();
>  
> -      let localeList = Array.from(locales.keys(), locale => {
> -        return {name: locale, locales: [locale]};
> +      let matches = Services.locale.negotiateLanguages(
> +        Services.locale.getRequestedLocales(),

Why requested instead of appLocales?
Comment on attachment 8864709 [details]
Bug 1357902 - Use improved locale service APIs for localization.

https://reviewboard.mozilla.org/r/136352/#review139674

> Why requested instead of appLocales?

I guess I don't understand the difference well enough. We want to choose the closest extension locale to the requested locales list regardless of what app locales are available. Does appLocales give us that?
appLocales keep you bound to Firefox locales.

Imagine that the user requests a locale that Firefox has no resources for:

requested: ['de', 'it', 'en-US']
available: ['fr', 'it', 'pl', 'en-US']

appLocales: ['it', 'en-US']


Now, imagine that the extension does have the following locales:

availableExt: ['de', 'it', 'ru', 'en-US']

=====

If you use requested, you'll negotiate between ['de', 'it', 'en-US'] and ['de', 'it', 'ru', 'en-US'] and end up on ['de', 'it', 'en-US'], while Firefox will negotiate down to ['it', 'en-US'].

If you use appLocales instead, you'll negotiate between ['it', 'en-US'] and ['de', 'it', 'ru', 'en-US'] and end up on ['it', 'en-US'].

In result, using appLocales as your "requested" allows web extensions to stick to Firefox locale closer.
Are you planning to update your patch? I think it'd be good to have it for 55.
Flags: needinfo?(kmaglione+bmo)
Yes, I've been trying to find the time to get back to it, but I keep getting side-tracked by more urgent bugs. It's still on my pre-code-freeze todo list, though.
Flags: needinfo?(kmaglione+bmo)
Assignee: nobody → kmaglione+bmo
ping? :) Can we get this for 56?
Flags: needinfo?(kmaglione+bmo)
Kris, can I take over this bug from you? :)
Hey Zibi, I'm sure Kris will not mind if you take the bug, since it seems urgent for you.
Blocks: 1381580
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8864709 [details]
Bug 1357902 - Use improved locale service APIs for localization.

https://reviewboard.mozilla.org/r/136352/#review163060
Attachment #8864709 - Flags: review?(gandalf) → review+
Any chance you will have time to re-land it before 56 closes?
Ugh. I didn't realize it had been backed out again. I guess I'll just disable those two tests on Android for now. I'm probably not going to have time to dig into why they're still failing before the end of the week.
Flags: needinfo?(kmaglione+bmo)
sgtm! I'm fairly confident this patch doesn't break things. The APIs you're using are well tested and I'd like to see this landed soon to minimize the risk that people start writing web extensions banking on the wrong behavior.
https://hg.mozilla.org/mozilla-central/rev/1955c9d37186
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1356376
See Also: → 1408609
Product: Toolkit → WebExtensions
Regressions: 1712214
You need to log in before you can comment on or make changes to this bug.