1491394 - Migrate mozILocaleService to use Array<>

Status: RESOLVED FIXED

(Core :: Internationalization, enhancement, P3)

Description

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

mozILocaleService uses the old array XPIDL API.

In bug 1474369 Nika added a new one. Let's switch to it.

Comment 1

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

Attachment: Bug 1491394 - Migrate mozILocaleService to use Array<> interface. r=jfkthame

Comment 2

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

This patch removes most of the duplication between C++ and XPIDL targeting methods in LocaleService. I left one - negotiateLanguages - because I need to convert the int to enum for C++.

Everywhere else I just moved the non-XPIDL API to XPIDL API and turned return value to NS_IMETHODIMP.

I removed several invalid tests because XPIDL will not throw if you try to pass a non-array to the API.

Comment 3

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

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=34e24a89bc1056afa7bff1110a660855610adfe6

Comment 4

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

Jonathan: I updated the patch to include the test for erroring out when not-array is passed.

There's one more issue in tests. Before this change, we convert empty array elements to empty strings, so things like this:

```
negotiateLanguages([null], [undefined], ...);
```

pass.
With this change, we just pass it and then error in FilterMatches on [0] but only in debug mode.

What should I do? Remove the test? Make us throw in optimized mode too?



[0] https://searchfox.org/mozilla-central/source/intl/locale/LocaleService.cpp#437

Comment 5

[Jonathan Kew (:jfkthame)]

I think I'd favor making it consistently throw an error, like passing a non-array where an array is expected; consider it part of an overall tightening up of the API strictness. There's no good reason we actually need to support such usage, is there?

Comment 6

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

(In reply to Jonathan Kew (:jfkthame) from comment #5)
> I think I'd favor making it consistently throw an error, like passing a
> non-array where an array is expected; consider it part of an overall
> tightening up of the API strictness. There's no good reason we actually need
> to support such usage, is there?

The only things that come to mind are malformed `intl.locale.request` strings or malformed `multilingual.txt`.

Do you want to cover C++ with this as well? Should we turn `NegotiateLanguages` into a bool that can fail if any of the requested/available is empty?

Comment 7

[Jonathan Kew (:jfkthame)]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #6)
> (In reply to Jonathan Kew (:jfkthame) from comment #5)
> > I think I'd favor making it consistently throw an error, like passing a
> > non-array where an array is expected; consider it part of an overall
> > tightening up of the API strictness. There's no good reason we actually need
> > to support such usage, is there?
> 
> The only things that come to mind are malformed `intl.locale.request`
> strings or malformed `multilingual.txt`.

I think we should fix ReadRequestedLocales() so that it falls back to the default locale if parsing the intl.locale.request string doesn't result in any valid locale codes. (While we're here, change its return type to `void`, as it never returns false anyhow, and nobody checks it.)

InitPackagedLocales() already looks like it would handle a malformed multilingual.txt by falling back to the default locale if it ends up with an empty array after parsing the file, so that should be OK.

> Do you want to cover C++ with this as well? Should we turn
> `NegotiateLanguages` into a bool that can fail if any of the
> requested/available is empty?

I'm not sure it's worth adding extra checking here. AFAICS there's only one direct C++ caller at the moment, and if it were to pass in empty arrays (and potentially get empty results back), that seems reasonable behavior.

Comment 8

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

Attachment: Bug 1491394 - Update callsites to use new mozILocaleService API. r=jfkthame

Depends on D5924

Comment 9

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

All right! The patch is ready and I updated all callsites I found. Testing on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=56cadf618fbc2a3b1adf8468eb18ab3b466a3865

This is going to be a bitrotting machine, so lets try to land it soon :)

Comment 10

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

Updated try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=05852b96d839cba3e32b7b01748adba462b0f934

Comment 11

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

Updated to feedback. Green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=74fa167e2003675e3a8b97f442689806cb44ae83&selectedJob=200563168

Comment 12

[Jonathan Kew (:jfkthame)]

Comment on attachment 9009308 [details]
Bug 1491394 - Migrate mozILocaleService to use Array<> interface. r=jfkthame

Jonathan Kew (:jfkthame) has approved the revision.

Comment 13

[Jonathan Kew (:jfkthame)]

Comment on attachment 9010468 [details]
Bug 1491394 - Update callsites to use new mozILocaleService API. r=jfkthame

Jonathan Kew (:jfkthame) has approved the revision.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a33fb1d3cda0
Migrate mozILocaleService to use Array<> interface. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/54dba2807ef8
Update callsites to use new mozILocaleService API. r=jfkthame

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/a33fb1d3cda0
https://hg.mozilla.org/mozilla-central/rev/54dba2807ef8

Comment 16

[Francesco Lodolo [:flod]]

I don't see anyone from Activity Stream in this bug (CCed a couple of folks).

Did you get in touch or upstream these patches? m-c is just a dump of external code for AS.

Comment 17

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/4446

Comment 18

[[github robot]]

Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/96fa15450f8e8efae75420d47c34946614c3fff6
Backport Bug 1491394 - Update callsites to use new mozILocaleService API. r=jfkthame (#4446)