Bug 1011008 - Part 1: handle system locale changes correctly. r=nalexander

mobile/android/base/BrowserApp.java

@@ -15,12 +15,9 @@ import java.util.Vector;
 import org.json.JSONArray;
 import org.json.JSONException;
 import org.json.JSONObject;
-import org.mozilla.gecko.AndroidGamepadManager;
 import org.mozilla.gecko.DynamicToolbar.PinReason;
 import org.mozilla.gecko.DynamicToolbar.VisibilityTransition;
 import org.mozilla.gecko.GeckoProfileDirectories.NoMozillaDirectoryException;
-import org.mozilla.gecko.Telemetry;
-import org.mozilla.gecko.TelemetryContract;
 import org.mozilla.gecko.animation.PropertyAnimator;
 import org.mozilla.gecko.animation.ViewHelper;
 import org.mozilla.gecko.db.BrowserContract.Combined;
@@ -43,9 +40,9 @@ import org.mozilla.gecko.health.HealthRecorder;
 import org.mozilla.gecko.health.SessionInformation;
 import org.mozilla.gecko.home.BrowserSearch;
 import org.mozilla.gecko.home.HomeBanner;
-import org.mozilla.gecko.home.HomePanelsManager;
 import org.mozilla.gecko.home.HomePager;
 import org.mozilla.gecko.home.HomePager.OnUrlOpenListener;
+import org.mozilla.gecko.home.HomePanelsManager;
 import org.mozilla.gecko.home.SearchEngine;
 import org.mozilla.gecko.menu.GeckoMenu;
 import org.mozilla.gecko.menu.GeckoMenuItem;
@@ -108,8 +105,8 @@ import android.view.ViewStub;
 import android.view.ViewTreeObserver;
 import android.view.Window;
 import android.view.animation.Interpolator;
-import android.widget.RelativeLayout;
 import android.widget.ListView;
+import android.widget.RelativeLayout;
 import android.widget.Toast;
 import android.widget.ViewFlipper;
 

mobile/android/base/BrowserLocaleManager.java

@@ -5,15 +5,6 @@
 
 package org.mozilla.gecko;
 
-import android.content.BroadcastReceiver;
-import android.content.Context;
-import android.content.Intent;
-import android.content.IntentFilter;
-import android.content.SharedPreferences;
-import android.content.res.Configuration;
-import android.content.res.Resources;
-import android.util.Log;
-
 import java.io.File;
 import java.util.Collection;
 import java.util.HashSet;
@@ -27,6 +18,15 @@ import org.json.JSONException;
 import org.json.JSONObject;
 import org.mozilla.gecko.util.GeckoJarReader;
 
