Closed Bug 1337078 Opened 7 years ago Closed 7 years ago

Improve the use of Android API in OSPreferences

Categories

(Core Graveyard :: Widget: Android, defect, P3)

Product:
Core Graveyard
Component:
Widget: Android
Type:
defect

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: zbraniecki, Assigned: droeh, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(2 files, 5 obsolete files)

Bug 1337078 - Improve the use of Android API in OSPreferences;
59 bytes, text/x-review-board-request
jchen
: review+
Details
Add JNI getLocale and use it in OSPreferences
12.62 KB, patch
snorp
: review+
Details | Diff | Splinter Review 
In bug 1333184 we're introducing OSPreferences API. In this bug we'll work on using more of the Android API to handle more of the information from the OS.
Depends on: 1333184
Blocks: 1255242
Mentor: gandalf
Keywords: good-first-bug
Hi, I am new here and am looking for a good first Java bug to solve. Can I pick this bug and start working on it?
Hi vaibhav,

I'm afraid that the code here would be in C++, not Java.

If you're still interested in it, I'll be happy to mentor you!
Flags: needinfo?(vaibhavsinghacads)
Sure! I haven't used C++ much but would love the challenge!
Flags: needinfo?(vaibhavsinghacads)
Excellent!

The main interface is here: http://searchfox.org/mozilla-central/source/intl/locale/OSPreferences.h

The first interesting function for you is 

bool ReadSystemLocales(nsTArray<nsCString>& aRetVal);

This is a private method implemented separately for each operating system. You can see example of that in:

http://searchfox.org/mozilla-central/source/intl/locale/windows/OSPreferences_win.cpp#13
http://searchfox.org/mozilla-central/source/intl/locale/mac/OSPreferences_mac.cpp#13
http://searchfox.org/mozilla-central/source/intl/locale/gtk/OSPreferences_gtk.cpp#15
http://searchfox.org/mozilla-central/source/intl/locale/android/OSPreferences_android.cpp#13

In particular, you'll be reworking the last piece.
Currently, has two codepaths. If we do have "ENABLE_INTL_API" set to true:
 - the function just takes `uloc_getDefault` which is an ICU function to retrieve the default locale.
If it's set to false:
 - it uses nsPosixLocale::GetXPLocale

My belief is that Android provides better API for retrieving current OS locale or even locale fallback list.

Your first task is to investigate it and rewrite this function in the OSPreferences_android.cpp file to make it work.

You can build Android using those instructions: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build


===

The second part would be to find the APIs in Android to read the current user preferences wrt. date/time formatting - things like hour12/hour24 time format, or date format ("YYYY-mm-dd" vs "mm/dd/yyyy" etc.).
I don't know if Android exposes that, maybe it doesn't, but it's worth investigating.

Let me know if that's enough info to start!
Hi Zibi,

Thanks for the headstart, it definitely would help me out. I am currently trying to set up my build, and cannot upgrade some libraries which the build environment needs(some gtk libraries) because a different project needs the old libraries in my work station. That dependency will not be present after 1st April though, so I will upgrade my Ubuntu environment to 16.04 that day and will start the build process then.

In the meantime I will spend some time refreshing on basic C++ to get started.
:vaibhav - unfortunately, this bug popped up as a dependency for a regression (bug 1351873) which I need to fix.

In result, I'm afraid I have to take it and fix ASAP.

Please, when you get your machine working, look at other easy first bugs (for example bug 1337065, bug 1337067, or bug 1337069) which are not as urgent. Thank you!
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
The current locale on Android -- that is, the locale currently being used in the front end -- is not necessarily the OS locale. Sometimes it's a locale that isn't even present in the Android system locale list; we ship a bigger set than is present on most devices.

Assuming that what you want is that, the locale the user chose (which defaults to the OS locale), you will need to use JNI.

This _should_ be as simple as calling

  Locale.getDefault()

and ensuring that the locale string is translated, which you can do by calling

  org.mozilla.gecko.Locales.getLanguage(Locale.getDefault())

If there's any chance that a locale-aware activity has not yet been initialized by the time you call this, you need to first initialize the locale system.

To do that, get hold of an application Context, and call:

  org.mozilla.gecko.Locales.initializeLocale(appContext);


If you want to find out which locale was baked into the APK, use:

  org.mozilla.gecko.Locales.getLocaleManager().getFallbackLocaleTag()

Right now that always returns "en-US".

If you want to find out the true OS locale, we can dig that out of BrowserLocaleManager, or you can get it from Gecko prefs (see handling of Locale:OS in browser.js).

Those two things together should be a sufficient list.
A good starting point for figuring out the JNI stuff is

mozglue/android/APKOpen.cpp

-- look for "GeckoAppShell".

You'll want Jim Chen, glandium, or snorp to review (and I'm happy to, of course).
I started looking into this, but lack of familiarity with JNI and FFI in C++ overwhelmed me. I decided to focus on fixing the regression first (bug 1351873 comment 13), but I believe that this bug should be addressed soon.

If anyone more familiar with Java can kickstart a WIP to help me jump over the FFI cliff, I'll do my best to finish the work and refactor Fennec calls to properly use LocaleService.
Status: ASSIGNED → NEW
(In reply to Richard Newman [:rnewman] from comment #7)
> The current locale on Android -- that is, the locale currently being used in
> the front end -- is not necessarily the OS locale. Sometimes it's a locale
> that isn't even present in the Android system locale list; we ship a bigger
> set than is present on most devices.
> 
> Assuming that what you want is that, the locale the user chose (which
> defaults to the OS locale), you will need to use JNI.
> 
> This _should_ be as simple as calling
> 
>   Locale.getDefault()

No, it won't return current system locale.  It returns the locale at start up, so it isn't updated by user change.  For current system locale, use Resources.getSystem().getConfiguration().locale.  When api level is 24+, you can use LocaleList. (Actually, Gecko doesn't support API level 24+...)
(In reply to Makoto Kato [:m_kato] from comment #10)

> It returns the locale at start up, so it isn't updated by user change.

It's a little more complicated than that.

The docs for Locale.getDefault:

---
Gets the current value of the default locale for this instance of the Java Virtual Machine.

The Java Virtual Machine sets the default locale during startup based on the host environment. It is used by many locale-sensitive methods if no locale is explicitly specified. It can be changed using the setDefault method.
---

Each Android process gets a VM. The VM inherits its locale from the OS. The Fennec locale switcher uses `setDefault` to alter the locale for the entire Fennec process. A subsequent call to `getDefault` returns the last value provided to `setDefault`, falling back, naturally, to the value provided when the process was created.

Android processes can listen for OS configuration changes, including for locales, and indeed we do so:

https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java#78

If you don't listen for those locale changes, Android will kill your process and restart it with the new configuration, which is a brute-force way of making sure that `getDefault` returns the right value!

So:

- getDefault is correct at process launch.
- It's correct after the locale picker sets the locale.
- It remains correct after OS locale changes, because we process those configuration change notices in GeckoApplication.
:jfkthame, putting this on your radar in case you can help me with this.

I'd like to make it my priority now so that we don't live with the pref+os-bindings combo for too long especially since pref initialization is sub-optimal (bug 1344355) at bootstrap.
Flags: needinfo?(jfkthame)
I filed bug 1352343 to make LocaleService react to the intl:system-locales-changed event. I put it as a dependency on this one since this will be the first platform to trigger the event.
:zibi I understand the urgency to fix a regression bug, and I can probably pick up bug 1337069 to start off with, if that is fine with you.
yes, that's perfect, thank you! The instructions from comment 4 apply here, but your investigation is into Unix APIs instead of Android APIs. Let's talk more in bug 1337069 :)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #12)
> :jfkthame, putting this on your radar in case you can help me with this.

I suspect I won't have much time to look into this for now, sorry. I'm not familiar with JNI and all that, so there'll be a learning curve.... if you can tag someone from the Fennec team who already knows their way around that side of things, it'll probably be a lot more efficient.
Flags: needinfo?(jfkthame)
Hi! I would like to pick this up. I have not worked with JNI in fennec but I have the experience from my other projects. 
I can try to fix this :)

Would this require a complete backend build, or an artifact one would do?
Flags: needinfo?(gandalf)
You need a complete backend build unfortunately.

Great to hear that you're willing to take it. Please, feel free to ask either me, or :rnewman, :snorp, :glandium or Jim Chen for any help or feedback as you go.

Thank you so much for expressing interest in this bug! It'll help us a lot!
Assignee: gandalf → nobody
Flags: needinfo?(gandalf)
Hi Zibi!

As I could see in the discussion above, on Android side, we're already listening for OS configuration changes including locales. So calling Locale.getDefault() would give us correct results. 

Technically, what I could understand is that we want to call 'org.mozilla.gecko.Locales.getLanguage(Locale.getDefault())' from our C++ code, precisely in 'ReadSystemLocales' function of OSPreferences_android.cpp. 
Am I correct here?

I went through the code in mozglue/android/APKOpen.cpp, I can see the JNI methods to load native libraries there. 

Based on my JNI experience, we want to call is a Java function via C++. I know how to do so via JNI, I mean with some research and help, I think I will be able to write a function in APKOpen.cpp which calls org.mozilla.gecko.Locales.getLanguage(Locale.getDefault()) function which will give us correct locale.  

Actually, I want to know how OSPreferences_android.cpp and APKOpen.cpp are connected. I'm not sure how exactly I would call Locale.getDefault() in OSPreferences_android.cpp.

I'd appreciate some inputs here. 
Thanks :)
Flags: needinfo?(gandalf)
> Am I correct here?

Not sure.

I assumed we just need to call "Locale.getDefault()", so I'm not sure why we'd need the `org.mozilla.get.Locales.getLanguage` part.

> Actually, I want to know how OSPreferences_android.cpp and APKOpen.cpp are connected. I'm not sure how exactly I would call Locale.getDefault() in OSPreferences_android.cpp.


I have no idea.

