Closed Bug 1345957 Opened 7 years ago Closed 6 years ago

Tighten LocaleService::NegotiateLanguages input handling.

Categories

(Core :: Internationalization, enhancement, P3)

Product:
Core
Component:
Internationalization
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Bug 1345957 - Tighten LocaleService::NegotiateLanguages input handling.
59 bytes, text/x-review-board-request
jfkthame
: review+
Details
Bug 1345957 - Update the use of LocaleService API in gfxPlatformFontList.
59 bytes, text/x-review-board-request
jfkthame
: review+
Details 
For sanity reasons, we should not allow a defaultLocale to not be present in availableLocales as it spells disaster later on.
Depends on: 1337694
And also, we should not accept empty strings as locales.
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".
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Summary: Throw in LocaleService::NegotaiteLanguages if defaultLocale is not in availableLocales → Throw in LocaleService::NegotiateLanguages if defaultLocale is not in availableLocales
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 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?
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.
Summary: Throw in LocaleService::NegotiateLanguages if defaultLocale is not in availableLocales → Tighten LocaleService::NegotiateLanguages input handling.
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!
Priority: -- → P3
Attachment #8851379 - Attachment is obsolete: true
Attachment #8851379 - Flags: review?(jfkthame)
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 on attachment 8956693 [details]
Bug 1345957 - Tighten LocaleService::NegotiateLanguages input handling.

https://reviewboard.mozilla.org/r/225658/#review234554
Attachment #8956693 - Flags: review?(jfkthame) → review+
Comment on attachment 8956694 [details]
Bug 1345957 - Update the use of LocaleService API in gfxPlatformFontList.

https://reviewboard.mozilla.org/r/225660/#review234560
Attachment #8956694 - Flags: review?(jfkthame) → review+
Pushed by zbraniecki@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/675d641249f1
https://hg.mozilla.org/mozilla-central/rev/8d24dfbdc98a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
I think this broke Linux code coverage builds.
Depends on: 1447189
Flags: needinfo?(gandalf)
Flags: needinfo?(gandalf)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: