1373061 - require("sdk/l10n").get locale fallback broken

Status: RESOLVED FIXED

(Firefox :: Extension Compatibility, defect)

Description

[mirh]

I just updated to FF 55 beta. 

https://addons.mozilla.org/en-US/firefox/addon/restartfox/
And this add-on button changed name from 'restart' to 'buttonLabel'

I guess that with the new lazy tabs humongous memory improvements I basically won't need it anymore - moreover afaiu in 2 versions it shouldn't work anymore... But still it seems a bug.

Comment 1

[YF (Yang)]

got "buttonLabel" with require("sdk/l10n").get('buttonLabel') in Debugging the add-on.

Comment 2

[YF (Yang)]

I cannot reproduce it if using mozregression, need to find out its origin.

Comment 3

[Ciprian Muresan [:cmuresan], Ecosystem QA]

User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID 	20170615063713

Hi Mirh,

I was unable to reproduce the issue on the latest Release (54.0) and on the latest Nightly (56.0a1, Build ID 20170616030207).
I also tried updating Beta 54b11 to 55b2 and I had no luck.

>moreover afaiu in 2 versions it shouldn't work anymore...
Looks like the add-on is a legacy add-on and indeed it will stop working starting with Nightly 57. As a side note, Firefox has a built-in restart command. Press shift+F2 to bring up the browser toolbar and write "restart" in it. Once you press enter Firefox will restart and you won't lose your tabs.


What beta version were you on at the time of the update? 
Also what Windows OS are you using? 
And lastly, could you please try to reproduce the issue again on your end and provide the results?

Thanks,
Ciprian

Comment 4

[mirh]

At the time of the update I was using latest "beta" (54b99, I think you called it). 

Now I just updated to 55b2 (from b1) and it's still there too. 
Windows 7 x64 (but firefox is 32 bit)

E10s is disabled.

Comment 5

[Kohei Yoshino]

Please report the issue to the extension developer.

Comment 6

[YF (Yang)]

Regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=54f983e0eb8143559c35a5d45f32fabd253932b3&tochange=39a903b099a6b50e3921fd6cb2ecc7d2b53b94d3


STR:
1. Set general.useragent.locale to a locale, except en-US, es or ja (they included in restartfox).
2. Install the https://addons.mozilla.org/firefox/addon/restartfox/.
3. See the restartFox toolbar's tooltip, or its menu in File menu.

Comment 7

[mirh]

Thank you Yang.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

This is breaking Mailvelope too. Any chance you can weigh in, gandalf?

Comment 9

[Martin Giger [:freaktechnik]]

I would think this is caused by https://hg.mozilla.org/mozilla-central/rev/39a903b099a6, as it changes behavior of getPreferedLocales from what I can tell. It used to always contain en-US, which it no longer does, so the fallback to en-US that extension developers expect (not sure where that was documented, but it was a thing) no longer happens now (see the string lookup in https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/l10n/properties/core.js, which returns null if ti doesn't find a string in a locale from the list returned by getPreferedLocales).

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

I think that the summary from comment 9 sounds plausible.

I would prefer not to pretend that en-US is in requested if it's not, but rather make the language negotiation fallback on en-US if none of the requested matches.

Comment 11

[Mike Taylor [:miketaylr]]

Gandalf, how likely are we gonna be able to fix this in the 55 cycle?

Comment 12

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1373061 - Fallback on en-US in sdk/l10n.get.

Review commit: https://reviewboard.mozilla.org/r/155274/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/155274/

Comment 13

[Zibi Braniecki [:zbraniecki][:gandalf]]

This is a regression from bug 1346616. The fix is easy and I think it makes sense to bring back the expected behavior even if only for a couple releases.

Comment 14

[Axel Hecht [:Pike]]

Comment on attachment 8884354 [details]
Bug 1373061 - Fallback on en-US in sdk/l10n.get.

https://reviewboard.mozilla.org/r/155274/#review160644

