Fixes #23492: Perf regression of calling isFirefoxDefault from main t…

…hread (#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
mozilla-mobile · Feb 8, 2022 · b230c39 · b230c39
1 parent 534838c
commit b230c39
Show file tree

Hide file tree

Showing 6 changed files with 53 additions and 45 deletions.
diff --git 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"
-      }
-    }
   }
 }
diff --git 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
         }
+    }
 }
diff --git 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)

diff --git 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())

diff --git 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
     )
 

diff --git 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: