Closed Bug 1491394 Opened Last year Closed Last year

Migrate mozILocaleService to use Array<>

Categories

(Core :: Internationalization, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

mozILocaleService uses the old array XPIDL API.

In bug 1474369 Nika added a new one. Let's switch to it.
Blocks: 1346877
Priority: -- → P3
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
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.
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
Flags: needinfo?(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?
Flags: needinfo?(jfkthame)
(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?
Flags: needinfo?(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.
Flags: needinfo?(jfkthame)
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 on attachment 9009308 [details]
Bug 1491394 - Migrate mozILocaleService to use Array<> interface. r=jfkthame

Jonathan Kew (:jfkthame) has approved the revision.
Attachment #9009308 - Flags: review+
Comment on attachment 9010468 [details]
Bug 1491394 - Update callsites to use new mozILocaleService API. r=jfkthame

Jonathan Kew (:jfkthame) has approved the revision.
Attachment #9010468 - Flags: review+
Pushed by zbraniecki@mozilla.com:
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
See Also: → 1493220
https://hg.mozilla.org/mozilla-central/rev/a33fb1d3cda0
https://hg.mozilla.org/mozilla-central/rev/54dba2807ef8
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
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.
Depends on: 1493367
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)
You need to log in before you can comment on or make changes to this bug.