Bug 1460874 - Part 12: Enforce sensible API usage for manual font size settings. r=snorp
☠☠ backed out by cd616c3e3eea ☠ ☠
authorJan Henning <jh+bugzilla@buttercookie.de>
Wed, 13 Feb 2019 20:11:22 +0000
changeset 458954 593a2316ac28
parent 458953 3d4297e781cb
child 458955 e249a6801e6e
push id35552
push usershindli@mozilla.com
push dateThu, 14 Feb 2019 04:39:44 +0000
treeherdermozilla-central@c6829642e2d0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssnorp
bugs1460874
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1460874 - Part 12: Enforce sensible API usage for manual font size settings. r=snorp Modifying the manual font size settings while the GeckoFontScaleListener is active is theoretically possible, but probably not the most sensible way of using that API. Therefore, we prohibit it and throw an exception in that case. There is one complication, though: The very same API is used by the font scale listener itself in order to modify the font size settings according to the system font scale. Therefore, we have to move the GeckoFontScaleListener into the GeckoView package itself, so that we can provide a package-private internal API that bypasses the above usage checks. This means that going forward, Fennec needs to use the official GeckoView API to communicate with the font scale listener, too. As we've moved out the Shared- Preferences watching in part 5, this doesn't pose any insurmountable difficulties. Because for a short while I encountered some strange crashes where getRuntime() in GeckoApplication apparently returned null while trying to initialise the listener, I'm tying its initialisation to creation of the runtime, just to be on the safe side. Differential Revision: https://phabricator.services.mozilla.com/D18507
mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java
mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoFontScaleListener.java
mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoFontScaleListener.java
mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java
mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java
--- a/mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java
+++ b/mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java
@@ -210,17 +210,16 @@ public class GeckoApplication extends Ap
         });
 
         GeckoNetworkManager.getInstance().stop();
     }
 
     public void onApplicationForeground() {
         if (mIsInitialResume) {
             GeckoBatteryManager.getInstance().start(this);
-            initFontScaleListener();
             GeckoNetworkManager.getInstance().start(this);
             mIsInitialResume = false;
         } else if (mPausedGecko) {
             GeckoThread.onResume();
             mPausedGecko = false;
             GeckoNetworkManager.getInstance().start(this);
         }
 
@@ -282,27 +281,34 @@ public class GeckoApplication extends Ap
 
             Bundle extras = intent.getExtras();
             if (extras != null) {
                 builder.extras(extras);
             }
         }
 
         sGeckoRuntime = GeckoRuntime.create(context, builder.build());
+        ((GeckoApplication) GeckoAppShell.getApplicationContext()).initFontScaleListener();
         return sGeckoRuntime;
     }
 
     @Override
     public void onCreate() {
         Log.i(LOG_TAG, "zerdatime " + SystemClock.elapsedRealtime() +
               " - application start");
 
         final Context oldContext = GeckoAppShell.getApplicationContext();
         if (oldContext instanceof GeckoApplication) {
             ((GeckoApplication) oldContext).onDestroy();
+            if (sGeckoRuntime != null) {
+                // The listener is registered when the runtime gets created, so if we already have
+                // a runtime, we need to transfer the listener registration to the new instance.
+                // The old listener will be unregistered through onDestroy().
+                GeckoSharedPrefs.forApp(this).registerOnSharedPreferenceChangeListener(this);
+            }
         }
 
         final Context context = getApplicationContext();
         if (AppConstants.MOZ_CRASHREPORTER) {
             GeckoAppShell.ensureCrashHandling(getCrashHandlerServiceClass());
         }
         GeckoAppShell.setApplicationContext(context);
 
@@ -919,12 +925,12 @@ public class GeckoApplication extends Ap
             currentActivity.getWindow().getDecorView().performHapticFeedback(effect);
         }
     }
 
     @Override // OnSharedPreferenceChangeListener
     public void onSharedPreferenceChanged(SharedPreferences prefs, String key) {
         if (GeckoPreferences.PREFS_SYSTEM_FONT_SIZE.equals(key)) {
             final boolean enabled = prefs.getBoolean(GeckoPreferences.PREFS_SYSTEM_FONT_SIZE, false);
-            GeckoFontScaleListener.getInstance().setEnabled(enabled);
+            getRuntime().getSettings().setAutomaticFontSizeAdjustment(enabled);
         }
     }
 }
