1345957 - Tighten LocaleService::NegotiateLanguages input handling.

Status: RESOLVED FIXED

(Core :: Internationalization, enhancement, P3)

Description

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

For sanity reasons, we should not allow a defaultLocale to not be present in availableLocales as it spells disaster later on.

Comment 1

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

And also, we should not accept empty strings as locales.

Comment 2

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

As Axel pointed out the variant should be 5+ chars long: https://github.com/projectfluent/fluent.js/pull/19#discussion_r106291293

We should fix it and just special-case "-mac".

Comment 3

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

Attachment: Bug 1345957 - Tighten LocaleService::NegotiateLanguages input handling.

The main change here, is that we now require default locale to be one of the
available locales. I keep this test behind DEBUG ifdef to avoid having to
do the test in release build.

Two other changes are minor:
 - I'm aligning us with RFC4646 in two aspecs:
    - We now handle 5+alphanum variants
    - We now handle "sl-nedis" language tags (skip region, fallover to variant)
 - We're not accepting empty strings in available and requested locales in C++ API

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

Comment 4

[Jonathan Kew [:jfkthame]]

Comment on attachment 8851379 [details]
Bug 1345957 - Tighten LocaleService::NegotiateLanguages input handling.

https://reviewboard.mozilla.org/r/123694/#review126430