My very naive guess was that you won't need APKOpen.cpp, just make OSPreferences_android.cpp call `Locale.getDefault` in Java.
Flags: needinfo?(gandalf)
(In reply to Gautam Prajapati from comment #20)

> Technically, what I could understand is that we want to call
> 'org.mozilla.gecko.Locales.getLanguage(Locale.getDefault())' from our C++
> code, precisely in 'ReadSystemLocales' function of
> OSPreferences_android.cpp. 
> Am I correct here?

Yes.

> Actually, I want to know how OSPreferences_android.cpp and APKOpen.cpp are
> connected. I'm not sure how exactly I would call Locale.getDefault() in
> OSPreferences_android.cpp.

They're not; APKOpen.cpp was merely a good place to point Zibi to show examples of existing JNI calls in the tree.

You should either do the JNI work inline in OSPreferences_android.cpp, or find a good place in mozglue/android/.
gandalf and I talked about this in IRC.  You can certainly query the Android OS locale from JNI, although it's going to be tricky since the multi-locale call (https://developer.android.com/reference/android/os/LocaleList.html#getDefault()) is new in Android 24, and I don't think our generating code in widget/android/bindings will deal with that.  So, it's probably best to define a Java wrapper that does sensible things on Android 24 and on Android < 23.  (Perhaps returning a list with one element on 23 and below.)

That should be easy to expose to Gecko via JNI, although I don't know the C/C++ scoping rules well enough to say how you access that symbol from outside of widget/android, say.

ni to jchen to comment on the SDK bindings and how hard it would be to expose a 24+ JNI function.
Flags: needinfo?(nchen)
Yeah you can do the version check in Java. To do it natively, right now the auto-generated SDK bindings in widget/android/bindings are limited to API 16. Once bug 1352177 is fixed, we can extend the version limit to API 24 and above. You can already check API version through `mozilla::jni::GetAPIVersion()` so once we have bindings support, using it won't be a problem.
Flags: needinfo?(nchen)
As :rnewman suggested in comment 7, we need to call Locale.getDefault() via JNI. It will return us the correct OS locale.
 
Then we can do other stuff like properly translating it using getLanguage() function of Locales.java gecko class(https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/Locales.java#84) after proper initialization.   

This can be easily done by auto-generated bindings because they support methods/classes till API 16. 

Then a tricky part comes in, API 24 introduced a new method to get the OS Locale, LocaleList.getDefault(). I have some queries here:

- Can I know why do we want to use LocaleList.getDefault() when using Locale.getDefault() can get us the required locale? 
Sorry if I missed something. :)

- Even if we have to use the LocaleList.getDefault(), I think the best way would be to modify the Gecko's Locales.java [https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/Locales.java] to handle the appropriate call to getDefault() according to API levels <=23 and 24. 

As :snorp and I discussed on IRC, we can annotate gecko's Locales.java with @WrapForJNI to access it in JNI similar to bindings(Am I correct here?). I'd appreciate if you can elaborate more on this if we will be doing it. :) 

> Once bug 1352177 is fixed, we can extend the version limit to API 24 and
> above. You can already check API version through
> `mozilla::jni::GetAPIVersion()` so once we have bindings support, using it
> won't be a problem.

Does that mean we should wait for Bug 1352177 to be fixed before working on this?
Flags: needinfo?(nchen)
(In reply to Gautam Prajapati from comment #25)
> 
> - Can I know why do we want to use LocaleList.getDefault() when using
> Locale.getDefault() can get us the required locale? 
> Sorry if I missed something. :)

See comment 10.

> As :snorp and I discussed on IRC, we can annotate gecko's Locales.java with
> @WrapForJNI to access it in JNI similar to bindings(Am I correct here?). I'd
> appreciate if you can elaborate more on this if we will be doing it. :) 

Yes, this is probably the easiest approach. Adding @WrapForJNI will cause the build system to generate C++ code that you can use to call the target Java method.

For example, you can add a method in Locales.java called "foo", annotate it with @WrapForJNI:

> @WrapForJNI(calledFrom = "gecko")
> private static int foo() {
>     return 42;
> }

Now build Fennec with this change. The build will fail, but the build output will give you a command to execute to update the auto-generated bindings. After running that command, you'll be able to use the binding in C++, e.g.,

> int32_t result = mozilla::java::Locales::Foo();
> MOZ_ASSERT(result == 42);

> > Once bug 1352177 is fixed, we can extend the version limit to API 24 and
> > above. You can already check API version through
> > `mozilla::jni::GetAPIVersion()` so once we have bindings support, using it
> > won't be a problem.
> 
> Does that mean we should wait for Bug 1352177 to be fixed before working on
> this?

I think you should work on this now with the plan above, which does not require bug 1352177.
Flags: needinfo?(nchen)
Sorry for a delayed response.

(In reply to Jim Chen [:jchen] [:darchons] from comment #26)
> (In reply to Gautam Prajapati from comment #25)
> > 
> > - Can I know why do we want to use LocaleList.getDefault() when using
> > Locale.getDefault() can get us the required locale? 
> > Sorry if I missed something. :)
> 
> See comment 10.

I'm so confused right now. :(

Quoting :rnewman from comment 11:-

> So:
> - getDefault is correct at process launch.
> - It's correct after the locale picker sets the locale.
> - It remains correct after OS locale changes, because we process those configuration change notices in GeckoApplication.

We need to get the current OS locale which is correctly returned by 'Locale.getDefault()'. 

LocaleList.getDefault() is another way introduced in API 24. While trying to use it I realized I cannot use this function because Gecko/Fennec doesn't support API Level 24. 

Since Gecko doesn't support API Level 24, I don't have to check for version, be it in Java side or JNI side.  

I may be wrong, but the simplest and correct procedure to solve this issue, in my opinion, is to call  
`org.mozilla.gecko.Locales.getLanguage(Locale.getDefault())` from JNI side, which becomes very simple when I annotate the function getLanguage() with @WrapForJNI. 

Is that the correct way to go, or I missed something?
Flags: needinfo?(rnewman)
Flags: needinfo?(nchen)
(In reply to Gautam Prajapati from comment #27)

> Is that the correct way to go, or I missed something?

Yes. Do that.

Ignore LocaleList for now; we don't need it and it's not yet supported. When we start using that on the Android side, we can extend all consumers.

Ignore comment 10; it's incorrect.
Flags: needinfo?(rnewman)
Flags: needinfo?(nchen)
Comment on attachment 8860275 [details]
Bug 1337078 - Improve the use of Android API in OSPreferences;

https://reviewboard.mozilla.org/r/132296/#review135184

::: intl/locale/android/OSPreferences_android.cpp:22
(Diff revision 1)
>    //     Once we fix this (bug 1337078), this hack should not be necessary.
>    //
>    //XXX: Notice, this value may be empty on an early read. In that case
>    //     we won't add anything to the return list so that it doesn't get
>    //     cached in mSystemLocales.
> -  nsAdoptingCString locale = Preferences::GetCString("intl.locale.os");
> +  nsAdoptingCString locale = sdk::Locale::GetDefault();

[WIP] How can I typecast the result returned by GetDefault() to nsAdoptingCString? 

I went through the mozilla docs of nsAdoptingCString

* https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Glue_classes/nsAdoptingCString

I'm not that experienced with C/C++, couldn't find for the right method to do so. I'd appreciate your inputs here. 

Also, GetDefault() will return us the Locale. Where do we need the language from returned Locale?

I mean where do I need to call getLanguage() of Gecko's Locales class. 
The code right now just want me to return the Locale.
Attachment #8860275 - Flags: review?(rnewman) → review?(nchen)
Comment on attachment 8860275 [details]
Bug 1337078 - Improve the use of Android API in OSPreferences;

https://reviewboard.mozilla.org/r/132296/#review135324

You didn't include `widget/android/bindings/Locale-classes.txt` in your patch.

::: intl/locale/android/OSPreferences_android.cpp:22
(Diff revision 1)
>    //     Once we fix this (bug 1337078), this hack should not be necessary.
>    //
>    //XXX: Notice, this value may be empty on an early read. In that case
>    //     we won't add anything to the return list so that it doesn't get
>    //     cached in mSystemLocales.
> -  nsAdoptingCString locale = Preferences::GetCString("intl.locale.os");
> +  nsAdoptingCString locale = sdk::Locale::GetDefault();

You should use `GetDefault` like this,

    // namespace jni = mozilla::jni;
    // namespace java = mozilla::java;
    
    // Call Locale.getDefault to get the default locale.
    jni::Object::LocalRef locale;
    NS_ENSURE_SUCCESS(java::sdk::Locale::GetDefault(&locale), false);
    
    // Call Locales.getLanguage to get the language string from locale.
    nsCString lang = java::Locales::GetLanguage(locale)->ToCString();
    
    // lang is now the language string.
Comment on attachment 8860275 [details]
Bug 1337078 - Improve the use of Android API in OSPreferences;

https://reviewboard.mozilla.org/r/132296/#review135476

::: intl/locale/android/OSPreferences_android.cpp:10
(Diff revisions 1 - 2)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "OSPreferences.h"
>  #include "mozilla/Preferences.h"
>  
> +using namespace mozilla::jni;

Thanks for reviewing jchen, I tried as you said
For some reason build fails at this line saying that jni or java is not a namespace.

I tried adding the namespace in different ways like:

using namespace mozilla;
using namespace mozilla::jni;
using namespace mozilla::java;

or by referring to some existing cpp files this:

using namespace mozilla;
using mozilla::jni;
using mozilla::java;

or Defining it inline like you suggested:-

namespace jni = mozilla::jni;
namespace java = mozilla::java;
(Included 'using namespace mozilla::jni' above as well as you have suggested in your blog)

But it didn't work in any way. I thought it was problem with my system only, so tried complete building the default branch, then adding namespaces one by one which took a lot of time. 
I think I'm missing something.
Comment on attachment 8860275 [details]
Bug 1337078 - Improve the use of Android API in OSPreferences;

https://reviewboard.mozilla.org/r/132296/#review135944

Looks good! Thanks!

::: intl/locale/android/OSPreferences_android.cpp:18
(Diff revision 2)
>  
>  bool
>  OSPreferences::ReadSystemLocales(nsTArray<nsCString>& aLocaleList)
>  {
> -  //XXX: This is a quite sizable hack to work around the fact that we cannot
> -  //     retrieve OS locale in C++ without reaching out to JNI.
> +  // Call Locale.getDefault to get the default locale
> +  mozilla::jni::Object::LocalRef locale;

Don't need `mozilla::jni::`

::: intl/locale/android/OSPreferences_android.cpp:19
(Diff revision 2)
>  bool
>  OSPreferences::ReadSystemLocales(nsTArray<nsCString>& aLocaleList)
>  {
> -  //XXX: This is a quite sizable hack to work around the fact that we cannot
> -  //     retrieve OS locale in C++ without reaching out to JNI.
> -  //     Once we fix this (bug 1337078), this hack should not be necessary.
> +  // Call Locale.getDefault to get the default locale
> +  mozilla::jni::Object::LocalRef locale;
> +  NS_ENSURE_SUCCESS(mozilla::java::sdk::Locale::GetDefault(&locale), false);

Don't need the `mozilla::java::` part

::: intl/locale/android/OSPreferences_android.cpp:22
(Diff revision 2)
> -  //XXX: This is a quite sizable hack to work around the fact that we cannot
> -  //     retrieve OS locale in C++ without reaching out to JNI.
> -  //     Once we fix this (bug 1337078), this hack should not be necessary.
> -  //
> -  //XXX: Notice, this value may be empty on an early read. In that case
> -  //     we won't add anything to the return list so that it doesn't get
> +  // Call Locale.getDefault to get the default locale
> +  mozilla::jni::Object::LocalRef locale;
> +  NS_ENSURE_SUCCESS(mozilla::java::sdk::Locale::GetDefault(&locale), false);
> +
> +  // Call Locales.getLanguage to get the language string from locale.
> +  nsCString language = mozilla::java::Locales::GetLanguage(locale)->ToCString();

Don't need `mozilla::java::`

::: intl/locale/android/OSPreferences_android.cpp:31
(Diff revision 2)
>      return true;
>    }
>    return false;
>  }
>  
> -bool
> +bool 

Extra space at the end.
Attachment #8860275 - Flags: review?(nchen) → review+
Comment on attachment 8860275 [details]
Bug 1337078 - Improve the use of Android API in OSPreferences;

https://reviewboard.mozilla.org/r/132296/#review135476

> Thanks for reviewing jchen, I tried as you said
> For some reason build fails at this line saying that jni or java is not a namespace.
> 
> I tried adding the namespace in different ways like:
> 
> using namespace mozilla;
> using namespace mozilla::jni;
> using namespace mozilla::java;
> 
> or by referring to some existing cpp files this:
> 
> using namespace mozilla;
> using mozilla::jni;
> using mozilla::java;
> 
> or Defining it inline like you suggested:-
> 
> namespace jni = mozilla::jni;
> namespace java = mozilla::java;
> (Included 'using namespace mozilla::jni' above as well as you have suggested in your blog)
> 
> But it didn't work in any way. I thought it was problem with my system only, so tried complete building the default branch, then adding namespaces one by one which took a lot of time. 
> I think I'm missing something.

Ah you have to add `#include "GeneratedJNIWrappers.h"` at the top of the file.
> Ah you have to add `#include "GeneratedJNIWrappers.h"` at the top of the file.

Oh, that was one really silly mistake. I have updated the patch, please review. 

One more thing, Quoting Zibi from comment 4:

> The second part would be to find the APIs in Android to read the current user preferences wrt. date/time formatting - things  like hour12/hour24 time format, or date format ("YYYY-mm-dd" vs "mm/dd/yyyy" etc.).
> I don't know if Android exposes that, maybe it doesn't, but it's worth investigating.

This bug also needs changes to know current user preferences wrt date/time formatting. Should I start investigating on that part as well?

Added NI for Zibi if he can say more about the second part now, as first part is almost resolved.
Flags: needinfo?(gandalf)
Comment on attachment 8860275 [details]
Bug 1337078 - Improve the use of Android API in OSPreferences;

https://reviewboard.mozilla.org/r/132296/#review136340

::: intl/locale/android/OSPreferences_android.cpp:31
(Diff revision 3)
> -    aLocaleList.AppendElement(locale);
> +
> +  if (!language.IsEmpty()) {
> +    aLocaleList.AppendElement(language);
>      return true;
>    }
> +  

Remove the empty spaces.

::: intl/locale/android/OSPreferences_android.cpp:35
(Diff revision 3)
>    }
> +  
>    return false;
>  }
>  
> -bool
> +bool 

Remove the space at the end.
> This bug also needs changes to know current user preferences wrt date/time formatting. Should I start investigating on that part as well?

This can be a separate bug.

But in order to land this and replace the current approach we need to also be able to react to language change in Java (we currently does to intl.locale.os pref).

Does it seem reasonable to add here or should it be a separate patch?
Flags: needinfo?(gandalf)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #40)
> But in order to land this and replace the current approach we need to also
> be able to react to language change in Java (we currently does to
> intl.locale.os pref).
> 
> Does it seem reasonable to add here or should it be a separate patch?

Zibi: Referring to comment 11,  I think, we are already listening to language changes on Java side.

Quoting rnewman from comment 11:

> So:
> - getDefault is correct at process launch.
> - It's correct after the locale picker sets the locale.
> - It remains correct after OS locale changes, because we process those configuration change notices in GeckoApplication.
I'll test this patch on Android and will get back to you :)
Flags: needinfo?(gandalf)
Hi Gautam,

I tried to build Fennec with your patch and the build errored with:

https://pastebin.mozilla.org/9021113

How should I build it?
Flags: needinfo?(gautamprajapati06)
Hello Zibi, 

Sorry for the delayed response. 

With my limited knowledge about the JNI wrapper bindings, I guess it is not able to find the GetDefault() function in Generated JNI wrappers after adding the Locale-classes.txt file.  

You can try one thing, without any changes, first add these two files from the patch 

widget/android/bindings/Locale-classes.txt	
widget/android/bindings/moz.build

Build the source. It should be successful and you should be able to see the Locale class functions in files

widget/android/GeneratedJNIWrappers.h	
widget/android/GeneratedJNIWrappers.cpp 

Same changes as what you see in my patch for these two files.

If that doesn't work, I'm afraid :jchen will be the best person to ask for help.  
Also, I guess my last updated patch in pending for review. 
It would be better to try it after it has been reviewed as working.
Flags: needinfo?(gautamprajapati06)
Try changing this line in the patch

> jni::Object::LocalRef locale;

to

> java::sdk::Locale::LocalRef locale;
I see this error:

 0:02.90 *** Error parsing config file: /home/zbraniecki/projects/mozilla-unified/widget/android/bindings/Locale-classes.txt
 0:02.90 *** org.mozilla.gecko.annotationProcessors.SDKProcessor$ParseException: Missing class: java.util.Locale


Do you have a successful try run for android with this patch?
Flags: needinfo?(gandalf) → needinfo?(gautamprajapati06)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #46)
> I see this error:
> 
>  0:02.90 *** Error parsing config file:
> /home/zbraniecki/projects/mozilla-unified/widget/android/bindings/Locale-
> classes.txt
>  0:02.90 ***
> org.mozilla.gecko.annotationProcessors.SDKProcessor$ParseException: Missing
> class: java.util.Locale
> 
> 
> Do you have a successful try run for android with this patch?

After changing the scope of locale variable as suggested by jChen in comment 45, my build was successful, but I'm not sure how to try this patch on Android.

I guess I did ask about this on IRC a month back or so... that how do I try OSPreferences API on Android but didn't found much to work on.

