Add LocaleService::SetRequestedLocales

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: zbraniecki, Assigned: zbraniecki)

Tracking

(Blocks 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

As a counterpart to bug 1344445, we need an API for the app to update the list of requested locales so that they don't directly set the pref.

Currently, Android has UI for updating locale, and it should use the API.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
This patch also removes the StaticPtr leftovers from the GetRequestedLocales patch that somehow stayed in.
Comment on attachment 8846468 [details]
Bug 1346617 - Add LocaleService::SetRequestedLocales.

https://reviewboard.mozilla.org/r/119502/#review121454

::: intl/locale/LocaleService.cpp:574
(Diff revision 1)
> +NS_IMETHODIMP
> +LocaleService::SetRequestedLocales(bool matchOS,
> +                                   const char** aRequested,
> +                                   uint32_t aRequestedCount)
> +{
> +  MOZ_ASSERT(matchOS || aRequestedCount == 1, "We can only handle one requested locale");

The plan is to eventually support a list rather than just a single requested locale, presumably, so this is temporary?

This assertion would indicate that the caller (which is probably front-end code) has misused the API, e.g. by implementing UI for a list before we've updated the service to handle it. Given that I'd guess front-end devs are likely to be working with non-debug Gecko, so they wouldn't see the assertion, it might be better to check the args and return NS_ERROR_INVALID_ARG if they're bad. (In the case when `matchOS == false && aRequestedCount == 0` we really shouldn't continue here, as the access to `aRequested[0]` is unsafe.)

::: intl/locale/mozILocaleService.idl:131
(Diff revision 1)
> +   * localized to.
> +   *
> +   * The argument is an ordered list of locale IDs which should be
> +   * used as a requestedLocales input list for language negotiation.
> +   *
> +   * Example: ["en-US", "de", "pl", "sr-Cyrl", "zh-Hans-HK"]

The implementation doesn't currently support a list, and will assert if given one; so this comment is rather misleading for the time being. If we're going to land a single-value-only implementation initially, there should be a note here to let potential users know about the limitation.

::: intl/locale/mozILocaleService.idl:133
(Diff revision 1)
> +  void setRequestedLocales(in boolean matchOS,
> +                           [array, size_is(aRequestedCount)] in string aRequested,
> +                           [optional] in unsigned long aRequestedCount);

Also, I wondered about the use of a separate `matchOS` flag in this API. How about simply using an empty list to mean "match the OS"?
Love the idea. Updated the patch to take an empty array as matchOS. Updated the patch and to tests.
Comment on attachment 8846468 [details]
Bug 1346617 - Add LocaleService::SetRequestedLocales.

https://reviewboard.mozilla.org/r/119502/#review121930

::: intl/locale/tests/unit/test_localeService.js:129
(Diff revision 3)
> +  const localeService =
> +    Components.classes["@mozilla.org/intl/localeservice;1"]
> +    .getService(Components.interfaces.mozILocaleService);

This seems to happen in every function here. How about just doing it once at the start of the file, like osPrefs?
Attachment #8846468 - Flags: review?(jfkthame) → review+
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dd6b11222fbc
Add LocaleService::SetRequestedLocales. r=jfkthame
https://hg.mozilla.org/mozilla-central/rev/dd6b11222fbc
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.