Can this be tested in addon-sdk/source/test/test-l10n-locale.js or the like?

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8884354 [details]
Bug 1373061 - Fallback on en-US in sdk/l10n.get.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155274/diff/1-2/

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8884354 [details]
Bug 1373061 - Fallback on en-US in sdk/l10n.get.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155274/diff/2-3/

Comment 17

[Zibi Braniecki [:zbraniecki][:gandalf]]

Added the test.

Comment 18

[Axel Hecht [:Pike]]

Comment on attachment 8884354 [details]
Bug 1373061 - Fallback on en-US in sdk/l10n.get.

https://reviewboard.mozilla.org/r/155274/#review161210

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/97f380543bc7
Fallback on en-US in sdk/l10n.get. r=Pike

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/97f380543bc7

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 22

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8884354 [details]
Bug 1373061 - Fallback on en-US in sdk/l10n.get.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1346616 
[User impact if declined]: Potential to break extensions code
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: It's a trivial single line change that adds one locale to a list of locales.
[String changes made/needed]: none

Comment 23

[Julien Cristau [:jcristau]]

Comment on attachment 8884354 [details]
Bug 1373061 - Fallback on en-US in sdk/l10n.get.

addon compat fix, beta55+

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c5f2e0685215

Comment 25

[Andrei Vaida [:avaida]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #22)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Zibi Braniecki's assessment on manual testing needs and the fact that this fix has automated coverage.

Comment 26

[mirh]

Confirmed fixed in 55.0b9
Thank you all.

Comment 27

[Thomas Oberndörfer]

sdk/l10n.get gives still different result in 55.0b13 compared to <=54: previously if a region was not supplied, then the fallback was to use the language file (e.g. en-US -> en.properties). With 55 you need to have a separate language file for each possible region (en-US.properties, en-GB.properties, ...).

Comment 28

[Martin Giger [:freaktechnik]]

(In reply to Thomas Oberndörfer from comment #27)
> sdk/l10n.get gives still different result in 55.0b13 compared to <=54:
> previously if a region was not supplied, then the fallback was to use the
> language file (e.g. en-US -> en.properties). With 55 you need to have a
> separate language file for each possible region (en-US.properties,
> en-GB.properties, ...).

The default fallback for SDK extensions generated by JPM has always been en-US.properties and not en.properties. that was a change from cfx to jpm.

If this is not the default fallback, this would be a behaviour change outside of the SDK code base, as I can not see any code change this year in the decision to leave out the regional specifier of a language for properties (there is none, essentially). What may have changed are the contents of the list of prefered locals, since before it included all content locales (where I assume most would have the generic fallback of their regional locale), the UI language etc. Not sure what it now contains.

Comment 29

[Zibi Braniecki [:zbraniecki][:gandalf]]

As :freaktechnik said - we didn't touch the fallbacking, the default is en-US.

There's a new method that AddonSDK could use to better match between requested and available: Services.locale.negotiateLanguages(requested, available, default), which has better matching capabilities [0], but that would be a new feature, rather than fixing any regressions.


[0] http://searchfox.org/mozilla-central/source/intl/locale/LocaleService.cpp#348

Comment 30

[Thomas Oberndörfer]

I tested again and I think the difference with FF55 is that the language settings in the preferences are not used anymore for sdk/l10n.get. So for example in FF52 if you set general.useragent.locale to "en-US" and you only ship a en.properties file, but you have a "en" entry in the language preferences, then en.properties is picked.
This does not work in FF55, only general.useragent.locale is considered.
What does neither work in FF52 and FF55 is that a "en-US" setting does not fallback to a en.properties file. My expectation would be that if the exact language-region combination is not available, then the fallback should be the available language file.
Obviously for the SDK this does not matter anymore, but I wonder how the WebEx i18n API will behave in this respect.

Comment 31

[Zibi Braniecki [:zbraniecki][:gandalf]]

> Obviously for the SDK this does not matter anymore, but I wonder how the WebEx i18n API will behave in this respect.

:kmag landed bug 1357902 recently which uses Services.locale.negotitateLanguages for WebExt. This will match en-XX to en.