If you could explain the process to me, it would be great! :)
Flags: needinfo?(gautamprajapati06)
Inside the ReviewBoard - https://reviewboard.mozilla.org/r/132296/diff/4/ you should see "Automation" tab at the top and there you should see "Trigger a try build".

From there you can select android build and no tests just to see if the build is successful.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #48)
> Inside the ReviewBoard - https://reviewboard.mozilla.org/r/132296/diff/4/
> you should see "Automation" tab at the top and there you should see "Trigger
> a try build".
> 
> From there you can select android build and no tests just to see if the
> build is successful.

I have level 1 commit access but it's not sufficient for triggering a try build. I cannot trigger a try build. The button under Automation is disabled.

Can you please do that for me, on the latest patch?
(In reply to Gautam Prajapati from comment #50)
> (In reply to Zibi Braniecki [:gandalf][:zibi] from comment #48)
> > Inside the ReviewBoard - https://reviewboard.mozilla.org/r/132296/diff/4/
> > you should see "Automation" tab at the top and there you should see "Trigger
> > a try build".
> > 
> > From there you can select android build and no tests just to see if the
> > build is successful.
> 
> I have level 1 commit access but it's not sufficient for triggering a try
> build. I cannot trigger a try build. The button under Automation is disabled.
> 
> Can you please do that for me, on the latest patch?

I triggered android-api-15 and android-api-15-gradle.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #52)
> Seems like the patch breaks the build -
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=d5280c1332d5&selectedJob=98742887

Indeed.  I think that's cuz we're crawling android.jar, which probably contains all the android.* namespace but probably doesn't have all of the vanilla java.* namespace.  You might be the first java.* consumer. 

jchen: following https://bugzilla.mozilla.org/show_bug.cgi?id=1337078#c45, do you have a suggestion?
Flags: needinfo?(nchen)
(In reply to Nick Alexander :nalexander from comment #53)
> (In reply to Zibi Braniecki [:gandalf][:zibi] from comment #52)
> > Seems like the patch breaks the build -
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=d5280c1332d5&selectedJob=98742887
> 
> Indeed.  I think that's cuz we're crawling android.jar, which probably
> contains all the android.* namespace but probably doesn't have all of the
> vanilla java.* namespace.  You might be the first java.* consumer. 
> 
> jchen: following https://bugzilla.mozilla.org/show_bug.cgi?id=1337078#c45,
> do you have a suggestion?

It's strange, after I changed the locale namespace in my previous patch, the build was successfully completed locally. 

> java::sdk::Locale::LocalRef locale; 

After the fail on try server, I tried building it locally again, after clobbering. It failed with the same error as on try server.

This is the error:

 1:24.20 *** Error parsing config file: /home/brainbreaker/Firefox/src/mozilla-central/widget/android/bindings/Locale-classes.txt
 1:24.20 *** org.mozilla.gecko.annotationProcessors.SDKProcessor$ParseException: Missing class: java.util.Locale
 1:24.20 ***
 1:24.20 Makefile:34: recipe for target 'Locale.h' failed
The format for `Locale-classes.txt` changed recently. See [1] for a current example.

If you want the same behavior as before, the new `Locale-classes.txt` should be,

> [java.util.Locale = exceptionMode:nsresult]

[1] https://dxr.mozilla.org/mozilla-central/rev/8a7d0b15595f9916123848ca906f29c62d4914c9/widget/android/bindings/ViewConfiguration-classes.txt
Flags: needinfo?(nchen)
(In reply to Gautam Prajapati from comment #56)
> Comment on attachment 8860275 [details]
> Bug 1337078 - Improve the use of Android API in OSPreferences;
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/132296/diff/5-6/

I'm able to build successfully with this patch. Just changed the format for Locale-classes.txt. 

Can someone push it to the try server? :)
Jim, seems like there are xpcshell test failures with crashes in mozilla::jni::GetEnvForThread. Can you take a look?
Flags: needinfo?(nchen)
Ah right. xpcshell tests don't have access to Java, so any JNI code that can run under xpcshell should be wrapped in `mozilla::jni::IsAvailable` checks (e.g. [1]). One way to do that is to put,

> if (!jni::IsAvailable()) {
>   return false;
> }

at the start of `OSPreferences::ReadSystemLocales`.

[1] http://searchfox.org/mozilla-central/rev/24c443a440104dabd9447608bd41b8766e8fc2f5/mobile/android/components/build/nsAndroidHistory.cpp#79
Flags: needinfo?(nchen)
Setting a needinfo for me:

<gandalf> when you're at it, can you evaluate https://bugzilla.mozilla.org/show_bug.cgi?id=1337078 as a replacement of our use of intl.locale.os to get data from Java into LocaleService?
<gandalf> I was hoping to land it when 58 opens
<gandalf> in order to reduce the reliance on prefs for transferring data and also because I believe we should be receiving locale list from OS, not a single locale
Flags: needinfo?(rnewman)
See Also: → 1398209
My expectations for this feature:

1) Get rid of intl.locale.os for communication between Java and Gecko about Android OS locales
2) Java should call OSPreferences::ReadSystemLocales or equivalent to provide it with the current Android OS locale set
3) Java should react to OS locale change by flushing OSPreferences mSystemLocales
4) This should be a list of locales (as supported by the new Android), rather than a single locale

I don't know how far this patch is from the goal, and I'll let Richard evaluate the design decisions around it. Just wanted to express my position and what I believe would naturally hook well into LocaleService model.
Comment on attachment 8860275 [details]
Bug 1337078 - Improve the use of Android API in OSPreferences;

https://reviewboard.mozilla.org/r/132296/#review184002

This means we can:

