investigate the locale setting code and implications on android

RESOLVED FIXED in Firefox 28

Status

()

Firefox for Android
General
P3
normal
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: Pike, Assigned: rnewman)

Tracking

unspecified
Firefox 28
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec-)

Details

(Reporter)

Description

6 years ago
Doug mentioned that the getting/setting locale dance was removed, but it's unclear what the fall-out is.

Doug, can you provide pointers on what exactly the code change was, so that we can try to reproduce?

Updated

6 years ago
Assignee: nobody → doug.turner
Priority: -- → P3

Comment 1

6 years ago
we removed the following from the startup path:

We want to disable this code if possible.  It is about 145ms

SharedPreferences settings = getPreferences(Activity.MODE_PRIVATE);
String localeCode = settings.getString(getPackageName() + ".locale", "");

if (localeCode != null && localeCode.length() > 0)
   GeckoAppShell.setSelectedLocale(localeCode);
(Reporter)

Comment 2

6 years ago
We're talking about http://hg.mozilla.org/users/dougt_mozilla.com/birch_native_ui_pre_wipe/rev/5b8a01f59171, right?

So, here's my take:

This is only an issue if we're trying to do explicit locale selection in a multi-locale build. In my local testing with the multi-locale builds I did post bug 702302, things seemed to work allright for the matched locales. And that tree had the commenting out already.

I assume in our planned setup with single-locale builds with an option to do multi, that should be OK.

I also speculate that the expensive call is setting the locale, as that's probably ripping out strings in the android process and paging in new ones. Not sure if getting the pref is the expensive part.

Which leads me to, was the conditional and the pref handling working on the right conditions? Maybe it'd be cheap to keep, if we make sure the pref isn't set accidently. Which might be easier, if we restrict it to explicitly set within java code, and not an observer of stuff within gecko.
Depends on: 702302
tracking-fennec: --- → 11+

Comment 3

6 years ago
i am not working on these right now.  resetting assignee.
Assignee: doug.turner → nobody
tracking-fennec: 11+ → ?
tracking-fennec: ? → -
(Reporter)

Comment 4

4 years ago
I recall that Brad added more detail:

The costly part is reading the android pref.

Which makes all other optimizations mute. I'm wondering if there'd be a point in our startup path where we read prefs anyway and the getter may be cheap. In that scenario, we could be able to make startup regressions only affect users that actually do switch locales, which might be OK.
(Assignee)

Comment 5

4 years ago
Things of note:

* That code ran in a background thread.
* And in the exact same runnable, we check for Sync migration, _write to prefs_ for our last launch time, and send some broadcasts to trigger FHR and product announcements.
* Reading a prefs file that's already been loaded, or will be loaded shortly thereafter, is essentially free.

In other words, I think there's a path forward here that doesn't impact main-thread perf.
(Assignee)

Comment 6

3 years ago
We fixed this in Bug 936756.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Depends on: 936756
Resolution: --- → FIXED
Assignee: nobody → rnewman
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.