Add LocaleService::SetRequestedLocales

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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)

Updated

2 years ago
Blocks: 1334772
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
(Assignee)

Comment 2

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

Comment 3

2 years ago
mozreview-review
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"?
(Assignee)

Updated

2 years ago
Blocks: 1346877
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

2 years ago
Love the idea. Updated the patch to take an empty array as matchOS. Updated the patch and to tests.
(Assignee)

Updated

a year ago
Blocks: 1347071

Comment 7

a year ago
mozreview-review
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+
Comment hidden (mozreview-request)

Comment 9

a year ago
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dd6b11222fbc
Add LocaleService::SetRequestedLocales. r=jfkthame

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dd6b11222fbc
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.