- Remove the work done in Bug 1397925.
- Kill `PREF_OS_LOCALE` and `ANDROID_OS_LOCALE_PREF` usage in `/intl`.
- File a follow-up bug for a couple of releases later to stop us _writing_ that pref in `browser.js`, and remove its uses in `/mobile`.

There might be some follow-up work here before this lands. I think Bug 1352343 has to happen at the same time, so we don't regress Bug 1378501. And we must guarantee that this `OSPreferences` API isn't called before the Android-side Fennec locale manager is initialized — otherwise we might temporarily return the OS locale before the user's locale choice has been applied.

Then there's a follow-up bug to return a locale list, which presumably would be the concatenation of:

- The user's chosen in-app locale, if they have one.
- The user's OS locale choices, either the solitary default OS locale or the modern Android locale list itself.

This will best be achieved by adding an API to `BrowserLocaleManager`.

::: intl/locale/android/OSPreferences_android.cpp:23
(Diff revision 7)
> +bool OSPreferences::ReadSystemLocales(nsTArray<nsCString>& aLocaleList)
>  {
> -  //XXX: This is a quite sizable hack to work around the fact that we cannot
> -  //     retrieve OS locale in C++ without reaching out to JNI.
> -  //     Once we fix this (bug 1337078), this hack should not be necessary.
> -  //
> +  namespace jni = mozilla::jni;
> +  namespace java = mozilla::java;
> +
> +  // Call Locale.getDefault to get the default locale

Nit: sentences end with a period.

Expand the comment:

```
// Fennec's in-app locale picker will set the default locale
// in response to a user's selection, so `Locale.getDefault()` will always
// be correct.
```
> Then there's a follow-up bug to return a locale list, which presumably would be the concatenation of:
> 
> - The user's chosen in-app locale, if they have one.
> - The user's OS locale choices, either the solitary default OS locale or the modern Android locale list itself.
>
> This will best be achieved by adding an API to `BrowserLocaleManager`.

OSPreferences and LocaleService are two separate things.

OSPreferences deals only with OS locales. In Android case, it should just handle the OS locale choices.

I'd expect BrowserLocaleManager to set `general.useragent.locale` and `intl.locale.matchOS` properly (which it does), and then LocaleService will look into OSPreferences if `intl.locale.matchOS` is true.

So all I need in this bug is for the Java code to properly maintain the OS locales in OSPreferences, so no concatenation necessary.

The user chosen in-app locale is currently taken from `general.useragent.locale` by LocaleService and until we move that away from the pref, that's ok to stay :)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #65)

> OSPreferences deals only with OS locales. In Android case, it should just
> handle the OS locale choices.

If you want the OS locale here, then I'm afraid this patch is wrong, and the documentation for OSPreferences is unclear:

   * Returns a list of locales used by the host environment for UI
   * localization.

To me that sounds like "which locale should I use for strings?", not "if the user chose to use Firefox in French on an en-US Android, this API call will return en-US". Best to clarify, if OSPreferences is really meant to ignore settings elsewhere in the environment.


> So all I need in this bug is for the Java code to properly maintain the OS
> locales in OSPreferences, so no concatenation necessary.

OK, then it should get a properly initialized BrowserLocaleManager via `getInstance()`, and fetch the (currently private) `systemLocale` field, rather than hitting `Locale.getDefault`. We can wrap `systemLocale` in an accessor that returns the langtag, too.
Flags: needinfo?(rnewman)
> To me that sounds like "which locale should I use for strings?", not "if the user chose to use Firefox in French on an en-US Android, this API call will return en-US". Best to clarify, if OSPreferences is really meant to ignore settings elsewhere in the environment.

Good point! I agree. Will the docs! :)

Can you help me find someone willing to take this bug in the 58 cycle then?
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #67)

> Can you help me find someone willing to take this bug in the 58 cycle then?

That is probably a question for snorp. I can certainly mentor.
Flags: needinfo?(snorp)
Something along the lines of this? I don't know exactly what I should be looking for in testing this, but doing some logging in OSPreferences::ReadSystemLocales seems to suggest this works.
Assignee: nobody → droeh
Flags: needinfo?(droeh)
Attachment #8915227 - Flags: review?(snorp)
Comment on attachment 8915227 [details] [diff] [review]
Add JNI getLocale and use it in OSPreferences

