Closed
Bug 1486552
Opened 6 years ago
Closed 6 years ago
Trigger locale change through GeckoRuntime
Categories
(GeckoView :: General, enhancement, P1)
GeckoView
General
Tracking
(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)
46 bytes,
text/x-phabricator-request
|
snorp
:
review+
pascalc
:
approval-mozilla-beta+
|
Details | Review |
6.59 KB,
patch
|
Details | Diff | Splinter Review |
In Focus, we want to be able to override the system locale and update GV to a chosen locale in Settings.
Updated•6 years ago
|
Assignee: nobody → mbrubeck
status-geckoview62:
--- → affected
Priority: -- → P1
Whiteboard: [geckoview:klar:p1]
Assignee | ||
Comment 1•6 years ago
|
||
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)
Comment 2•6 years ago
|
||
Focus+WebView supports this so not supporting it in GeckoView would be a functionality regression.
status-firefox61:
--- → wontfix
status-firefox62:
--- → wontfix
status-firefox63:
--- → affected
status-firefox-esr52:
--- → wontfix
status-firefox-esr60:
--- → wontfix
Reporter | ||
Comment 3•6 years ago
|
||
I'm not sure if we need all of those. NI Sebastian for his expertise.
Flags: needinfo?(ekager) → needinfo?(s.kaspari)
Assignee | ||
Comment 4•6 years ago
|
||
Assignee | ||
Comment 5•6 years ago
|
||
(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)
Comment 7•6 years ago
|
||
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"
Comment 8•6 years ago
|
||
(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.
Updated•6 years ago
|
Comment 10•6 years ago
|
||
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.
status-firefox64:
--- → affected
Assignee | ||
Comment 11•6 years ago
|
||
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)
Assignee | ||
Updated•6 years ago
|
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+
Assignee | ||
Updated•6 years ago
|
Attachment #9005710 -
Attachment is obsolete: false
Assignee | ||
Updated•6 years ago
|
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+
Updated•6 years ago
|
Attachment #9005710 -
Attachment description: Bug 1486552 - geckoview: Add GeckoRuntime.setLocale method. r=snorp → Bug 1486552 - geckoview: Add GeckoRuntimeSetting.setLocale method. r=snorp
Updated•6 years ago
|
Attachment #9005710 -
Attachment description: Bug 1486552 - geckoview: Add GeckoRuntimeSetting.setLocale method. r=snorp → Bug 1486552 - geckoview: Add GeckoRuntimeSettings.setLocale method. r=snorp
Comment 14•6 years ago
|
||
Pushed by mbrubeck@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6021f554fe5b geckoview: Add GeckoRuntimeSettings.setLocale method. r=snorp
Comment 15•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6021f554fe5b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Assignee | ||
Comment 16•6 years ago
|
||
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 17•6 years ago
|
||
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+
Comment 18•6 years ago
|
||
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)
Assignee | ||
Comment 19•6 years ago
|
||
Here is the patch rebased onto mozilla-beta.
Flags: needinfo?(mbrubeck)
Attachment #9009700 -
Flags: checkin?(ebalazs)
Comment 20•6 years ago
|
||
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]
Comment 21•6 years ago
|
||
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)
Comment 22•6 years ago
|
||
(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.
Assignee | ||
Comment 23•6 years ago
|
||
(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 24•6 years ago
|
||
I landed this on beta: https://hg.mozilla.org/releases/mozilla-beta/rev/96ac13491c0250233edfc613e22a6f0cb271c5fb
Comment 25•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/96ac13491c02
Comment 26•6 years ago
|
||
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+
Comment 28•6 years ago
|
||
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)
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(mbrubeck)
Comment 29•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-release/rev/691b21e74a51
tracking-geckoview62:
--- → +
Updated•5 years ago
|
Product: Firefox for Android → GeckoView
Updated•5 years ago
|
Target Milestone: Firefox 64 → mozilla64
Updated•5 years ago
|
Whiteboard: [geckoview:klar:p2] → [geckoview:fenix:p1]
You need to log in
before you can comment on or make changes to this bug.
Description
•