1336281 - Expose mozILocaleService

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

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

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

Comment 1

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

Attachment: Bug 1336281 - Expose mozILocaleService.

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

Comment 2

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

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.

Comment 3

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

Attachment: Bug 1336281 - Expose mozILocaleService.

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

Comment 4

[Jonathan Kew [:jfkthame]]

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.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: Expose mozILocaleService

Comment 6

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

Comment on attachment 8833793 [details]
Bug 1336281 - Expose mozILocaleService.

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

Comment 7

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

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

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: 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).

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: 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.

Comment 10

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

Comment on attachment 8833793 [details]
Bug 1336281 - Expose mozILocaleService.

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

Comment 11

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

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

Thank you! That looks great.

Comment 12

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

Comment on attachment 8833965 [details] [diff] [review]
Expose mozILocaleService

Interesting. Lots to learn for me I see about memory management :)

Comment 13

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

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

Comment 14

[Jonathan Kew [:jfkthame]]

Comment on attachment 8833793 [details]
Bug 1336281 - Expose mozILocaleService.

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9a949ee9c2cd
Expose mozILocaleService. r=jfkthame

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9a949ee9c2cd

Comment 17

[Jonathan Kew [:jfkthame]]

Attachment: 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.

Comment 18

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

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?

Comment 19

[Jonathan Kew [:jfkthame]]

(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.)

Comment 20

[Jonathan Kew [:jfkthame]]

Attachment: followup - Unify the JS-callable and C++-only versions of LocaleService::GetAppLocale

With comments adjusted. Carrying over r=gandalf.

Comment 21

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

yep, lgtm!

Comment 22

[Jonathan Kew [:jfkthame]]

Attachment: 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?

Comment 23

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

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?

Comment 24

[Jonathan Kew [:jfkthame]]

OK, that makes sense. Let's stick with the callable function, then.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
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

Comment 26

[Jonathan Kew [:jfkthame]]

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.

Comment 27

[Carsten Book [:Tomcat]]

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

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 29

[Jonathan Kew [:jfkthame]]

(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.

Comment 30

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

> :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.