Review of attachment 8915227 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java
@@ +1813,5 @@
>          return GeckoProcessManager.getInstance().start(type, args, crashFd, ipcFd);
>      }
> +
> +    @WrapForJNI
> +    private static String getLocale() {

Let's just add this to the locale manager itself, we don't need to put more stuff into GeckoAppShell.
Attachment #8915227 - Flags: review?(snorp) → review-
Does this also trigger an update of OSPreferences::mSystemLocales and corresponding "intl:system-locales-changed" event on Android OS locales change?

And shouldn't it pull a list of locales, rather than a single locale?
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #72)

> And shouldn't it pull a list of locales, rather than a single locale?

If you mean from Java: not yet; that's API 24+.

If you mean for access from Gecko… well, I'm not sure it matters which side wraps the single value in a list.
> If you mean from Java: not yet; that's API 24+.

I'm not familiar with Java, but is there a way for us to do what we do with GTK and WinAPI - if/else depending on API version? Get the locale list on newer, and a single locale on older?
Updated per snorp's feedback.
Attachment #8915227 - Attachment is obsolete: true
Attachment #8915582 - Flags: review?(snorp)
Comment on attachment 8915582 [details] [diff] [review]
Add JNI getLocale and use it in OSPreferences v1.1

Review of attachment 8915582 [details] [diff] [review]:
-----------------------------------------------------------------

::: intl/locale/android/OSPreferences_android.cpp
@@ +20,4 @@
>    //XXX: Notice, this value may be empty on an early read. In that case
>    //     we won't add anything to the return list so that it doesn't get
>    //     cached in mSystemLocales.
> +  auto locale = java::BrowserLocaleManager::GetCurrentLocaleString();

Sorry, I should've caught this before, but I think this will blow up in GeckoView because we don't have BrowserLocaleManager. I think you should have a fallback that just gets the system locale via java.util.Locale.getDefault() in that case. You'll want to change the exception mode for the BrowserLocaleManager call to 'nsresult' so it doesn't just abort when it fails.
Attachment #8915582 - Flags: review?(snorp) → review-
> I'm not familiar with Java, but is there a way for us to do what we do with
> GTK and WinAPI - if/else depending on API version? Get the locale list on
> newer, and a single locale on older?

Not yet; we target SDK 23, so we can't refer to the new API in the code we compile. It'll be 26 soon, and then we can do what you describe.


(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #76)

> Sorry, I should've caught this before, but I think this will blow up in
> GeckoView because we don't have BrowserLocaleManager.

Worth a TODO for a GeckoView extension point, no?
No longer blocks: 1352343
Depends on: 1352343
Updated per conversation with snorp and gandalf last week. snorp, I didn't follow all of your suggestions because I'm not sure I agree with some; let's talk on Monday before you review this.
Attachment #8915582 - Attachment is obsolete: true
Attachment #8918387 - Flags: review?(snorp)
Some updates based on gandalf's feedback, otherwise same as above.
Attachment #8918387 - Attachment is obsolete: true
Attachment #8918387 - Flags: review?(snorp)
Attachment #8918410 - Flags: review?(snorp)
Updated to return the system default locale from GeckoAppShell.getLocale when appropriate and to fix some JNI stuff, should be good to go now.
Attachment #8918410 - Attachment is obsolete: true
Attachment #8918410 - Flags: review?(snorp)
Attachment #8920673 - Flags: review?(snorp)
Comment on attachment 8920673 [details] [diff] [review]
Add JNI getLocale and use it in OSPreferences v3.0

Review of attachment 8920673 [details] [diff] [review]:
-----------------------------------------------------------------

Looks ok if you remove the unused code

::: widget/android/nsAppShell.cpp
@@ +378,5 @@
>                  aName->ToString(), aTopic->ToCString().get(),
>                  aCookie->ToString().get());
>      }
> +
> +    static void RefreshLocales()

This doesn't seem to be used anywhere?
Attachment #8920673 - Flags: review?(snorp) → review+
Comment on attachment 8920673 [details] [diff] [review]
Add JNI getLocale and use it in OSPreferences v3.0

Jim, can you take a look at this patch when you get a chance? I'm getting UnsatisfiedLinkErrors on GeckoAppShell.refreshLocales(), and I can't figure out why.
Attachment #8920673 - Flags: feedback?(nchen)
(In reply to Dylan Roeh (:droeh) from comment #82)
> Comment on attachment 8920673 [details] [diff] [review]
> Add JNI getLocale and use it in OSPreferences v3.0
> 
> Jim, can you take a look at this patch when you get a chance? I'm getting
> UnsatisfiedLinkErrors on GeckoAppShell.refreshLocales(), and I can't figure
> out why.

Usually this means the library (.so) providing the symbol hasn't been loaded yet; this is easy to do, because BrowserLocaleManager is "super funky" and is an intersection between background services (which might not have loaded libmozglue.so yet) and the browser.  Try to make sure that mozglue libs have been loaded... and figure out what to do when they definitely have *not* been loaded, e.g., when Sync calls these functions.
(In reply to Dylan Roeh (:droeh) from comment #82)
> Comment on attachment 8920673 [details] [diff] [review]
> Add JNI getLocale and use it in OSPreferences v3.0
> 
> Jim, can you take a look at this patch when you get a chance? I'm getting
> UnsatisfiedLinkErrors on GeckoAppShell.refreshLocales(), and I can't figure
> out why.

You need to queue the call when Gecko is not loaded yet, e.g. [1]. Also, `refreshLocales()` should probably go inside BrowserLocaleManager itself rather than GeckoAppShell.

[1] http://searchfox.org/mozilla-central/rev/1c4901d060e3953e41088c038b62a0c026c1e1fb/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoNetworkManager.java#337
Attachment #8920673 - Flags: feedback?(nchen) → feedback+
Thanks for the advice, but unfortunately it doesn't appear that using GeckoThread.queueNativeCall alone is sufficient: https://treeherder.mozilla.org/logviewer.html#?job_id=141078712&repo=try&lineNumber=5831

We're still hitting an UnsatisfiedLinkError when the queued call is made.
Now that you moved it to BrowserLocaleManager, you need to initialize the native bits separately. e.g. [1]

[1] http://searchfox.org/mozilla-central/rev/1c4901d060e3953e41088c038b62a0c026c1e1fb/widget/android/nsAppShell.cpp#413
Oh, right! Thanks for your help.
Fixed the try failures I was running into as well as a GV crash and reorganized code a bit. Should be good to go now, I think.
Attachment #8920673 - Attachment is obsolete: true
Attachment #8927352 - Flags: review?(snorp)
Attachment #8927352 - Flags: review?(snorp) → review+
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d7012af17e5
Improve the use of Android API in OSPreferences r=snorp
https://hg.mozilla.org/mozilla-central/rev/1d7012af17e5
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
After landing this, systemLocale will be en-US even if Android locale is ja-JP :-<.
Depends on: 1420332
Depends on: 1420335
Depends on: 1425898
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: