For mozilla-mobile#8554 - Addressed comments

components/browser/state/src/main/java/mozilla/components/browser/state/action/BrowserAction.kt

@@ -448,35 +448,31 @@ sealed class ContentAction : BrowserAction() {
     data class UpdateLoadRequestAction(val sessionId: String, val loadRequest: LoadRequestState) : ContentAction()
 
     /**
-     * Adds a new unprocessed content permission request to the [ContentState] list, to be
-     * processed in [mozilla.components.feature.sitepermissions.SitePermissionsFeature]
+     * Adds a new content permission request to the [ContentState] list.
      * */
     data class UpdatePermissionsRequest(
         val sessionId: String,
         val permissionRequest: PermissionRequest
     ) : ContentAction()
 
     /**
-     * Deletes a content permission request from the [ContentState] list, has been
-     * processed in [mozilla.components.feature.sitepermissions.SitePermissionsFeature]
+     * Deletes a content permission request from the [ContentState] list.
      * */
     data class ConsumePermissionsRequest(
         val sessionId: String,
         val permissionRequest: PermissionRequest
     ) : ContentAction()
 
     /**
-     * Adds a new unprocessed app permission request to the [ContentState] list, to be
-     * processed in [mozilla.components.feature.sitepermissions.SitePermissionsFeature]
+     * Adds a new app permission request to the [ContentState] list.
      * */
     data class UpdateAppPermissionsRequest(
         val sessionId: String,
         val appPermissionRequest: PermissionRequest
     ) : ContentAction()
 
     /**
-     * Deletes an app permission request from the [ContentState] list, has been
-     * processed in [mozilla.components.feature.sitepermissions.SitePermissionsFeature]
+     * Deletes an app permission request from the [ContentState] list.
      * */
     data class ConsumeAppPermissionsRequest(
         val sessionId: String,

components/browser/state/src/main/java/mozilla/components/browser/state/ext/PermissionRequest.kt

@@ -5,8 +5,7 @@ package mozilla.components.browser.state.ext
 import mozilla.components.concept.engine.permission.PermissionRequest
 
 /**
- * An improved version of [List.contains] built to be used in [ContentStateReducer] to make sure
- * no duplicate requests are added to the state.
+ * @returns true if the given [permissionRequest] is contained in this list otherwise false
  * */
 fun List<PermissionRequest>.doesContain(permissionRequest: PermissionRequest): Boolean {
     return this.any {

components/browser/state/src/main/java/mozilla/components/browser/state/reducer/ContentStateReducer.kt

@@ -11,6 +11,7 @@ import mozilla.components.browser.state.state.BrowserState
 import mozilla.components.browser.state.state.ContentState
 import mozilla.components.browser.state.state.SessionState
 import mozilla.components.browser.state.state.content.HistoryState
+import mozilla.components.concept.engine.permission.PermissionRequest
 import mozilla.components.support.ktx.android.net.sameSchemeAndHostAs
 
 internal object ContentStateReducer {
@@ -134,10 +135,8 @@ internal object ContentStateReducer {
                 action.sessionId
             ) {
                 if (!it.permissionRequestsList.doesContain(action.permissionRequest)) {
-                    val currentList = it.permissionRequestsList.toMutableList()
-                    currentList.add(action.permissionRequest)
                     it.copy(
-                        permissionRequestsList = currentList
+                        permissionRequestsList = it.permissionRequestsList + action.permissionRequest
                     )
                 } else {
                     it
@@ -148,10 +147,8 @@ internal object ContentStateReducer {
                 action.sessionId
             ) {
                 if (it.permissionRequestsList.doesContain(action.permissionRequest)) {
-                    val newList = it.permissionRequestsList.toMutableList()
-                    newList.remove(action.permissionRequest)
                     it.copy(
-                        permissionRequestsList = newList
+                        permissionRequestsList = it.permissionRequestsList - action.permissionRequest
                     )
                 } else {
                     it
@@ -162,10 +159,8 @@ internal object ContentStateReducer {
                 action.sessionId
             ) {
                 if (!it.appPermissionRequestsList.doesContain(action.appPermissionRequest)) {
-                    val currentList = it.appPermissionRequestsList.toMutableList()
-                    currentList.add(action.appPermissionRequest)
                     it.copy(
-                        appPermissionRequestsList = currentList
+                        appPermissionRequestsList = it.appPermissionRequestsList + action.appPermissionRequest
                     )
                 } else {
                     it
@@ -176,10 +171,8 @@ internal object ContentStateReducer {
                 action.sessionId
             ) {
                 if (it.appPermissionRequestsList.doesContain(action.appPermissionRequest)) {
-                    val newList = it.appPermissionRequestsList.toMutableList()
-                    newList.remove(action.appPermissionRequest)
                     it.copy(
-                        appPermissionRequestsList = newList
+                        appPermissionRequestsList = it.appPermissionRequestsList - action.appPermissionRequest
                     )
                 } else {
                     it

components/browser/state/src/main/java/mozilla/components/browser/state/state/ContentState.kt

@@ -43,9 +43,9 @@ import mozilla.components.concept.engine.window.WindowRequest
  * @property webAppManifest the Web App Manifest for the currently visited page (or null).
  * @property firstContentfulPaint whether or not the first contentful paint has happened.
  * @property pictureInPictureEnabled True if the session is being displayed in PIP mode.
- * @property permissionRequestsList Holds unprocessed content requests sent from EngineObserver.
- * @property appPermissionRequestsList Holds unprocessed app requests sent from EngineObserver.
  * @property loadRequest last [LoadRequestState] if this session.
+ * @property permissionRequestsList Holds unprocessed content requests.
+ * @property appPermissionRequestsList Holds unprocessed app requests.
  */
 data class ContentState(
     val url: String,

components/feature/sitepermissions/src/main/java/mozilla/components/feature/sitepermissions/SitePermissionsFeature.kt

@@ -12,6 +12,7 @@ import android.content.pm.PackageManager
 import androidx.annotation.ColorRes
 import androidx.annotation.DrawableRes
 import androidx.annotation.StringRes
+import androidx.annotation.VisibleForTesting
 import androidx.core.net.toUri
 import androidx.fragment.app.FragmentManager
 import kotlinx.coroutines.CoroutineScope
@@ -24,10 +25,9 @@ import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.cancel
 import mozilla.components.browser.session.Session
 import mozilla.components.browser.session.SessionManager
-import mozilla.components.browser.session.runWithSession
-import mozilla.components.browser.session.runWithSessionIdOrSelected
 import mozilla.components.browser.state.action.ContentAction
 import mozilla.components.browser.state.selector.findTabOrCustomTabOrSelectedTab
+import mozilla.components.browser.state.state.ContentState
 import mozilla.components.browser.state.store.BrowserStore
 import mozilla.components.concept.engine.permission.Permission
 import mozilla.components.concept.engine.permission.Permission.ContentAudioCapture
@@ -122,7 +122,7 @@ class SitePermissionsFeature(
                     .collect { permissionRequest ->
 
                         val host =
-                            store.state.findTabOrCustomTabOrSelectedTab(customTabId)?.content?.url
+                            getCurrentContentState()?.url
                                 ?: ""
 
                         if (permissionRequest.permissions.all { it.isSupported() }) {
@@ -150,8 +150,12 @@ class SitePermissionsFeature(
             }
     }
 
-    private fun consumePermissionRequest(permissionRequest: PermissionRequest) {
-        sessionId?.let { sessionId ->
+    private fun consumePermissionRequest(
+        permissionRequest: PermissionRequest,
+        optionalSessionId: String? = null
+    ) {
+        val thisSessionId = optionalSessionId ?: sessionId
+        thisSessionId?.let { sessionId ->
             store.dispatch(ContentAction.ConsumePermissionsRequest(sessionId, permissionRequest))
         }
     }
@@ -180,32 +184,40 @@ class SitePermissionsFeature(
      */
     @Suppress("MaxLineLength")
     override fun onPermissionsResult(permissions: Array<String>, grantResults: IntArray) {
-        sessionManager.runWithSessionIdOrSelected(sessionId) { session ->
+        val currentContentSate = getCurrentContentState()
+        val appPermissionRequest = findRequestedAppPermission(permissions)
 
-            val appPermissionRequest =
-                store.state.findTabOrCustomTabOrSelectedTab(customTabId)?.content?.appPermissionRequestsList?.find {
-                    permissions.contains(it.permissions.first().id)
-                }
-
-            if (appPermissionRequest != null) {
-                val allPermissionWereGranted = grantResults.all { grantResult ->
-                    grantResult == PackageManager.PERMISSION_GRANTED
-                }
+        if (appPermissionRequest != null && currentContentSate != null) {
+            val allPermissionWereGranted = grantResults.all { grantResult ->
+                grantResult == PackageManager.PERMISSION_GRANTED
+            }
 
-                if (grantResults.isNotEmpty() && allPermissionWereGranted) {
-                    appPermissionRequest.grant()
-                } else {
-                    appPermissionRequest.reject()
-                    permissions.forEach { systemPermission ->
-                        if (!onShouldShowRequestPermissionRationale(systemPermission)) {
-                            // The system permission is denied permanently
-                            storeSitePermissions(session, appPermissionRequest, status = BLOCKED)
-                        }
+            if (grantResults.isNotEmpty() && allPermissionWereGranted) {
+                appPermissionRequest.grant()
+            } else {
+                appPermissionRequest.reject()
+                permissions.forEach { systemPermission ->
+                    if (!onShouldShowRequestPermissionRationale(systemPermission)) {
+                        // The system permission is denied permanently
+                        storeSitePermissions(
+                            currentContentSate,
+                            appPermissionRequest,
+                            status = BLOCKED
+                        )
                     }
                 }
-                consumeAppPermissionRequest(appPermissionRequest)
             }
-            true
+            consumeAppPermissionRequest(appPermissionRequest)
+        }
+    }
+
+    private fun getCurrentContentState() =
+        store.state.findTabOrCustomTabOrSelectedTab(customTabId)?.content
+
+    @VisibleForTesting
+    internal fun findRequestedAppPermission(permissions: Array<String>): PermissionRequest? {
+        return getCurrentContentState()?.appPermissionRequestsList?.find {
+            permissions.contains(it.permissions.first().id)
         }
     }
 
@@ -218,75 +230,67 @@ class SitePermissionsFeature(
      * If it's true none prompt will show otherwise the prompt will be shown.
      */
     internal fun onContentPermissionGranted(
-        session: Session,
         permissionRequest: PermissionRequest,
         shouldStore: Boolean
     ) {
         permissionRequest.grant()
         if (shouldStore) {
-            storeSitePermissions(session, permissionRequest, ALLOWED)
+            getCurrentContentState()?.let { contentState ->
+                storeSitePermissions(contentState, permissionRequest, ALLOWED)
+            }
         }
-        consumePermissionRequest(permissionRequest)
     }
 
     internal fun onPositiveButtonPress(
         permissionRequest: PermissionRequest,
         sessionId: String,
         shouldStore: Boolean
     ) {
-        sessionManager.runWithSession(sessionId) { session ->
-            consumePermissionRequest(permissionRequest)
-            onContentPermissionGranted(session, permissionRequest, shouldStore)
-            true
-        }
+        consumePermissionRequest(permissionRequest, sessionId)
+        onContentPermissionGranted(permissionRequest, shouldStore)
     }
 
     internal fun onNegativeButtonPress(
         permissionRequest: PermissionRequest,
         sessionId: String,
         shouldStore: Boolean
     ) {
-        sessionManager.runWithSession(sessionId) { session ->
-            onContentPermissionDeny(permissionRequest, session, shouldStore)
-            consumePermissionRequest(permissionRequest)
-            true
-        }
+        consumePermissionRequest(permissionRequest, sessionId)
+        onContentPermissionDeny(permissionRequest, shouldStore)
     }
 
     internal fun onDismiss(
         permissionRequest: PermissionRequest,
         sessionId: String
     ) {
-        sessionManager.runWithSession(sessionId) { session ->
-            consumePermissionRequest(permissionRequest)
-            onContentPermissionDeny(permissionRequest, session, false)
-            true
-        }
+        consumePermissionRequest(permissionRequest, sessionId)
+        onContentPermissionDeny(permissionRequest, false)
     }
 
     internal fun storeSitePermissions(
-        session: Session,
+        contentState: ContentState,
         request: PermissionRequest,
         status: SitePermissions.Status
     ) {
-        if (session.private) {
+        if (contentState.private) {
             return
         }
         ioCoroutineScope.launch {
             synchronized(storage) {
+                val host = contentState.url.tryGetHostFromUrl()
                 var sitePermissions =
-                    storage.findSitePermissionsBy(request.getHost(session))
+                    storage.findSitePermissionsBy(host)
 
                 if (sitePermissions == null) {
                     sitePermissions =
                         request.toSitePermissions(
-                            session,
+                            host,
                             status = status,
                             permissions = request.permissions
                         )
                     storage.save(sitePermissions)
                 } else {
-                    sitePermissions = request.toSitePermissions(session, status, sitePermissions)
+                    sitePermissions = request.toSitePermissions(host, status, sitePermissions)
                     storage.update(sitePermissions)
                 }
             }
@@ -301,14 +305,14 @@ class SitePermissionsFeature(
      */
     internal fun onContentPermissionDeny(
         permissionRequest: PermissionRequest,
-        session: Session,
         shouldStore: Boolean
     ) {
         permissionRequest.reject()
         if (shouldStore) {
-            storeSitePermissions(session, request = permissionRequest, status = BLOCKED)
+            getCurrentContentState()?.let { contentState ->
+                storeSitePermissions(contentState, permissionRequest, BLOCKED)
+            }
         }
-        consumePermissionRequest(permissionRequest)
     }
 
     internal suspend fun onContentPermissionRequested(
@@ -422,9 +426,9 @@ class SitePermissionsFeature(
     }
 
     private fun PermissionRequest.toSitePermissions(
-        session: Session,
+        host: String,
         status: SitePermissions.Status,
-        initialSitePermission: SitePermissions = getInitialSitePermissions(session, this),
+        initialSitePermission: SitePermissions = getInitialSitePermissions(host/*, this*/),
         permissions: List<Permission> = this.permissions
     ): SitePermissions {
         var sitePermissions = initialSitePermission
@@ -435,15 +439,15 @@ class SitePermissionsFeature(
     }
 
     internal fun getInitialSitePermissions(
-        session: Session,
-        request: PermissionRequest
+        host: String/*,
+        request: PermissionRequest*/
     ): SitePermissions {
         val rules = sitePermissionsRules
         return rules?.toSitePermissions(
-            request.getHost(session),
+            host,
             savedAt = System.currentTimeMillis()
         )
-            ?: SitePermissions(request.getHost(session), savedAt = System.currentTimeMillis())
+            ?: SitePermissions(host, savedAt = System.currentTimeMillis())
     }
 
     private fun PermissionRequest.isForAutoplay() =