Closes #9757 Remove downloads notification when private tabs are closed

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

@@ -962,6 +962,11 @@ sealed class DownloadAction : BrowserAction() {
      */
     data class UpdateDownloadAction(val download: DownloadState) : DownloadAction()
 
+    /**
+     * Mark the download notification of the provided [downloadId] as removed from the status bar.
+     */
+    data class DismissDownloadNotificationAction(val downloadId: String) : DownloadAction()
+
     /**
      * Restores the [BrowserState.downloads] state from the storage.
      */

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

@@ -19,6 +19,14 @@ internal object DownloadStateReducer {
             is DownloadAction.UpdateDownloadAction -> {
                 updateDownloads(state, action.download)
             }
+            is DownloadAction.DismissDownloadNotificationAction -> {
+                val download = state.downloads[action.downloadId]
+                if (download != null) {
+                    updateDownloads(state, download.copy(notificationId = null))
+                } else {
+                    state
+                }
+            }
             is DownloadAction.RemoveDownloadAction -> {
                 state.copy(downloads = state.downloads - action.downloadId)
             }

components/browser/state/src/main/java/mozilla/components/browser/state/state/content/DownloadState.kt

@@ -28,6 +28,8 @@ import java.util.UUID
  * @property createdTime A timestamp when the download was created.
  * @property response A response object associated with this request, when provided can be
  * used instead of performing a manual a download.
+ * @property notificationId Identifies the download notification in the status bar, if this
+ * [DownloadState] has one otherwise null.
  */
 @Suppress("Deprecation")
 data class DownloadState(
@@ -45,7 +47,8 @@ data class DownloadState(
     val sessionId: String? = null,
     val private: Boolean = false,
     val createdTime: Long = System.currentTimeMillis(),
-    val response: Response? = null
+    val response: Response? = null,
+    val notificationId: Int? = null
 ) {
     val filePath: String get() =
         Environment.getExternalStoragePublicDirectory(destinationDirectory).path + File.separatorChar + fileName

components/browser/state/src/test/java/mozilla/components/browser/state/action/DownloadActionTest.kt

@@ -10,6 +10,8 @@ import mozilla.components.browser.state.store.BrowserStore
 import mozilla.components.support.test.ext.joinBlocking
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertFalse
+import org.junit.Assert.assertNotNull
+import org.junit.Assert.assertNull
 import org.junit.Assert.assertTrue
 import org.junit.Assert.assertSame
 import org.junit.Test
@@ -31,6 +33,32 @@ class DownloadActionTest {
         assertEquals(2, store.state.downloads.size)
     }
 
+    @Test
+    fun `WHEN DismissDownloadNotificationAction is dispatched THEN notificationId is set to null`() {
+        val store = BrowserStore(BrowserState())
+
+        val download = DownloadState("https://mozilla.org/download1", destinationDirectory = "", notificationId = 100)
+        store.dispatch(DownloadAction.AddDownloadAction(download)).joinBlocking()
+        assertNotNull(store.state.downloads[download.id]!!.notificationId)
+
+        store.dispatch(DownloadAction.DismissDownloadNotificationAction(download.id)).joinBlocking()
+        assertNull(store.state.downloads[download.id]!!.notificationId)
+    }
+
+    @Test
+    fun `WHEN DismissDownloadNotificationAction is dispatched with an invalid downloadId THEN the state must not change`() {
+        val store = BrowserStore(BrowserState())
+
+        val download = DownloadState("https://mozilla.org/download1", destinationDirectory = "", notificationId = 100)
+        store.dispatch(DownloadAction.AddDownloadAction(download)).joinBlocking()
+        assertNotNull(store.state.downloads[download.id]!!.notificationId)
+        assertEquals(1, store.state.downloads.size)
+
+        store.dispatch(DownloadAction.DismissDownloadNotificationAction("-1")).joinBlocking()
+        assertNotNull(store.state.downloads[download.id]!!.notificationId)
+        assertEquals(download, store.state.downloads[download.id])
+    }
+
     @Test
     fun `RestoreDownloadStateAction adds download`() {
         val store = BrowserStore(BrowserState())

components/feature/downloads/src/main/java/mozilla/components/feature/downloads/AbstractFetchDownloadService.kt

@@ -256,14 +256,35 @@ abstract class AbstractFetchDownloadService : Service() {
             store.state.downloads[it]
         } ?: return START_REDELIVER_INTENT
 
+        if (intent.action == ACTION_REMOVE_PRIVATE_DOWNLOAD) {
+            handleRemovePrivateDownloadIntent(download)
+        } else {
+            handleDownloadIntent(download)
+        }
+
+        return super.onStartCommand(intent, flags, startId)
+    }
+
+    @VisibleForTesting
+    internal fun handleRemovePrivateDownloadIntent(download: DownloadState) {
+        if (download.private) {
+            downloadJobs[download.id]?.let {
+                removeDownloadJob(it)
+            }
+            store.dispatch(DownloadAction.RemoveDownloadAction(download.id))
+        }
+    }
+
+    @VisibleForTesting
+    internal fun handleDownloadIntent(download: DownloadState) {
         // If the job already exists, then don't create a new ID. This can happen when calling tryAgain
         val foregroundServiceId = downloadJobs[download.id]?.foregroundServiceId ?: Random.nextInt()
 
         val actualStatus = if (download.status == INITIATED) DOWNLOADING else download.status
 
         // Create a new job and add it, with its downloadState to the map
         val downloadJobState = DownloadJobState(
-            state = download.copy(status = actualStatus),
+            state = download.copy(status = actualStatus, notificationId = foregroundServiceId),
             foregroundServiceId = foregroundServiceId,
             status = actualStatus
         )
@@ -287,8 +308,6 @@ abstract class AbstractFetchDownloadService : Service() {
                 if (downloadJobs.isEmpty()) cancel()
             }
         }
-
-        return super.onStartCommand(intent, flags, startId)
     }
 
     /**
@@ -1002,6 +1021,7 @@ abstract class AbstractFetchDownloadService : Service() {
         const val ACTION_RESUME = "mozilla.components.feature.downloads.RESUME"
         const val ACTION_CANCEL = "mozilla.components.feature.downloads.CANCEL"
         const val ACTION_DISMISS = "mozilla.components.feature.downloads.DISMISS"
+        const val ACTION_REMOVE_PRIVATE_DOWNLOAD = "mozilla.components.feature.downloads.ACTION_REMOVE_PRIVATE_DOWNLOAD"
         const val ACTION_TRY_AGAIN = "mozilla.components.feature.downloads.TRY_AGAIN"
         const val COMPAT_DEFAULT_FOREGROUND_ID = -1
     }

components/feature/downloads/src/main/java/mozilla/components/feature/downloads/DownloadMiddleware.kt

@@ -15,11 +15,13 @@ import kotlinx.coroutines.InternalCoroutinesApi
 import kotlinx.coroutines.launch
 import mozilla.components.browser.state.action.BrowserAction
 import mozilla.components.browser.state.action.DownloadAction
+import mozilla.components.browser.state.action.TabListAction
 import mozilla.components.browser.state.state.BrowserState
 import mozilla.components.browser.state.state.content.DownloadState
 import mozilla.components.browser.state.state.content.DownloadState.Status.CANCELLED
 import mozilla.components.browser.state.state.content.DownloadState.Status.COMPLETED
 import mozilla.components.browser.state.state.content.DownloadState.Status.FAILED
+import mozilla.components.feature.downloads.AbstractFetchDownloadService.Companion.ACTION_REMOVE_PRIVATE_DOWNLOAD
 import mozilla.components.lib.state.Middleware
 import mozilla.components.lib.state.MiddlewareContext
 import mozilla.components.lib.state.Store
@@ -31,7 +33,7 @@ import kotlin.coroutines.CoroutineContext
  * purpose is to react to global download state changes (e.g. of [BrowserState.downloads])
  * and notify the download service, as needed.
  */
-@Suppress("ComplexMethod")
+@Suppress("ComplexMethod", "TooManyFunctions")
 class DownloadMiddleware(
     private val applicationContext: Context,
     private val downloadServiceClass: Class<*>,
@@ -67,6 +69,10 @@ class DownloadMiddleware(
         next(action)
 
         when (action) {
+            is TabListAction.RemoveAllTabsAction,
+            is TabListAction.RemoveAllPrivateTabsAction -> removePrivateNotifications(context.store)
+            is TabListAction.RemoveTabsAction -> removePrivateNotifications(context.store, action.tabIds)
+            is TabListAction.RemoveTabAction -> removePrivateNotifications(context.store, listOf(action.tabId))
             is DownloadAction.AddDownloadAction -> sendDownloadIntent(action.download)
             is DownloadAction.RestoreDownloadStateAction -> sendDownloadIntent(action.download)
         }
@@ -136,4 +142,27 @@ class DownloadMiddleware(
     internal fun startForegroundService(intent: Intent) {
         ContextCompat.startForegroundService(applicationContext, intent)
     }
+
+    @VisibleForTesting
+    internal fun removeStatusBarNotification(store: Store<BrowserState, BrowserAction>, download: DownloadState) {
+        download.notificationId?.let {
+            val intent = Intent(applicationContext, downloadServiceClass)
+            intent.action = ACTION_REMOVE_PRIVATE_DOWNLOAD
+            intent.putExtra(DownloadManager.EXTRA_DOWNLOAD_ID, download.id)
+            applicationContext.startService(intent)
+            store.dispatch(DownloadAction.DismissDownloadNotificationAction(download.id))
+        }
+    }
+
+    @VisibleForTesting
+    internal fun removePrivateNotifications(store: Store<BrowserState, BrowserAction>) {
+        val privateDownloads = store.state.downloads.filterValues { it.private }
+        privateDownloads.forEach { removeStatusBarNotification(store, it.value) }
+    }
+
+    @VisibleForTesting
+    internal fun removePrivateNotifications(store: Store<BrowserState, BrowserAction>, tabIds: List<String>) {
+        val privateDownloads = store.state.downloads.filterValues { it.sessionId in tabIds && it.private }
+        privateDownloads.forEach { removeStatusBarNotification(store, it.value) }
+    }
 }

components/feature/downloads/src/test/java/mozilla/components/feature/downloads/AbstractFetchDownloadServiceTest.kt

@@ -41,6 +41,7 @@ import mozilla.components.concept.fetch.Request
 import mozilla.components.concept.fetch.Response
 import mozilla.components.feature.downloads.AbstractFetchDownloadService.Companion.ACTION_CANCEL
 import mozilla.components.feature.downloads.AbstractFetchDownloadService.Companion.ACTION_PAUSE
+import mozilla.components.feature.downloads.AbstractFetchDownloadService.Companion.ACTION_REMOVE_PRIVATE_DOWNLOAD
 import mozilla.components.feature.downloads.AbstractFetchDownloadService.Companion.ACTION_RESUME
 import mozilla.components.feature.downloads.AbstractFetchDownloadService.Companion.ACTION_TRY_AGAIN
 import mozilla.components.feature.downloads.AbstractFetchDownloadService.Companion.PROGRESS_UPDATE_INTERVAL
@@ -160,6 +161,100 @@ class AbstractFetchDownloadServiceTest {
         assertNotNull(service.downloadJobs[providedDownload.value.state.id])
     }
 
+    @Test
+    fun `WHEN a download intent is received THEN handleDownloadIntent must be called`() = runBlocking {
+        val download = DownloadState("https://example.com/file.txt", "file.txt")
+        val downloadIntent = Intent("ACTION_DOWNLOAD")
+
+        doNothing().`when`(service).handleRemovePrivateDownloadIntent(any())
+        doNothing().`when`(service).handleDownloadIntent(any())
+
+        downloadIntent.putExtra(EXTRA_DOWNLOAD_ID, download.id)
+
+        browserStore.dispatch(DownloadAction.AddDownloadAction(download)).joinBlocking()
+
+        service.onStartCommand(downloadIntent, 0, 0)
+
+        verify(service).handleDownloadIntent(download)
+        verify(service, never()).handleRemovePrivateDownloadIntent(download)
+    }
+
+    @Test
+    fun `WHEN an intent does not provide an action THEN handleDownloadIntent must be called`() = runBlocking {
+        val download = DownloadState("https://example.com/file.txt", "file.txt")
+        val downloadIntent = Intent()
+
+        doNothing().`when`(service).handleRemovePrivateDownloadIntent(any())
+        doNothing().`when`(service).handleDownloadIntent(any())
+
+        downloadIntent.putExtra(EXTRA_DOWNLOAD_ID, download.id)
+
+        browserStore.dispatch(DownloadAction.AddDownloadAction(download)).joinBlocking()
+
+        service.onStartCommand(downloadIntent, 0, 0)
+
+        verify(service).handleDownloadIntent(download)
+        verify(service, never()).handleRemovePrivateDownloadIntent(download)
+    }
+
+    @Test
+    fun `WHEN a remove download intent is received THEN handleRemoveDownloadIntent must be called`() = runBlocking {
+        val download = DownloadState("https://example.com/file.txt", "file.txt")
+        val downloadIntent = Intent(ACTION_REMOVE_PRIVATE_DOWNLOAD)
+
+        doNothing().`when`(service).handleRemovePrivateDownloadIntent(any())
+        doNothing().`when`(service).handleDownloadIntent(any())
+
+        downloadIntent.putExtra(EXTRA_DOWNLOAD_ID, download.id)
+
+        browserStore.dispatch(DownloadAction.AddDownloadAction(download)).joinBlocking()
+
+        service.onStartCommand(downloadIntent, 0, 0)
+
+        verify(service).handleRemovePrivateDownloadIntent(download)
+        verify(service, never()).handleDownloadIntent(download)
+    }
+
+    @Test
+    fun `WHEN handleRemovePrivateDownloadIntent with a privae download is called THEN removeDownloadJob must be called`() {
+        val downloadState = DownloadState(url = "mozilla.org/mozilla.txt", private = true)
+        val downloadJobState = DownloadJobState(state = downloadState, status = COMPLETED)
+        val browserStore = mock<BrowserStore>()
+        val service = spy(object : AbstractFetchDownloadService() {
+            override val httpClient = client
+            override val store = browserStore
+        })
+
+        doAnswer { Unit }.`when`(service).removeDownloadJob(any())
+
+        service.downloadJobs[downloadState.id] = downloadJobState
+
+        service.handleRemovePrivateDownloadIntent(downloadState)
+
+        verify(service).removeDownloadJob(downloadJobState)
+        verify(browserStore).dispatch(DownloadAction.RemoveDownloadAction(downloadState.id))
+    }
+
+    @Test
+    fun `WHEN handleRemovePrivateDownloadIntent is called with with a non-private (or regular) download THEN removeDownloadJob must not be called`() {
+        val downloadState = DownloadState(url = "mozilla.org/mozilla.txt", private = false)
+        val downloadJobState = DownloadJobState(state = downloadState, status = COMPLETED)
+        val browserStore = mock<BrowserStore>()
+        val service = spy(object : AbstractFetchDownloadService() {
+            override val httpClient = client
+            override val store = browserStore
+        })
+
+        doAnswer { Unit }.`when`(service).removeDownloadJob(any())
+
+        service.downloadJobs[downloadState.id] = downloadJobState
+
+        service.handleRemovePrivateDownloadIntent(downloadState)
+
+        verify(service, never()).removeDownloadJob(downloadJobState)
+        verify(browserStore, never()).dispatch(DownloadAction.RemoveDownloadAction(downloadState.id))
+    }
+
     @Test
     fun `service redelivers if no download extra is passed `() = runBlocking {
         val downloadIntent = Intent("ACTION_DOWNLOAD")