+import android.content.BroadcastReceiver;
+import android.content.Context;
+import android.content.Intent;
+import android.content.IntentFilter;
+import android.content.SharedPreferences;
+import android.content.res.Configuration;
+import android.content.res.Resources;
+import android.util.Log;
+
 /**
  * This class manages persistence, application, and otherwise handling of
  * user-specified locales.
@@ -132,6 +132,8 @@ public class BrowserLocaleManager implements LocaleManager {
         receiver = new BroadcastReceiver() {
             @Override
             public void onReceive(Context context, Intent intent) {
+                final Locale current = systemLocale;
+
                 // We don't trust Locale.getDefault() here, because we make a
                 // habit of mutating it! Use the one Android supplies, because
                 // that gets regularly reset.
@@ -139,6 +141,8 @@ public class BrowserLocaleManager implements LocaleManager {
                 // yet swizzled Locale during static initialization.
                 systemLocale = context.getResources().getConfiguration().locale;
                 systemLocaleDidChange = true;
+
+                Log.d(LOG_TAG, "System locale changed from " + current + " to " + systemLocale);
             }
         };
         context.registerReceiver(receiver, new IntentFilter(Intent.ACTION_LOCALE_CHANGED));
@@ -157,6 +161,7 @@ public class BrowserLocaleManager implements LocaleManager {
     public void correctLocale(Context context, Resources res, Configuration config) {
         final Locale current = getCurrentLocale(context);
         if (current == null) {
+            Log.d(LOG_TAG, "No selected locale. No correction needed.");
             return;
         }
 
@@ -180,6 +185,48 @@ public class BrowserLocaleManager implements LocaleManager {
         res.updateConfiguration(config, null);
     }
 
+    /**
+     * We can be in one of two states.
+     *
+     * If the user has not explicitly chosen a Firefox-specific locale, we say
+     * we are "mirroring" the system locale.
+     *
+     * When we are not mirroring, system locale changes do not impact Firefox
+     * and are essentially ignored; the user's locale selection is the only
+     * thing we care about, and we actively correct incoming configuration
+     * changes to reflect the user's chosen locale.
+     *
+     * By contrast, when we are mirroring, system locale changes cause Firefox
+     * to reflect the new system locale, as if the user picked the new locale.
+     *
+     * If we're currently mirroring the system locale, this method returns the
+     * supplied configuration's locale, unless the current activity locale is
+     * correct. , If we're not currently mirroring, this methodupdates the
+     * configuration object to match the user's currently selected locale, and
+     * returns that, unless the current activity locale is correct.
+     *
+     * If the current activity locale is correct, returns null.
+     *
+     * The caller is expected to redisplay themselves accordingly.
+     *
+     * This method is intended to be called from inside
+     * <code>onConfigurationChanged(Configuration)</code> as part of a strategy
+     * to detect and either apply or undo system locale changes.
+     */
+    @Override
+    public Locale onSystemConfigurationChanged(final Context context, final Resources resources, final Configuration configuration, final Locale currentActivityLocale) {
+        if (!isMirroringSystemLocale(context)) {
+            correctLocale(context, resources, configuration);
+        }
+
+        final Locale changed = configuration.locale;
+        if (changed.equals(currentActivityLocale)) {
+            return null;
+        }
+
+        return changed;
+    }
+
     @Override
     public String getAndApplyPersistedLocale(Context context) {
         initialize(context);
@@ -323,6 +370,10 @@ public class BrowserLocaleManager implements LocaleManager {
         return locale.toString();
     }
 
+    private boolean isMirroringSystemLocale(final Context context) {
+        return getPersistedLocale(context) == null;
+    }
+
     /**
      * Examines <code>multilocale.json</code>, returning the included list of
      * locale codes.

mobile/android/base/GeckoApp.java

@@ -16,7 +16,6 @@ import java.net.URL;
 import java.text.DateFormat;
 import java.text.SimpleDateFormat;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Date;
 import java.util.HashMap;
 import java.util.Iterator;
@@ -209,6 +208,7 @@ public abstract class GeckoApp
     private String mPrivateBrowsingSession;
 
     private volatile HealthRecorder mHealthRecorder = null;
+    private volatile Locale mLastLocale = null;
 
     private int mSignalStrenth;
     private PhoneStateListener mPhoneStateListener = null;
@@ -1279,7 +1279,8 @@ public abstract class GeckoApp
 
                 // Wait until now to set this, because we'd rather throw an exception than
                 // have a caller of BrowserLocaleManager regress startup.
-                BrowserLocaleManager.getInstance().initialize(getApplicationContext());
+                final LocaleManager localeManager = BrowserLocaleManager.getInstance();
+                localeManager.initialize(getApplicationContext());
 
                 SessionInformation previousSession = SessionInformation.fromSharedPrefs(prefs);
                 if (previousSession.wasKilled()) {
@@ -1303,7 +1304,7 @@ public abstract class GeckoApp
                 Log.i(LOGTAG, "Creating HealthRecorder.");
 
                 final String osLocale = Locale.getDefault().toString();
-                String appLocale = BrowserLocaleManager.getInstance().getAndApplyPersistedLocale(GeckoApp.this);
+                String appLocale = localeManager.getAndApplyPersistedLocale(GeckoApp.this);
                 Log.d(LOGTAG, "OS locale is " + osLocale + ", app locale is " + appLocale);
 
                 if (appLocale == null) {
@@ -1351,6 +1352,11 @@ public abstract class GeckoApp
             throw new RuntimeException("onLocaleReady must always be called from the UI thread.");
         }
 
+        final Locale loc = BrowserLocaleManager.parseLocaleCode(locale);
+        if (loc.equals(mLastLocale)) {
+            Log.d(LOGTAG, "New locale same as old; onLocaleReady has nothing to do.");
+        }
+
         // The URL bar hint needs to be populated.
         TextView urlBar = (TextView) findViewById(R.id.url_bar_title);
         if (urlBar != null) {
@@ -1360,8 +1366,13 @@ public abstract class GeckoApp
             Log.d(LOGTAG, "No URL bar in GeckoApp. Not loading localized hint string.");
         }
 
+        mLastLocale = loc;
+
         // Allow onConfigurationChanged to take care of the rest.
-        onConfigurationChanged(getResources().getConfiguration());
+        // We don't call this.onConfigurationChanged, because (a) that does
+        // work that's unnecessary after this locale action, and (b) it can
+        // cause a loop! See Bug 1011008, Comment 12.
+        super.onConfigurationChanged(getResources().getConfiguration());
     }
 
     protected void initializeChrome() {
@@ -2155,7 +2166,12 @@ public abstract class GeckoApp
     @Override
     public void onConfigurationChanged(Configuration newConfig) {
         Log.d(LOGTAG, "onConfigurationChanged: " + newConfig.locale);
-        BrowserLocaleManager.getInstance().correctLocale(this, getResources(), newConfig);
+
+        final LocaleManager localeManager = BrowserLocaleManager.getInstance();
+        final Locale changed = localeManager.onSystemConfigurationChanged(this, getResources(), newConfig, mLastLocale);
+        if (changed != null) {
+            onLocaleChanged(BrowserLocaleManager.getLanguageTag(changed));
+        }
 
         // onConfigurationChanged is not called for 180 degree orientation changes,
         // we will miss such rotations and the screen orientation will not be

mobile/android/base/LocaleManager.java

@@ -10,6 +10,12 @@ import android.content.Context;
 import android.content.res.Configuration;
 import android.content.res.Resources;
 
+/**
+ * Implement this interface to provide Fennec's locale switching functionality.
+ *
+ * The LocaleManager is responsible for persisting and applying selected locales,
+ * and correcting configurations after Android has changed them.
+ */
 public interface LocaleManager {
     void initialize(Context context);
     Locale getCurrentLocale(Context context);
@@ -19,4 +25,12 @@ public interface LocaleManager {
     String setSelectedLocale(Context context, String localeCode);
     boolean systemLocaleDidChange();
     void resetToSystemLocale(Context context);
+
+    /**
+     * Call this in your onConfigurationChanged handler. This method is expected
+     * to do the appropriate thing: if the user has selected a locale, it
+     * corrects the incoming configuration; if not, it signals the new locale to
+     * use.
+     */
+    Locale onSystemConfigurationChanged(Context context, Resources resources, Configuration configuration, Locale currentActivityLocale);
 }

mobile/android/base/preferences/GeckoPreferenceFragment.java

@@ -10,12 +10,14 @@ import java.util.Locale;
 
 import org.mozilla.gecko.BrowserLocaleManager;
 import org.mozilla.gecko.GeckoSharedPrefs;
+import org.mozilla.gecko.LocaleManager;
 import org.mozilla.gecko.PrefsHelper;
 import org.mozilla.gecko.R;
 
 import android.app.ActionBar;
 import android.app.Activity;
 import android.content.Context;
+import android.content.res.Configuration;
 import android.content.res.Resources;
 import android.os.Build;
 import android.os.Bundle;
@@ -33,6 +35,20 @@ import android.view.ViewConfiguration;
 */
 public class GeckoPreferenceFragment extends PreferenceFragment {
 
+    @Override
+    public void onConfigurationChanged(Configuration newConfig) {
+        super.onConfigurationChanged(newConfig);
+        Log.d(LOGTAG, "onConfigurationChanged: " + newConfig.locale);
+
+        final Activity context = getActivity();
+
+        final LocaleManager localeManager = BrowserLocaleManager.getInstance();
+        final Locale changed = localeManager.onSystemConfigurationChanged(context, getResources(), newConfig, lastLocale);
+        if (changed != null) {
+            applyLocale(changed);
+        }
+    }
+
     private static final String LOGTAG = "GeckoPreferenceFragment";
     private int mPrefsRequestId = 0;
     private Locale lastLocale = Locale.getDefault();
@@ -112,7 +128,13 @@ public class GeckoPreferenceFragment extends PreferenceFragment {
 
     @Override
     public void onResume() {
-        final Locale currentLocale = Locale.getDefault();
+        // This is a little delicate. Ensure that you do nothing prior to
+        // super.onResume that you wouldn't do in onCreate.
+        applyLocale(Locale.getDefault());
+        super.onResume();
+    }
+
+    private void applyLocale(final Locale currentLocale) {
         final Context context = getActivity().getApplicationContext();
 
         BrowserLocaleManager.getInstance().updateConfiguration(context, currentLocale);
@@ -129,8 +151,6 @@ public class GeckoPreferenceFragment extends PreferenceFragment {
 
         // Fix the parent title regardless.
         updateTitle();
-
-        super.onResume();
     }
 
     /*

mobile/android/base/preferences/GeckoPreferences.java

@@ -45,6 +45,7 @@ import android.content.DialogInterface;
 import android.content.Intent;
 import android.content.SharedPreferences;
 import android.content.SharedPreferences.OnSharedPreferenceChangeListener;
+import android.content.res.Configuration;
 import android.os.Build;
 import android.os.Bundle;
 import android.preference.CheckBoxPreference;
@@ -866,6 +867,24 @@ OnSharedPreferenceChangeListener
         return true;
     }
 
+    @Override
+    public void onConfigurationChanged(Configuration newConfig) {
+        super.onConfigurationChanged(newConfig);
+
+        Log.d(LOGTAG, "onConfigurationChanged: " + newConfig.locale);
+
+        if (lastLocale.equals(newConfig.locale)) {
+            Log.d(LOGTAG, "Old locale same as new locale. Short-circuiting.");
+            return;
+        }
+
+        final LocaleManager localeManager = BrowserLocaleManager.getInstance();
+        final Locale changed = localeManager.onSystemConfigurationChanged(this, getResources(), newConfig, lastLocale);
+        if (changed != null) {
+            onLocaleChanged(changed);
+        }
+    }
+
     /**
      * Implementation for the {@link OnSharedPreferenceChangeListener} interface,
      * which we use to watch changes in our prefs file.