Expose mozILocaleService

RESOLVED FIXED in Firefox 54

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

2 years ago
We want to expose mozilla::intl::LocaleService (bug 1332207) to JS code to unify all callsites looking for current app locales.
(Assignee)

Updated

2 years ago
Blocks: 1334772
Depends on: 1332207
(Assignee)

Updated

2 years ago
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Comment 2

2 years ago
Comment on attachment 8833141 [details]
Bug 1336281 - Expose mozILocaleService.

This seems to be similar to what we had before, just based on the code that landed in central.
Attachment #8833141 - Flags: feedback?(jfkthame)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8833141 - Attachment is obsolete: true
Attachment #8833141 - Flags: feedback?(jfkthame)

Comment 4

2 years ago
mozreview-review
Comment on attachment 8833793 [details]
Bug 1336281 - Expose mozILocaleService.

https://reviewboard.mozilla.org/r/109942/#review110920

Basically this looks OK, with a few minor code-style nits noted below.

However, I'd like to propose a slight variation: I think we can simplify things if, instead of creating a new C++ class to implement mozILocaleService, we make mozilla::intl::LocaleService itself implement that interface (in addition to the C++-callable interface it already offers). This removes one layer of "wrapping", and allows the implementations of the mozILocaleService methods to directly access the LocaleService instance's data as needed.

I'll attach a revised version of your patch that aims to do this. It should provide exactly the same JS-callable interface, while reducing the amount of underlying boilerplate needed to implement it. This helps to reduce the proliferation of "locale-service" classes we currently have going on.

::: intl/locale/mozLocaleService.cpp:28
(Diff revision 1)
> +mozLocaleService::GetAppLocales(JSContext* cx, JS::MutableHandleValue retval)
> +{
> +  nsTArray<nsCString> appLocales;
> +  LocaleService::GetInstance()->GetAppLocales(appLocales);
> +
> +  PRUint32 appLocalesNum = appLocales.Length();

s/PRUint32/uint32_t/

::: intl/locale/mozLocaleService.cpp:33
(Diff revision 1)
> +  PRUint32 appLocalesNum = appLocales.Length();
> +
> +  JS::RootedObject locales(cx, JS_NewArrayObject(cx, appLocalesNum));
> +  JS::Rooted<JS::Value> value(cx);
> +
> +  JSString* str;

No need to declare this here, just declare it at the time of use.

::: intl/locale/mozLocaleService.cpp:36
(Diff revision 1)
> +  JS::Rooted<JS::Value> value(cx);
> +
> +  JSString* str;
> +
> +  for (size_t i = 0; i < appLocalesNum; i++) {
> +    str = JS_NewStringCopyZ(cx, appLocales[i].get());

Slightly more optimal, I guess:

    const nsCString& loc = appLocales[i];
    JSString* str = JS_NewStringCopyN(cx, loc.get(), loc.Length());

(avoiding the need for a strlen() within the JS_New... call, as the nsCString already knows its length).

::: intl/locale/mozLocaleService.cpp:47
(Diff revision 1)
> +  nsAutoCString appLocale;
> +  LocaleService::GetInstance()->GetAppLocale(appLocale);
> +
> +  retval = appLocale;

This involves a redundant copy of the string. Just do

    LocaleService::GetInstance()->GetAppLocale(retval);

to get the result directly into the outparam.
Attachment #8833793 - Flags: review?(jfkthame)
Created attachment 8833807 [details] [diff] [review]
Expose mozILocaleService
Attachment #8833807 - Flags: feedback?(gandalf)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

2 years ago
Comment on attachment 8833807 [details] [diff] [review]
Expose mozILocaleService

Sweet, thank you!

I took your patch and applied it into the mozreview with one change - I followed Mozilla Style Guide to name the IDL method using interCaps [0]


[0] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Use_leading-lowercase.2C_or_.22interCaps.22
Attachment #8833807 - Attachment is obsolete: true
Attachment #8833807 - Flags: feedback?(gandalf)
Created attachment 8833964 [details] [diff] [review]
Add a simple unit test for the localeservice API

I figured we should have some kind of unit test for the API here, so I put this together. Running it highlighted the fact that the patch as it stands isn't quite right, manifested as a crash during shutdown (because the refcounting is wrong). So I'll post a fixed-up version of the patch (easier to do that than to describe what needs fixing in individual line comments).
Attachment #8833964 - Flags: review?(gandalf)
Created attachment 8833965 [details] [diff] [review]
Expose mozILocaleService

Here's the corrected patch that runs the unit test without issues. Pushed a try run to double-check: https://treeherder.mozilla.org/#/jobs?repo=try&revision=292459bb91cb3396967ed64913557bffe839fb83.
Attachment #8833965 - Flags: feedback?(gandalf)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

2 years ago
Comment on attachment 8833964 [details] [diff] [review]
Add a simple unit test for the localeservice API

Thank you! That looks great.
Attachment #8833964 - Flags: review?(gandalf) → review+
(Assignee)

Comment 12

2 years ago
Comment on attachment 8833965 [details] [diff] [review]
Expose mozILocaleService

Interesting. Lots to learn for me I see about memory management :)
Attachment #8833965 - Flags: feedback?(gandalf) → feedback+
(Assignee)

Comment 13

2 years ago
Updated the MR patch with your latest version and the new test.

Comment 14

2 years ago
mozreview-review
Comment on attachment 8833793 [details]
Bug 1336281 - Expose mozILocaleService.

https://reviewboard.mozilla.org/r/109942/#review111250
Attachment #8833793 - Flags: review?(jfkthame) → review+
(Assignee)

Updated

2 years ago
Attachment #8833964 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8833965 - Attachment is obsolete: true

Comment 15

2 years ago
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9a949ee9c2cd
Expose mozILocaleService. r=jfkthame

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9a949ee9c2cd
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Created attachment 8834395 [details] [diff] [review]
followup - Unify the JS-callable and C++-only versions of LocaleService::GetAppLocale

Followup to simplify things a bit, as noted in bug 1308329 comment 34.
Attachment #8834395 - Flags: review?(gandalf)
(Assignee)

Comment 18

2 years ago
Comment on attachment 8834395 [details] [diff] [review]
followup - Unify the JS-callable and C++-only versions of LocaleService::GetAppLocale

ok, that makes sense. My only question is if there's a way not to lose the doc comment. I already wasn't sure if docs should be in the IDL of .h file, and now it seems that methods are spread between. Maybe move the docs to .cpp?
Attachment #8834395 - Flags: review?(gandalf) → review+
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #18)
> My only question is if there's a way not to lose the
> doc comment. I already wasn't sure if docs should be in the IDL of .h file,
> and now it seems that methods are spread between. Maybe move the docs to
> .cpp?

Actually, the doc comments for the IDL methods should be in the IDL; people using that interface shouldn't need to look into the C++ implementation for info.

Then, for methods that aren't exposed via IDL but are only in the concrete C++ implementation, the docs belong with the method declarations in the header. JS callers never need to look at them because they can't call them anyhow.

I'll move the comments around accordingly. (In the special case of GetAppLocales, where we have both JS-friendly and C++-friendly versions, I'll add a note cross-referencing to the other declaration.)
Created attachment 8834645 [details] [diff] [review]
followup - Unify the JS-callable and C++-only versions of LocaleService::GetAppLocale

With comments adjusted. Carrying over r=gandalf.
Attachment #8834395 - Attachment is obsolete: true
(Assignee)

Comment 21

2 years ago
yep, lgtm!
Created attachment 8834832 [details] [diff] [review]
followup - Unify the JS-callable and C++-only versions of LocaleService::GetAppLocale, and declare it as a read-only attribute in IDL

One more iteration of the followup here: as Gijs suggested, we can declare appLocale as an attribute, so that usage from JS becomes simply locSvc.appLocale rather than locSvc.getAppLocale(). Do you want to go with this form?
Attachment #8834832 - Flags: review?(gandalf)
Attachment #8834645 - Attachment is obsolete: true
(Assignee)

Comment 23

2 years ago
Comment on attachment 8834832 [details] [diff] [review]
followup - Unify the JS-callable and C++-only versions of LocaleService::GetAppLocale, and declare it as a read-only attribute in IDL

So, I think I'm against that change.

I realized that since we want to introduce a parameter `context` that will allow us to use different language negotiation strategies for different use cases (casus of main browser in `fr` vs devtools in `en-US`) it wouldn't make much sense to make it an attribute later and then switch back to a callable method.

An alternative would be to keep it as an attribute for the "main" context and if someone wants a different context we'd have `GetAppLocalesForContext` method but came to conclusion that it's not worth it.

What do you think :jfkthame?
Flags: needinfo?(jfkthame)
OK, that makes sense. Let's stick with the callable function, then.
Flags: needinfo?(jfkthame)

Comment 25

2 years ago
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0bbcee26372
followup - Unify the JS-callable and C++-only versions of LocaleService::GetAppLocale, and declare it as a read-only attribute in IDL. r=gandalf
Argh - sorry, I pushed the patch with the wrong version of the commit message; that really was the callable-function version of the patch, not the attribute version. Doesn't seem worth the churn of backing out and re-pushing just to fix that, so I'm just noting it here.
(Assignee)

Updated

2 years ago
Attachment #8834832 - Flags: review?(gandalf)
Why does mozILocaleService return jsval when it could just return an array of strings and then its implementation wouldn't need to use any JSAPI, but just allocate array and return it.

I guess the idl syntax would be rather ugly
void appLocales([optional] out unsigned long aCount,
                [retval, array, size_is(aCount)] out
                DOMString aOutArray);

As an example of nsIEventListenerService::getListenerInfoFor returns an array.

In general using JSAPI outside bindings and xpconnect should be avoided if easily possible, since it tends to be rather error prone.
(In reply to Olli Pettay [:smaug] from comment #28)
> Why does mozILocaleService return jsval when it could just return an array
> of strings and then its implementation wouldn't need to use any JSAPI, but
> just allocate array and return it.
> 
> I guess the idl syntax would be rather ugly
> void appLocales([optional] out unsigned long aCount,
>                 [retval, array, size_is(aCount)] out
>                 DOMString aOutArray);
> 
> As an example of nsIEventListenerService::getListenerInfoFor returns an
> array.
> 
> In general using JSAPI outside bindings and xpconnect should be avoided if
> easily possible, since it tends to be rather error prone.

:gandalf, do you have reasons why you went for the jsval version here?

(FWIW, at one point I had a version of the patch that used an IDL declaration a lot like the above, except that I used plain `string` rather than `DOMString` as the type of the array elements. Locale tags are plain-ASCII so 8-bit strings worked fine.)

I'd have no issue with converting this (to either the DOMString or string array versions) if that's the decision.
Flags: needinfo?(gandalf)
(Assignee)

Comment 30

2 years ago
> :gandalf, do you have reasons why you went for the jsval version here?

Nope, I just didn't know how else to mark returning of an array of strings.
Flags: needinfo?(gandalf)
Blocks: 1341094
You need to log in before you can comment on or make changes to this bug.