rename from mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoFontScaleListener.java
rename to mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoFontScaleListener.java
--- a/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoFontScaleListener.java
+++ b/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoFontScaleListener.java
@@ -1,14 +1,14 @@
 /* -*- Mode: Java; c-basic-offset: 4; tab-width: 4; indent-tabs-mode: nil; -*-
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-package org.mozilla.gecko;
+package org.mozilla.geckoview;
 
 import org.mozilla.gecko.util.ThreadUtils;
 import org.mozilla.geckoview.GeckoRuntimeSettings;
 
 import android.annotation.SuppressLint;
 import android.content.ContentResolver;
 import android.content.Context;
 import android.database.ContentObserver;
@@ -18,17 +18,17 @@ import android.support.annotation.UiThre
 import android.util.Log;
 
 /**
  * A class that automatically adjusts font size and font inflation settings for web content in Gecko
  * in accordance with the device's OS font scale setting.
  *
  * @see android.provider.Settings.System#FONT_SCALE
  */
-public final class GeckoFontScaleListener
+/* package */ final class GeckoFontScaleListener
         extends ContentObserver {
     private static final String LOGTAG = "GeckoFontScaleListener";
 
     // We're referencing the *application* context, so this is in fact okay.
     @SuppressLint("StaticFieldLeak")
     private static final GeckoFontScaleListener sInstance = new GeckoFontScaleListener();
 
     private Context mApplicationContext;
@@ -157,18 +157,18 @@ public final class GeckoFontScaleListene
         if (!stopping) { // Either we were enabled, or else the system font scale changed.
             fontScale = Settings.System.getFloat(contentResolver, Settings.System.FONT_SCALE, mPrevGeckoFontScale);
             fontInflationEnabled = true;
         } else { // We were turned off.
             fontScale = mPrevGeckoFontScale;
             fontInflationEnabled = mPrevFontInflationState;
         }
 
-        mSettings.setFontInflationEnabled(fontInflationEnabled);
-        mSettings.setFontSizeFactor(fontScale);
+        mSettings.setFontInflationEnabledInternal(fontInflationEnabled);
+        mSettings.setFontSizeFactorInternal(fontScale);
     }
 
     @UiThread // See constructor.
     @Override
     public void onChange(boolean selfChange) {
         onSystemFontScaleChange(mApplicationContext.getContentResolver(), false);
     }
 }
--- a/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java
+++ b/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java
@@ -20,17 +20,16 @@ import android.support.annotation.AnyThr
 import android.support.annotation.NonNull;
 import android.support.annotation.Nullable;
 import android.support.annotation.UiThread;
 import android.text.TextUtils;
 import android.util.Log;
 
 import org.mozilla.gecko.EventDispatcher;
 import org.mozilla.gecko.GeckoAppShell;
-import org.mozilla.gecko.GeckoFontScaleListener;
 import org.mozilla.gecko.GeckoSystemStateListener;
 import org.mozilla.gecko.GeckoScreenOrientation;
 import org.mozilla.gecko.GeckoThread;
 import org.mozilla.gecko.PrefsHelper;
 import org.mozilla.gecko.util.BundleEventListener;
 import org.mozilla.gecko.util.EventCallback;
 import org.mozilla.gecko.util.GeckoBundle;
 import org.mozilla.gecko.util.StringUtils;
--- a/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java
+++ b/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java
@@ -15,17 +15,16 @@ import android.os.Bundle;
 import android.os.Parcel;
 import android.os.Parcelable;
 import android.support.annotation.AnyThread;
 import android.support.annotation.IntDef;
 import android.support.annotation.NonNull;
 import android.support.annotation.Nullable;
 
 import org.mozilla.gecko.EventDispatcher;
