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

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

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

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 on attachment 8833964 [details] [diff] [review] Add a simple unit test for the localeservice API Thank you! That looks great.

Comment on attachment 8833965 [details] [diff] [review] Expose mozILocaleService Interesting. Lots to learn for me I see about memory management :)

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

Comment on attachment 8833793 [details] Bug 1336281 - Expose mozILocaleService. https://reviewboard.mozilla.org/r/109942/#review111250

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

Followup to simplify things a bit, as noted in bug 1308329 comment 34.

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?

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

With comments adjusted. Carrying over r=gandalf.

yep, lgtm!

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

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

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.

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.

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