::: intl/locale/LocaleService.cpp:362
(Diff revision 1)
> -  if (aStrategy == LangNegStrategy::Lookup && aDefaultLocale.IsEmpty()) {
> -    return false;
> +  MOZ_ASSERT(aStrategy != LangNegStrategy::Lookup || !aDefaultLocale.IsEmpty(),
> +             "Default locale cannot be empty for strategy `lookup`.");

I'm not sure about this change; it means that in a release build, we'd no longer do the check at all. Making the argument validation here debug-only may make it too easy for developers (of front-end, add-ons, etc) using non-debug builds to make mistakes and not be alerted to them.

If I write

    langSvc.negotiateLanguages(null, null, "", ls.langNegStrategyLookup)

I don't think the API should happily return an empty result (in a non-debug); better for it to throw an error.

::: intl/locale/LocaleService.cpp:365
(Diff revision 1)
> +#ifdef DEBUG
> +  // Make sure that we have defaultLocale in availableLocales.
> +  MOZ_ASSERT(aDefaultLocale.IsEmpty() || aAvailable.Contains(aDefaultLocale),
> +             "Default locale must be present in available locales");
> +#endif

No need for the `#ifdef` around this, `MOZ_ASSERT` is debug-only anyway.

Comment 5

[Jonathan Kew [:jfkthame]]

Comment on attachment 8851379 [details]
Bug 1345957 - Tighten LocaleService::NegotiateLanguages input handling.

https://reviewboard.mozilla.org/r/123694/#review126446

::: commit-message-65b0a:3
(Diff revision 1)
> +The main change here, is that we now require default locale to be one of the
> +available locales. I keep this test behind DEBUG ifdef to avoid having to
> +do the test in release build.

Hmm. Given that people can pass any string they like for the default locale, I'm inclined to think this should not be DEBUG-only, actually. If we care that the default is valid, we should perform that check in release builds, too.

Maybe we could move the check so that it's only done if and when we decide to append `defaultLocale` to the result?

Comment 6

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

Comment on attachment 8851379 [details]
Bug 1345957 - Tighten LocaleService::NegotiateLanguages input handling.

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

Comment 7

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

Ugh, this is pretty unfortunate.

I picked up this patch and applied your feedback and unfortunately, we cannot at the moment enforce that defaultLocale is in availableLocales because of how ChromeRegistry works. The first language negotiation happens before the first package is registered.

I also start questioning whether we should enforce it. LanguageNegotiation as of now is pretty infallible, unless you try to go for lookup and don't pass the default at all.

This is useful, because a lot of code wants to settle back on any locale, using the default if all fails, but doesn't want to fail if default is not present.

If we start throwing, they'll switch to cover every call to negotiateLanguages into try and assign "en-US" in catch.

I'd say it's better to let the default locale be missing, maybe with a warning later on once we move away from ChromeRegistry for l10n.

Either way, this patch is now simpler.

Comment 8

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

Comment on attachment 8851379 [details]
Bug 1345957 - Tighten LocaleService::NegotiateLanguages input handling.

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

Comment 9

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

Comment on attachment 8851379 [details]
Bug 1345957 - Tighten LocaleService::NegotiateLanguages input handling.

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

Comment 10

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

Comment on attachment 8851379 [details]
Bug 1345957 - Tighten LocaleService::NegotiateLanguages input handling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123694/diff/4-5/

Comment 11

[Jonathan Kew [:jfkthame]]

Comment on attachment 8851379 [details]
Bug 1345957 - Tighten LocaleService::NegotiateLanguages input handling.

https://reviewboard.mozilla.org/r/123694/#review171090

OK, seems reasonable. The one thing I wonder is whether it would be preferable to make the JS-callable NegotiateLanguages "more infallible" by having it use the result of GetDefaultLocale() if the underlying C++ method returns false (i.e. if it was called with strategy=lookup and defaultLocale=""), instead of returning NS_ERROR_INVALID_ARG. So basically it'd use GetDefaultLocale as a fallback if the caller failed to provide a defaultLocale arg. WDYT?

::: intl/locale/tests/unit/test_localeService.js:130
(Diff revision 5)
>    do_check_true(requestedLocale === "tlh", "requestedLocale returns the right value");
>  
> -  Services.prefs.setCharPref(PREF_SELECTED_LOCALE, "");
> +  Services.prefs.setCharPref(PREF_SELECTED_LOCALE, "sr-Cyrl");
>  
>    requestedLocale = localeService.getRequestedLocale();
> -  do_check_true(requestedLocale === "", "requestedLocale returns empty value value");
> +  do_check_true(requestedLocale === "sr-Cyrl", "requestedLocale returns empty value value");

The message here needs updating to match the changed test!

Comment 12

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

Attachment: Bug 1345957 - Tighten LocaleService::NegotiateLanguages input handling.

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

Comment 13

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

Attachment: Bug 1345957 - Update the use of LocaleService API in gfxPlatformFontList.

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

Comment 14

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

Finally got back to this! Many items from this patch already landed in the meantime, so this is mostly about turning NegotiateLanguages to be more infallible and adding a couple assertions.

When fixing gfx code I noticed that they're a good example for a switch to MozLocale, so I did that.

Comment 15

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

Comment on attachment 8956694 [details]
Bug 1345957 - Update the use of LocaleService API in gfxPlatformFontList.

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

Comment 16

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

Comment on attachment 8956693 [details]
Bug 1345957 - Tighten LocaleService::NegotiateLanguages input handling.

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

Comment 17

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

Comment on attachment 8956694 [details]
Bug 1345957 - Update the use of LocaleService API in gfxPlatformFontList.

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

Comment 18

[Jonathan Kew [:jfkthame]]

Comment on attachment 8956693 [details]
Bug 1345957 - Tighten LocaleService::NegotiateLanguages input handling.

https://reviewboard.mozilla.org/r/225658/#review234554

Comment 19

[Jonathan Kew [:jfkthame]]

Comment on attachment 8956694 [details]
Bug 1345957 - Update the use of LocaleService API in gfxPlatformFontList.

https://reviewboard.mozilla.org/r/225660/#review234560

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/675d641249f1
Tighten LocaleService::NegotiateLanguages input handling. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/8d24dfbdc98a
Update the use of LocaleService API in gfxPlatformFontList. r=jfkthame

Comment 21

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/675d641249f1
https://hg.mozilla.org/mozilla-central/rev/8d24dfbdc98a

Comment 22

[Marco Castelluccio [:marco]]

I think this broke Linux code coverage builds.