From b230c39a7fef879a97188661ea2d3b561d778581 Mon Sep 17 00:00:00 2001 From: jhugman Date: Tue, 8 Feb 2022 12:44:07 +0000 Subject: [PATCH] Fixes #23492: Perf regression of calling isFirefoxDefault from main thread (#23556) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 --- .experimenter.json | 37 ++++++++----------- .../components/toolbar/DefaultToolbarMenu.kt | 12 +++--- .../java/org/mozilla/fenix/home/HomeMenu.kt | 7 +--- .../fenix/settings/SettingsFragment.kt | 6 +-- .../java/org/mozilla/fenix/utils/Settings.kt | 25 +++++++++---- nimbus.fml.yaml | 11 +++++- 6 files changed, 53 insertions(+), 45 deletions(-) diff --git a/.experimenter.json b/.experimenter.json index aafcabe34..5b32be7b9 100644 --- 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" - } - } } } \ No newline at end of file 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 index 5d9949aa4..88a31432c 100644 --- 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 index 834a5d045..7f019a08d 100644 --- 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 index 5c5e5ab24..3286e833c 100644 --- 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 index fc2674137..048dbf674 100644 --- 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 index 3589a8722..bbdbf0bf6 100644 --- 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 + default: null types: objects: {} enums: