Closed Bug 1486552 Opened 6 years ago Closed 6 years ago

Trigger locale change through GeckoRuntime

Categories

(GeckoView :: General, enhancement, P1)

enhancement

Tracking

(geckoview62+ fixed, firefox-esr52 wontfix, firefox-esr60 wontfix, firefox61 wontfix, firefox62 wontfix, firefox63 fixed, firefox64 fixed)

RESOLVED FIXED
mozilla64
Tracking Status
geckoview62 + fixed
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed
firefox64 --- fixed

People

(Reporter: ekager, Assigned: mbrubeck)

References

Details

(Whiteboard: [geckoview:fenix:p1])

Attachments

(2 files, 1 obsolete file)

In Focus, we want to be able to override the system locale and update GV to a chosen locale in Settings.
Assignee: nobody → mbrubeck
Priority: -- → P1
Whiteboard: [geckoview:klar:p1]
In addition to setting the requested locale, do you also need the ability to query the currently requested locale, and/or the list of available locales?

For reference, this is internal API that Gecko supports:

https://searchfox.org/mozilla-central/rev/2fe43133dbc774dda668d4597174d73e3969181a/intl/locale/mozILocaleService.idl
Flags: needinfo?(ekager)
Focus+WebView supports this so not supporting it in GeckoView would be a functionality regression.
I'm not sure if we need all of those. NI Sebastian for his expertise.
Flags: needinfo?(ekager) → needinfo?(s.kaspari)
(In reply to Matt Brubeck (:mbrubeck) from comment #4)
> Created attachment 9005710 [details]
> Bug 1486552 - geckoview: Add GeckoRuntime.setLocale method. r=snorp

This is a bare-bones patch that just adds a `setLocale` method based on code from Fennec.  Some questions for snorp:

1) Should there be a test for this?
2) Is it okay to rely on the "geckoview.js" script for this?  Can this fail if the event is dispatched when the script hasn't loaded yet?
3) Should we call the LocaleService through JNI instead?  It looks like the LocaleService C++ code handles cross-process synchronization internally, so we don't need to send any messages to the content process ourselves.
Flags: needinfo?(snorp)
(In reply to Matt Brubeck (:mbrubeck) from comment #5)
> (In reply to Matt Brubeck (:mbrubeck) from comment #4)
> > Created attachment 9005710 [details]
> > Bug 1486552 - geckoview: Add GeckoRuntime.setLocale method. r=snorp
> 
> This is a bare-bones patch that just adds a `setLocale` method based on code
> from Fennec.  Some questions for snorp:
> 
> 1) Should there be a test for this?

If we can test it I think we should, yes.

> 2) Is it okay to rely on the "geckoview.js" script for this?  Can this fail
> if the event is dispatched when the script hasn't loaded yet?

We queue messages if Gecko isn't up yet, so your patch is fine there.

> 3) Should we call the LocaleService through JNI instead?  It looks like the
> LocaleService C++ code handles cross-process synchronization internally, so
> we don't need to send any messages to the content process ourselves.

The medium-to-long-term plan for GeckoView is to move the Gecko *parent* process out of the app process. The communication with the remote Gecko process will likely happen over Binder, so there won't be any existing Gecko IPC stuff to take advantage of there. We can easily remote messages sent via EventDispatcher, though, so I think we prefer that unless there is a good reason not to (like a arg that you don't want to copy, for instance).
Flags: needinfo?(snorp)
BTW, we usually implement behavior in one of the "GeckoView*.jsm" modules instead of "geckoview.js" itself. For example, normally the "GeckoView:SetLocale" listener would go inside "GeckoViewContent.jsm"
(In reply to Matt Brubeck (:mbrubeck) from comment #1)
> In addition to setting the requested locale, do you also need the ability to
> query the currently requested locale, and/or the list of available locales?
> 
> For reference, this is internal API that Gecko supports:
> 
> https://searchfox.org/mozilla-central/rev/
> 2fe43133dbc774dda668d4597174d73e3969181a/intl/locale/mozILocaleService.idl


(In reply to Emily Kager [:ekager] from comment #3)
> I'm not sure if we need all of those. NI Sebastian for his expertise.


No, I do not see a use case for those methods currently.
Flags: needinfo?(s.kaspari)
(In reply to Jim Chen [:jchen] [:darchons] from comment #7)
> BTW, we usually implement behavior in one of the "GeckoView*.jsm" modules
> instead of "geckoview.js" itself. For example, normally the
> "GeckoView:SetLocale" listener would go inside "GeckoViewContent.jsm"

Whoops, I didn't even notice that. I agree, should go into GeckoViewContent.jsm.
Matt has a low-risk patch but he is still fixing some test issues. He will uplift the fix to GV 62 and then file a follow-up bug for the test issues.
Attached patch patch v2 (obsolete) — Splinter Review
Moved the listener to GeckoViewStartup, and added a working test.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=95e5481b39b3daa6ea2c1019d4f78f19b2f48e51
Attachment #9005710 - Attachment is obsolete: true
Attachment #9008837 - Flags: review?(snorp)
Attachment #9008837 - Attachment description: patch v1 → patch v2
Comment on attachment 9008837 [details] [diff] [review]
patch v2

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

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/LocaleTest.kt
@@ +19,5 @@
> +@WithDevToolsAPI
> +class LocaleTest : BaseSessionTest() {
> +
> +    @Test fun setLocale() {
> +        val runtime = sessionRule.runtime;

You don't really need this assignment

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java
@@ +238,5 @@
> +     * Change the locale used by Gecko.
> +     *
> +     * @param localeCode A locale identifier in Gecko format (e.g. "en" or "en-US")
> +     */
> +    public void setLocale(String languageTag) {

I really think this should be in GeckoRuntimeSettings if possible, but this could be ok too.

It might Just Work to move this method into GeckoRuntimeSettings directly.
Attachment #9008837 - Flags: review?(snorp) → review+
Attachment #9005710 - Attachment is obsolete: false
Attachment #9008837 - Attachment is obsolete: true
Comment on attachment 9005710 [details]
Bug 1486552 - geckoview: Add GeckoRuntimeSettings.setLocale method. r=snorp

James Willcox (:snorp) (jwillcox@mozilla.com) has approved the revision.
Attachment #9005710 - Flags: review+
Attachment #9005710 - Attachment description: Bug 1486552 - geckoview: Add GeckoRuntime.setLocale method. r=snorp → Bug 1486552 - geckoview: Add GeckoRuntimeSetting.setLocale method. r=snorp
Attachment #9005710 - Attachment description: Bug 1486552 - geckoview: Add GeckoRuntimeSetting.setLocale method. r=snorp → Bug 1486552 - geckoview: Add GeckoRuntimeSettings.setLocale method. r=snorp
Pushed by mbrubeck@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6021f554fe5b
geckoview: Add GeckoRuntimeSettings.setLocale method. r=snorp
https://hg.mozilla.org/mozilla-central/rev/6021f554fe5b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Comment on attachment 9005710 [details]
Bug 1486552 - geckoview: Add GeckoRuntimeSettings.setLocale method. r=snorp

[Approval Request Comment]

If this is not a sec:{high,crit} bug, please state case for consideration: Fixes a Focus/GeckoView blocker.

User impact if declined: Gecko locale will not match locale set by Focus.

Fix Landed on Version: 64

Risk to taking this patch (and alternatives if risky): Low-risk, GeckoView-only patch that only adds a new setting.

String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/Uplift_rules for more info.
Attachment #9005710 - Flags: approval-mozilla-geckoview62?
Attachment #9005710 - Flags: approval-mozilla-beta?
Comment on attachment 9005710 [details]
Bug 1486552 - geckoview: Add GeckoRuntimeSettings.setLocale method. r=snorp

Low risk for our beta branch as this is geckoview only, approved for 63 beta 7. Note that the approval for the geckoview branch will be dealt with by the release manager in charge of the 62 branch. Thanks.
Attachment #9005710 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Hi, I couldn't uplift your patch, I get the following error:

grafting 501127:6021f554fe5b "Bug 1486552 - geckoview: Add GeckoRuntimeSettings.setLocale method. r=snorp"
merging mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java
warning: conflicts while merging mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java!
Flags: needinfo?(mbrubeck)
Here is the patch rebased onto mozilla-beta.
Flags: needinfo?(mbrubeck)
Attachment #9009700 - Flags: checkin?(ebalazs)
Lowering Focus priority to [geckoview:klar:p2] because this bug does not block the Focus 7.0 release (because Emily landed a Focus workaround for DDG). We still want to uplift this fix to the GV 62 relbranch for Focus 7.x or 8.0.
Whiteboard: [geckoview:klar:p1] → [geckoview:klar:p2]
Tried to uplift again.
Same problem:
grafting 491546:6021f554fe5b "Bug 1486552 - geckoview: Add GeckoRuntimeSettings.setLocale method. r=snorp"
merging mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java
warning: conflicts while merging mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
Flags: needinfo?(mbrubeck)
(In reply to Chris Peterson [:cpeterson] from comment #20)
> Lowering Focus priority to [geckoview:klar:p2] because this bug does not
> block the Focus 7.0 release (because Emily landed a Focus workaround for
> DDG).

DDG is actually unrelated to this bug. Emily's DDG workaround was for bug 1487542. That said, this bug still does not need to block Focus 7.0 release.
(In reply to Bogdan Tara[:bogdan_tara] from comment #21)
> Tried to uplift again.
> Same problem:
> grafting 491546:6021f554fe5b "Bug 1486552 - geckoview: Add
> GeckoRuntimeSettings.setLocale method. r=snorp"

Grafting won't work.  You can apply the rebased patch directly to a mozilla-beta tree with this command:

hg import https://bugzilla.mozilla.org/attachment.cgi?id=9009700
Flags: needinfo?(mbrubeck)
Comment on attachment 9009700 [details] [diff] [review]
Patch for mozilla-beta 63

Clearing the checkin?
Attachment #9009700 - Flags: checkin?(ebalazs)
Comment on attachment 9005710 [details]
Bug 1486552 - geckoview: Add GeckoRuntimeSettings.setLocale method. r=snorp

Needed for Focus 7.x, GV62+
Attachment #9005710 - Flags: approval-mozilla-geckoview62? → approval-mozilla-geckoview62+
Hi Matt. I wasn't aware of this bug until now, so I apologize for a late comment, but I really would prefer to use a list over a single locale string here.

We're trying very hard to move away from a single locale model, because it's insanely painful to switch later and it completely limits our ability to negotiate locales.

In particular, Android does expose a list of locale that can be ordered by the user and we switched to it in Fennec in bug 1337078.

The API is called `LocaleList::GetDefault` and is documented here: https://developer.android.com/reference/android/os/LocaleList.html#getDefault()

Can you please switch to it? In Gecko, we're happy to take it in OSPreferences_android [0] and keep it up to date to react when user updated their preference list.


[0] https://searchfox.org/mozilla-central/source/intl/locale/android/OSPreferences_android.cpp#24
Flags: needinfo?(mbrubeck)
Flags: needinfo?(mbrubeck)
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 64 → mozilla64
Whiteboard: [geckoview:klar:p2] → [geckoview:fenix:p1]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: