Clean up toolbar menu class

app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarView.kt

@@ -187,12 +187,6 @@ class BrowserToolbarView(
                 DefaultToolbarMenu(
                     context = this,
                     hasAccountProblem = components.backgroundServices.accountManager.accountNeedsReauth(),
-                    requestDesktopStateProvider = {
-                        sessionManager.selectedSession?.desktopMode ?: false
-                    },
-                    readerModeStateProvider = {
-                        sessionManager.selectedSession?.readerMode ?: false
-                    },
                     shouldReverseItems = !shouldUseBottomToolbar,
                     onItemTapped = { interactor.onBrowserToolbarMenuItemTapped(it) },
                     lifecycleOwner = container.context as AppCompatActivity,

app/src/main/java/org/mozilla/fenix/components/toolbar/DefaultToolbarMenu.kt

@@ -5,6 +5,7 @@
 package org.mozilla.fenix.components.toolbar
 
 import android.content.Context
+import androidx.annotation.ColorRes
 import androidx.core.content.ContextCompat.getColor
 import androidx.lifecycle.LifecycleOwner
 import androidx.lifecycle.lifecycleScope
@@ -29,28 +30,38 @@ import org.mozilla.fenix.ext.components
 import org.mozilla.fenix.theme.ThemeManager
 import org.mozilla.fenix.utils.Settings
 
-@Suppress("LargeClass") // While large, most of the class is very simple
+/**
+ * Builds the toolbar object used with the 3-dot menu in the browser fragment.
+ * @param sessionManager Reference to the session manager that contains all tabs.
+ * @param hasAccountProblem If true, there was a problem signing into the Firefox account.
+ * @param shouldReverseItems If true, reverse the menu items.
+ * @param onItemTapped Called when a menu item is tapped.
+ * @param lifecycleOwner View lifecycle owner used to determine when to cancel UI jobs.
+ * @param bookmarksStorage Used to check if a page is bookmarked.
+ */
+@Suppress("LargeClass")
 class DefaultToolbarMenu(
     private val context: Context,
-    private val hasAccountProblem: Boolean = false,
+    private val sessionManager: SessionManager,
-    private val requestDesktopStateProvider: () -> Boolean = { false },
+    hasAccountProblem: Boolean = false,
-    private val shouldReverseItems: Boolean,
+    shouldReverseItems: Boolean,
     private val onItemTapped: (ToolbarMenu.Item) -> Unit = {},
     private val lifecycleOwner: LifecycleOwner,
-    private val bookmarksStorage: BookmarksStorage,
+    private val bookmarksStorage: BookmarksStorage
-    readerModeStateProvider: () -> Boolean = { false },
-    sessionManager: SessionManager
 ) : ToolbarMenu {
 
     private var currentUrlIsBookmarked = false
     private var isBookmarkedJob: Job? = null
 
+    /** Gets the current browser session */
+    private val session: Session? get() = sessionManager.selectedSession
+
     override val menuBuilder by lazy {
         WebExtensionBrowserMenuBuilder(
             menuItems,
             endOfMenuAlwaysVisible = true,
             store = context.components.core.store,
-            appendExtensionActionAtStart = true
+            appendExtensionActionAtStart = !shouldReverseItems
         )
     }
 
@@ -58,17 +69,11 @@ class DefaultToolbarMenu(
         val forward = BrowserMenuItemToolbar.TwoStateButton(
             primaryImageResource = mozilla.components.ui.icons.R.drawable.mozac_ic_forward,
             primaryContentDescription = context.getString(R.string.browser_menu_forward),
-            primaryImageTintResource = ThemeManager.resolveAttribute(
+            primaryImageTintResource = primaryTextColor(),
-                R.attr.primaryText,
-                context
-            ),
             isInPrimaryState = {
-                context.components.core.sessionManager.selectedSession?.canGoForward ?: true
+                session?.canGoForward ?: true
             },
-            secondaryImageTintResource = ThemeManager.resolveAttribute(
+            secondaryImageTintResource = ThemeManager.resolveAttribute(R.attr.disabled, context),
-                R.attr.disabled,
-                context
-            ),
             disableInSecondaryState = true
         ) {
             onItemTapped.invoke(ToolbarMenu.Item.Forward)
@@ -77,23 +82,16 @@ class DefaultToolbarMenu(
         val refresh = BrowserMenuItemToolbar.TwoStateButton(
             primaryImageResource = mozilla.components.ui.icons.R.drawable.mozac_ic_refresh,
             primaryContentDescription = context.getString(R.string.browser_menu_refresh),
-            primaryImageTintResource = ThemeManager.resolveAttribute(
+            primaryImageTintResource = primaryTextColor(),
-                R.attr.primaryText,
-                context
-            ),
             isInPrimaryState = {
-                val loading = context.components.core.sessionManager.selectedSession?.loading
+                session?.loading == false
-                loading == false
             },
             secondaryImageResource = mozilla.components.ui.icons.R.drawable.mozac_ic_stop,
             secondaryContentDescription = context.getString(R.string.browser_menu_stop),
-            secondaryImageTintResource = ThemeManager.resolveAttribute(
+            secondaryImageTintResource = primaryTextColor(),
-                R.attr.primaryText,
-                context
-            ),
             disableInSecondaryState = false
         ) {
-            if (context.components.core.sessionManager.selectedSession?.loading == true) {
+            if (session?.loading == true) {
                 onItemTapped.invoke(ToolbarMenu.Item.Stop)
             } else {
                 onItemTapped.invoke(ToolbarMenu.Item.Reload)
@@ -109,24 +107,18 @@ class DefaultToolbarMenu(
             }
         )
 
-        registerForIsBookmarkedUpdates(sessionManager)
+        registerForIsBookmarkedUpdates()
         val bookmark = BrowserMenuItemToolbar.TwoStateButton(
             primaryImageResource = R.drawable.ic_bookmark_filled,
             primaryContentDescription = context.getString(R.string.browser_menu_edit_bookmark),
-            primaryImageTintResource = ThemeManager.resolveAttribute(
+            primaryImageTintResource = primaryTextColor(),
-                R.attr.primaryText,
-                context
-            ),
             // TwoStateButton.isInPrimaryState must be synchronous, and checking bookmark state is
             // relatively slow. The best we can do here is periodically compute and cache a new "is
             // bookmarked" state, and use that whenever the menu has been opened.
             isInPrimaryState = { currentUrlIsBookmarked },
             secondaryImageResource = R.drawable.ic_bookmark_outline,
             secondaryContentDescription = context.getString(R.string.browser_menu_bookmark),
-            secondaryImageTintResource = ThemeManager.resolveAttribute(
+            secondaryImageTintResource = primaryTextColor(),
-                R.attr.primaryText,
-                context
-            ),
             disableInSecondaryState = false
         ) {
             if (!currentUrlIsBookmarked) currentUrlIsBookmarked = true
@@ -144,18 +136,14 @@ class DefaultToolbarMenu(
             .shouldDeleteBrowsingDataOnQuit
 
         // Predicates that need to be repeatedly called as the session changes
-        fun shouldShowAddToHomescreen(): Boolean {
+        fun shouldShowAddToHomescreen(): Boolean =
-            return context.components.useCases.webAppUseCases.isPinningSupported() &&
+            session != null && context.components.useCases.webAppUseCases.isPinningSupported()
-                    context.components.core.sessionManager.selectedSession != null
+        fun shouldShowReaderMode(): Boolean = session?.readerable ?: false
-        }
+        fun shouldShowOpenInApp(): Boolean = session?.let { session ->
-        fun shouldShowReaderMode(): Boolean = sessionManager.selectedSession?.readerable ?: false
+            val appLink = context.components.useCases.appLinksUseCases.appLinkRedirect
-        fun shouldShowOpenInApp(): Boolean = sessionManager.selectedSession?.let { session ->
-            val appLink =
-                context.components.useCases.appLinksUseCases.appLinkRedirect
             appLink(session.url).hasExternalApp()
         } ?: false
-        fun shouldShowReaderAppearance(): Boolean =
+        fun shouldShowReaderAppearance(): Boolean = session?.readerMode ?: false
-            sessionManager.selectedSession?.readerMode ?: false
 
         val menuItems = listOfNotNull(
             help,
@@ -182,17 +170,17 @@ class DefaultToolbarMenu(
     }
 
     private val addons = BrowserMenuImageText(
-        context.getString(R.string.browser_menu_addon_manager),
+        label = context.getString(R.string.browser_menu_addon_manager),
-        R.drawable.mozac_ic_extensions,
+        imageResource = R.drawable.mozac_ic_extensions,
-        primaryTextColor()
+        iconTintColorResource = primaryTextColor()
     ) {
         onItemTapped.invoke(ToolbarMenu.Item.AddonsManager)
     }
 
     private val help = BrowserMenuImageText(
-        context.getString(R.string.browser_menu_help),
+        label = context.getString(R.string.browser_menu_help),
-        R.drawable.ic_help,
+        imageResource = R.drawable.ic_help,
-        primaryTextColor()
+        iconTintColorResource = primaryTextColor()
     ) {
         onItemTapped.invoke(ToolbarMenu.Item.Help)
     }
@@ -226,7 +214,9 @@ class DefaultToolbarMenu(
     private val desktopMode = BrowserMenuImageSwitch(
         imageResource = R.drawable.ic_desktop,
         label = context.getString(R.string.browser_menu_desktop_site),
-        initialState = requestDesktopStateProvider
+        initialState = {
+            session?.desktopMode ?: false
+        }
     ) { checked ->
         onItemTapped.invoke(ToolbarMenu.Item.RequestDesktop(checked))
     }
@@ -306,7 +296,9 @@ class DefaultToolbarMenu(
     private val readerMode = BrowserMenuHighlightableSwitch(
         label = context.getString(R.string.browser_menu_read),
         startImageResource = R.drawable.ic_readermode,
-        initialState = readerModeStateProvider,
+        initialState = {
+            session?.readerMode ?: false
+        },
         highlight = BrowserMenuHighlight.LowPriority(
             label = context.getString(R.string.browser_menu_read),
             notificationTint = getColor(context, R.color.whats_new_notification_color)
@@ -333,13 +325,14 @@ class DefaultToolbarMenu(
             notificationTint = getColor(context, R.color.whats_new_notification_color)
         ),
         isHighlighted = { true }
-        ) {
+    ) {
         onItemTapped.invoke(ToolbarMenu.Item.OpenInApp)
     }
 
+    @ColorRes
     private fun primaryTextColor() = ThemeManager.resolveAttribute(R.attr.primaryText, context)
 
-    private fun registerForIsBookmarkedUpdates(sessionManager: SessionManager) {
+    private fun registerForIsBookmarkedUpdates() {
         val observer = object : Session.Observer {
             override fun onUrlChanged(session: Session, url: String) {
                 currentUrlIsBookmarked = false
@@ -347,8 +340,8 @@ class DefaultToolbarMenu(
             }
         }
 
-        sessionManager.selectedSession?.url?.let { updateCurrentUrlIsBookmarked(it) }
+        session?.url?.let { updateCurrentUrlIsBookmarked(it) }
-        sessionManager.selectedSession?.register(observer, lifecycleOwner)
+        session?.register(observer, lifecycleOwner)
     }
 
     private fun updateCurrentUrlIsBookmarked(newUrl: String) {

app/src/main/java/org/mozilla/fenix/components/toolbar/ToolbarMenu.kt

@@ -36,8 +36,4 @@ interface ToolbarMenu {
 
     val menuBuilder: BrowserMenuBuilder
     val menuToolbar: BrowserMenuItemToolbar
-
-    companion object {
-        const val CAPTION_TEXT_SIZE = 12f
-    }
 }

app/src/main/java/org/mozilla/fenix/customtabs/CustomTabToolbarMenu.kt

@@ -6,6 +6,7 @@ package org.mozilla.fenix.customtabs
 
 import android.content.Context
 import android.graphics.Typeface
+import androidx.annotation.ColorRes
 import mozilla.components.browser.menu.BrowserMenuBuilder
 import mozilla.components.browser.menu.item.BrowserMenuCategory
 import mozilla.components.browser.menu.item.BrowserMenuDivider
@@ -19,6 +20,13 @@ import org.mozilla.fenix.R
 import org.mozilla.fenix.components.toolbar.ToolbarMenu
 import org.mozilla.fenix.theme.ThemeManager
 
+/**
+ * Builds the toolbar object used with the 3-dot menu in the custom tab browser fragment.
+ * @param sessionManager Reference to the session manager that contains all tabs.
+ * @param sessionId ID of the open custom tab session.
+ * @param shouldReverseItems If true, reverse the menu items.
+ * @param onItemTapped Called when a menu item is tapped.
+ */
 class CustomTabToolbarMenu(
     private val context: Context,
     private val sessionManager: SessionManager,
@@ -26,10 +34,12 @@ class CustomTabToolbarMenu(
     private val shouldReverseItems: Boolean,
     private val onItemTapped: (ToolbarMenu.Item) -> Unit = {}
 ) : ToolbarMenu {
+
     override val menuBuilder by lazy { BrowserMenuBuilder(menuItems) }
 
-    private val session: Session?
+    /** Gets the current custom tab session */
-        get() = sessionId?.let { sessionManager.findSessionById(it) }
+    private val session: Session? get() = sessionId?.let { sessionManager.findSessionById(it) }
+    private val appName = context.getString(R.string.app_name)
 
     override val menuToolbar by lazy {
         val back = BrowserMenuItemToolbar.TwoStateButton(
@@ -116,24 +126,23 @@ class CustomTabToolbarMenu(
     }
 
     private val openInFenix = SimpleBrowserMenuItem(
-        label = {
+        label = context.getString(R.string.browser_menu_open_in_fenix, appName),
-            val appName = context.getString(R.string.app_name)
-            context.getString(R.string.browser_menu_open_in_fenix, appName)
-        }(),
         textColorResource = primaryTextColor()
     ) {
         onItemTapped.invoke(ToolbarMenu.Item.OpenInFenix)
     }
 
     private val poweredBy = BrowserMenuCategory(
-        label = {
+        label = context.getString(R.string.browser_menu_powered_by, appName).toUpperCase(),
-            val appName = context.getString(R.string.app_name)
+        textSize = CAPTION_TEXT_SIZE,
-            context.getString(R.string.browser_menu_powered_by, appName).toUpperCase()
-        }(),
-        textSize = ToolbarMenu.CAPTION_TEXT_SIZE,
         textColorResource = primaryTextColor(),
         textStyle = Typeface.NORMAL
     )
 
+    @ColorRes
     private fun primaryTextColor() = ThemeManager.resolveAttribute(R.attr.primaryText, context)
+
+    companion object {
+        private const val CAPTION_TEXT_SIZE = 12f
+    }
 }