1346617 - Add LocaleService::SetRequestedLocales

Status: RESOLVED FIXED

(Core :: Internationalization, enhancement)

Description

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

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.

Comment 1

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

Attachment: Bug 1346617 - Add LocaleService::SetRequestedLocales.

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

Comment 2

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

This patch also removes the StaticPtr leftovers from the GetRequestedLocales patch that somehow stayed in.

Comment 3

[Jonathan Kew [:jfkthame]]

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"?

Comment 4

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

Comment on attachment 8846468 [details]
Bug 1346617 - Add LocaleService::SetRequestedLocales.

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

Comment 5

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

Comment on attachment 8846468 [details]
Bug 1346617 - Add LocaleService::SetRequestedLocales.

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

Comment 6

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

Love the idea. Updated the patch to take an empty array as matchOS. Updated the patch and to tests.

Comment 7

[Jonathan Kew [:jfkthame]]

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?

Comment 8

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

Comment on attachment 8846468 [details]
Bug 1346617 - Add LocaleService::SetRequestedLocales.

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dd6b11222fbc
Add LocaleService::SetRequestedLocales. r=jfkthame

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dd6b11222fbc