-import org.mozilla.gecko.GeckoFontScaleListener;
 import org.mozilla.gecko.util.GeckoBundle;
 
 @AnyThread
 public final class GeckoRuntimeSettings extends RuntimeSettings {
     /**
      * Settings builder used to construct the settings object.
      */
     @AnyThread
@@ -169,16 +168,20 @@ public final class GeckoRuntimeSettings 
         }
 
         /**
          * Set a font size factor that will operate as a global text zoom. All font sizes will be
          * multiplied by this factor.
          *
          * <p>The default factor is 1.0.
          *
+         * <p>This setting cannot be modified if
+         * {@link Builder#automaticFontSizeAdjustment automatic font size adjustment}
+         * has already been enabled.
+         *
          * @param fontSizeFactor The factor to be used for scaling all text. Setting a value of 0
          *                       disables both this feature and
          *                       {@link Builder#fontInflation font inflation}.
          * @return The builder instance.
          */
         public @NonNull Builder fontSizeFactor(float fontSizeFactor) {
             getSettings().setFontSizeFactor(fontSizeFactor);
             return this;
@@ -192,16 +195,20 @@ public final class GeckoRuntimeSettings 
          * &lt;meta&gt; viewport tag and have been loaded in a session using
          * {@link GeckoSessionSettings#VIEWPORT_MODE_MOBILE}. To improve readability, the font
          * inflation logic will attempt to increase font sizes for the main text content of the page
          * only.
          *
          * <p>The magnitude of font inflation applied depends on the
          * {@link Builder#fontSizeFactor font size factor} currently in use.
          *
+         * <p>This setting cannot be modified if
+         * {@link Builder#automaticFontSizeAdjustment automatic font size adjustment}
+         * has already been enabled.
+         *
          * @param enabled A flag determining whether or not font inflation should be enabled.
          * @return The builder instance.
          */
         public @NonNull Builder fontInflation(boolean enabled) {
             getSettings().setFontInflationEnabled(enabled);
             return this;
         }
 
@@ -611,22 +618,33 @@ public final class GeckoRuntimeSettings 
     /**
      * Set a font size factor that will operate as a global text zoom. All font sizes will be
      * multiplied by this factor.
      *
      * <p>The default factor is 1.0.
      *
      * <p>Currently, any changes only take effect after a reload of the session.
      *
+     * <p>This setting cannot be modified while
+     * {@link GeckoRuntimeSettings#setAutomaticFontSizeAdjustment automatic font size adjustment}
+     * is enabled.
+     *
      * @param fontSizeFactor The factor to be used for scaling all text. Setting a value of 0
      *                       disables both this feature and
      *                       {@link GeckoRuntimeSettings#setFontInflationEnabled font inflation}.
      * @return This GeckoRuntimeSettings instance.
      */
     public @NonNull GeckoRuntimeSettings setFontSizeFactor(float fontSizeFactor) {
+        if (getAutomaticFontSizeAdjustment()) {
+            throw new IllegalStateException("Not allowed when automatic font size adjustment is enabled");
+        }
+        return setFontSizeFactorInternal(fontSizeFactor);
+    }
+
+    /* package */ @NonNull GeckoRuntimeSettings setFontSizeFactorInternal(float fontSizeFactor) {
         if (fontSizeFactor < 0) {
             throw new IllegalArgumentException("fontSizeFactor cannot be < 0");
         }
 
         final int fontSizePercentage = Math.round(fontSizeFactor * 100);
         mFontSizeFactor.commit(Math.round(fontSizePercentage));
         if (getFontInflationEnabled()) {
             final int scaledFontInflation = Math.round(FONT_INFLATION_BASE_VALUE * fontSizeFactor);
@@ -653,20 +671,31 @@ public final class GeckoRuntimeSettings 
      * {@link GeckoSessionSettings#VIEWPORT_MODE_MOBILE}. To improve readability, the font inflation
      * logic will attempt to increase font sizes for the main text content of the page only.
      *
      * <p>The magnitude of font inflation applied depends on the
      * {@link GeckoRuntimeSettings#setFontSizeFactor font size factor} currently in use.
      *
      * <p>Currently, any changes only take effect after a reload of the session.
      *
+     * <p>This setting cannot be modified while
+     * {@link GeckoRuntimeSettings#setAutomaticFontSizeAdjustment automatic font size adjustment}
+     * is enabled.
+     *
      * @param enabled A flag determining whether or not font inflation should be enabled.
      * @return This GeckoRuntimeSettings instance.
      */
     public @NonNull GeckoRuntimeSettings setFontInflationEnabled(boolean enabled) {
+        if (getAutomaticFontSizeAdjustment()) {
+            throw new IllegalStateException("Not allowed when automatic font size adjustment is enabled");
+        }
+        return setFontInflationEnabledInternal(enabled);
+    }
+
+    /* package */ @NonNull GeckoRuntimeSettings setFontInflationEnabledInternal(boolean enabled) {
         final int minTwips =
                 enabled ? Math.round(FONT_INFLATION_BASE_VALUE * getFontSizeFactor()) : 0;
         mFontInflationMinTwips.commit(minTwips);
         return this;
     }
 
     /**
      * Get whether or not font inflation for non mobile-friendly pages is currently enabled.