Fixes #23492: Perf regression of calling isFirefoxDefault from main thread (#23556)

* Fixes #23492 — Fixup perf regression of calling isFirefoxDefault from the main thread * Tightening of near defunct default browser message * Fixup early crash in debug build * ktlint
2 years ago · b230c39a7f
parent 534838c0fb
commit b230c39a7f
6 changed files with 53 additions and 45 deletions
--- a/.experimenter.json
+++ b/.experimenter.json
@ -1,12 +1,12 @@
 {
-  "search-term-groups": {
-    "description": "A feature allowing the grouping of URLs around the search term that it came from.",
+  "homescreen": {
+    "description": "The homescreen that the user goes to when they press home or new tab.",
    "hasExposure": true,
    "exposureDescription": "",
    "variables": {
-      "enabled": {
-        "description": "If true, the feature shows up on the homescreen and on the new tab screen.",
-        "type": "boolean"
+      "sections-enabled": {
+        "description": "This property provides a lookup table of whether or not the given section should be enabled. If the section is enabled, it should be toggleable in the settings screen, and on by default.",
+        "type": "json"
      }
    }
  },
@ -17,15 +17,21 @@
    "variables": {
      "message-location": {
        "description": "Where is the message to be put.",
-        "enum": [
-          "homescreen-banner",
-          "settings",
-          "app-menu-item"
-        ],
        "type": "string"
      }
    }
  },
+  "search-term-groups": {
+    "description": "A feature allowing the grouping of URLs around the search term that it came from.",
+    "hasExposure": true,
+    "exposureDescription": "",
+    "variables": {
+      "enabled": {
+        "description": "If true, the feature shows up on the homescreen and on the new tab screen.",
+        "type": "boolean"
+      }
+    }
+  },
  "nimbus-validation": {
    "description": "A feature that does not correspond to an application feature suitable for showing that Nimbus is working. This should never be used in production.",
    "hasExposure": true,
@ -44,16 +50,5 @@
        "type": "string"
      }
    }
-  },
-  "homescreen": {
-    "description": "The homescreen that the user goes to when they press home or new tab.",
-    "hasExposure": true,
-    "exposureDescription": "",
-    "variables": {
-      "sections-enabled": {
-        "description": "This property provides a lookup table of whether or not the given section should be enabled. If the section is enabled, it should be toggleable in the settings screen, and on by default.",
-        "type": "json"
-      }
-    }
  }
 }
--- a/app/src/main/java/org/mozilla/fenix/components/toolbar/DefaultToolbarMenu.kt
+++ b/app/src/main/java/org/mozilla/fenix/components/toolbar/DefaultToolbarMenu.kt
@ -39,10 +39,8 @@ import org.mozilla.fenix.R
 import org.mozilla.fenix.components.accounts.FenixAccountManager
 import org.mozilla.fenix.ext.components
 import org.mozilla.fenix.ext.settings
-import org.mozilla.fenix.nimbus.FxNimbus
 import org.mozilla.fenix.nimbus.MessageSurfaceId
 import org.mozilla.fenix.theme.ThemeManager
-import org.mozilla.fenix.utils.BrowsersCache

 /**
 * Builds the toolbar object used with the 3-dot menu in the browser fragment.
@ -446,10 +444,11 @@ open class DefaultToolbarMenu(
        }
    }

-    private fun getSetDefaultBrowserItem(): BrowserMenuImageText? =
-        if (BrowsersCache.all(context).isFirefoxDefaultBrowser) {
-            null
-        } else if (FxNimbus.features.defaultBrowserMessage.value().messageLocation == MessageSurfaceId.APP_MENU_ITEM) {
+    private fun getSetDefaultBrowserItem(): BrowserMenuImageText? {
+        val settings = context.components.settings
+        return if (
+            settings.isDefaultBrowserMessageLocation(MessageSurfaceId.APP_MENU_ITEM)
+        ) {
            BrowserMenuImageText(
                label = context.getString(R.string.preferences_set_as_default_browser),
                imageResource = R.mipmap.ic_launcher
@ -459,4 +458,5 @@ open class DefaultToolbarMenu(
        } else {
            null
        }
+    }
 }
--- a/app/src/main/java/org/mozilla/fenix/home/HomeMenu.kt
+++ b/app/src/main/java/org/mozilla/fenix/home/HomeMenu.kt
@ -175,14 +175,9 @@ class HomeMenu(

        // Use nimbus to set the icon and title.
        val nimbusValidation = FxNimbus.features.nimbusValidation.value()
-        val settingsIcon = context.resources.getIdentifier(
-            nimbusValidation.settingsIcon,
-            "drawable",
-            context.packageName
-        )
        val settingsItem = BrowserMenuImageText(
            nimbusValidation.settingsTitle,
-            settingsIcon,
+            R.drawable.mozac_ic_settings,
            primaryTextColor
        ) {
            onItemTapped.invoke(Item.Settings)
--- a/app/src/main/java/org/mozilla/fenix/settings/SettingsFragment.kt
+++ b/app/src/main/java/org/mozilla/fenix/settings/SettingsFragment.kt
@ -148,7 +148,7 @@ class SettingsFragment : PreferenceFragmentCompat() {
     * Note: Changing Settings screen before experiment is over requires changing all layouts.
     */
    private fun getPreferenceLayoutId() =
-        if (isDefaultBrowserExperimentBranch() && !isFirefoxDefaultBrowser()) {
+        if (isDefaultBrowserExperimentBranch()) {
            R.xml.preferences_default_browser_experiment
        } else {
            R.xml.preferences
@ -473,7 +473,7 @@ class SettingsFragment : PreferenceFragmentCompat() {
     */
    private fun getClickListenerForMakeDefaultBrowser(): Preference.OnPreferenceClickListener {
        return Preference.OnPreferenceClickListener {
-            if (isDefaultBrowserExperimentBranch() && !isFirefoxDefaultBrowser()) {
+            if (isDefaultBrowserExperimentBranch()) {
                requireContext().metrics.track(Event.SetDefaultBrowserSettingsScreenClicked)
            }
            activity?.openSetDefaultBrowserOption()
@ -605,7 +605,7 @@ class SettingsFragment : PreferenceFragmentCompat() {
    }

    private fun isDefaultBrowserExperimentBranch(): Boolean =
-        FxNimbus.features.defaultBrowserMessage.value().messageLocation == MessageSurfaceId.SETTINGS
+        requireContext().settings().isDefaultBrowserMessageLocation(MessageSurfaceId.SETTINGS)

    private fun isFirefoxDefaultBrowser(): Boolean {
        val browsers = BrowsersCache.all(requireContext())
--- a/app/src/main/java/org/mozilla/fenix/utils/Settings.kt
+++ b/app/src/main/java/org/mozilla/fenix/utils/Settings.kt
@ -37,6 +37,7 @@ import org.mozilla.fenix.components.settings.lazyFeatureFlagPreference
 import org.mozilla.fenix.components.toolbar.ToolbarPosition
 import org.mozilla.fenix.ext.components
 import org.mozilla.fenix.ext.getPreferenceKey
+import org.mozilla.fenix.nimbus.DefaultBrowserMessage
 import org.mozilla.fenix.nimbus.FxNimbus
 import org.mozilla.fenix.nimbus.HomeScreenSection
 import org.mozilla.fenix.nimbus.MessageSurfaceId
@ -326,15 +327,25 @@ class Settings(private val appContext: Context) : PreferencesHolder {
     * Shows if the set default browser experiment card should be shown on home screen.
     */
    fun shouldShowSetAsDefaultBrowserCard(): Boolean {
-        val browsers = BrowsersCache.all(appContext)
-        val feature = FxNimbus.features.defaultBrowserMessage.value()
-        val isExperimentBranch = feature.messageLocation == MessageSurfaceId.HOMESCREEN_BANNER
-        return isExperimentBranch &&
+        return isDefaultBrowserMessageLocation(MessageSurfaceId.HOMESCREEN_BANNER) &&
            !userDismissedExperimentCard &&
-            !browsers.isFirefoxDefaultBrowser &&
            numberOfAppLaunches > APP_LAUNCHES_TO_SHOW_DEFAULT_BROWSER_CARD
    }

+    private val defaultBrowserFeature: DefaultBrowserMessage by lazy {
+        FxNimbus.features.defaultBrowserMessage.value()
+    }
+
+    fun isDefaultBrowserMessageLocation(surfaceId: MessageSurfaceId): Boolean =
+        defaultBrowserFeature.messageLocation?.let { experimentalSurfaceId ->
+            if (experimentalSurfaceId == surfaceId) {
+                val browsers = BrowsersCache.all(appContext)
+                !browsers.isFirefoxDefaultBrowser
+            } else {
+                false
+            }
+        } ?: false
+
    var gridTabView by booleanPreference(
        appContext.getPreferenceKey(R.string.pref_key_tab_view_grid),
        default = true
@ -453,9 +464,9 @@ class Settings(private val appContext: Context) : PreferencesHolder {
    /**
     * Indicates if the user has enabled the search term tab groups feature.
     */
-    var searchTermTabGroupsAreEnabled by featureFlagPreference(
+    var searchTermTabGroupsAreEnabled by lazyFeatureFlagPreference(
        appContext.getPreferenceKey(R.string.pref_key_search_term_tab_groups),
-        default = FeatureFlags.tabGroupFeature,
+        default = { FxNimbus.features.searchTermGroups.value(appContext).enabled },
        featureFlag = FeatureFlags.tabGroupFeature
    )

--- a/nimbus.fml.yaml
+++ b/nimbus.fml.yaml
@ -54,13 +54,20 @@ features:
        description: If true, the feature shows up on the homescreen and on the new tab screen.
        type: Boolean
        default: false
+    defaults:
+      - channel: nightly
+        value:
+          enabled: true
+      - channel: developer
+        value:
+          enabled: true
  default-browser-message:
    description: A small feature allowing experiments on the placement of a default browser message.
    variables:
      message-location:
        description: Where is the message to be put.
-        type: MessageSurfaceId
-        default: homescreen-banner
+        type: Option<MessageSurfaceId>
+        default: null
 types:
  objects: {}
  enums: