Closes mozilla-mobile#6893 Fulfill the foreground service contract on…

… AbstractFetchDownloadService

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

@@ -59,6 +59,7 @@ import mozilla.components.feature.downloads.facts.emitNotificationPauseFact
 import mozilla.components.feature.downloads.facts.emitNotificationCancelFact
 import mozilla.components.feature.downloads.facts.emitNotificationTryAgainFact
 import mozilla.components.feature.downloads.facts.emitNotificationOpenFact
+import mozilla.components.support.base.log.logger.Logger
 import mozilla.components.support.utils.DownloadUtils
 import java.io.File
 import java.io.FileOutputStream
@@ -77,7 +78,8 @@ import kotlin.random.Random
 @Suppress("TooManyFunctions", "LargeClass")
 abstract class AbstractFetchDownloadService : Service() {
 
-    private var notificationUpdateScope = MainScope()
+    private val notificationUpdateScope = MainScope()
+    private val logger = Logger("AbstractFetchDownloadService")
 
     protected abstract val httpClient: Client
     @VisibleForTesting
@@ -225,6 +227,7 @@ abstract class AbstractFetchDownloadService : Service() {
 
     override fun onCreate() {
         super.onCreate()
+        logger.info("onCreate")
         registerNotificationActionsReceiver()
     }
 
@@ -247,15 +250,9 @@ abstract class AbstractFetchDownloadService : Service() {
 
         setForegroundNotification(downloadJobState)
 
+        logger.info("notificationUpdateScope.isActive ${notificationUpdateScope.isActive}")
         downloadJobs[download.id] = downloadJobState
 
-        // We are canceling the notificationUpdateScope, because we don't want to keep it
-        // running when we don't have more downloads to process. As a result, we need to
-        // create a new one when there are more downloads to process and the scope is canceled
-        if (!notificationUpdateScope.isActive) {
-            notificationUpdateScope = MainScope()
-        }
-
         notificationUpdateScope.launch {
             while (isActive) {
                 delay(PROGRESS_UPDATE_INTERVAL)
@@ -323,12 +320,13 @@ abstract class AbstractFetchDownloadService : Service() {
         // service gets killed. In this situation we remove the notification since the download will
         // stop and cannot be controlled via the notification anymore (until we persist enough data
         // to resume downloads from a new process).
-
-        clearAllDownloadsNotificationsAndJobs()
+        logger.info("onTaskRemoved")
+        stopSelf()
     }
 
     override fun onDestroy() {
         super.onDestroy()
+        logger.info("onDestroy")
 
         clearAllDownloadsNotificationsAndJobs()
         unregisterNotificationActionsReceiver()
@@ -385,12 +383,9 @@ abstract class AbstractFetchDownloadService : Service() {
     }
 
     @VisibleForTesting
-    internal open fun removeDownloadJob(downloadJobState: DownloadJobState) {
+    internal fun removeDownloadJob(downloadJobState: DownloadJobState) {
         downloadJobs.remove(downloadJobState.state.id)
         if (downloadJobs.isEmpty()) {
-            stopForeground(false)
-            NotificationManagerCompat.from(context).cancel(getForegroundId())
-            clearAllDownloadsNotificationsAndJobs()
             stopSelf()
         } else {
             updateForegroundNotificationIfNeeded(downloadJobState)
@@ -445,14 +440,11 @@ abstract class AbstractFetchDownloadService : Service() {
     internal fun setForegroundNotification(downloadJobState: DownloadJobState) {
         var previousDownload: DownloadJobState? = null
 
-        // Ongoing notifications can't be deleted by calling startForeground
-        val isOngoing: ((DownloadJobStatus) -> Boolean) = { status -> status == ACTIVE || status == PAUSED }
-
         val (notificationId, notification) = if (SDK_INT >= Build.VERSION_CODES.N) {
             NOTIFICATION_DOWNLOAD_GROUP_ID to updateNotificationGroup()
         } else {
             previousDownload = downloadJobs.values.firstOrNull {
-                it.foregroundServiceId == compatForegroundNotificationId && !isOngoing(it.status)
+                it.foregroundServiceId == compatForegroundNotificationId
             }
             downloadJobState.foregroundServiceId to createCompactForegroundNotification(downloadJobState)
         }

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

@@ -54,15 +54,15 @@ internal object DownloadNotification {
         } else {
             R.drawable.mozac_feature_download_ic_ongoing_download
         }
-        val downloadSummary = getDownloadSummary(context, notifications)
-        val summaryLineArray = downloadSummary.split("\n")
-        val summaryLine1 = summaryLineArray.first()
-        val summaryLine2 = if (summaryLineArray.size == 2) summaryLineArray[1] else ""
+        val summaryList = getSummaryList(context, notifications)
+        val summaryLine1 = summaryList.first()
+        val summaryLine2 = if (summaryList.size == 2) summaryList[1] else ""
 
         return NotificationCompat.Builder(context, ensureChannelExists(context))
             .setSmallIcon(icon)
+            .setColor(ContextCompat.getColor(context, R.color.mozac_feature_downloads_notification))
             .setContentTitle(context.getString(R.string.mozac_feature_downloads_notification_channel))
-            .setContentText(downloadSummary)
+            .setContentText(summaryList.joinToString("\n"))
             .setStyle(NotificationCompat.InboxStyle().addLine(summaryLine1).addLine(summaryLine2))
             .setGroup(NOTIFICATION_GROUP_KEY)
             .setGroupSummary(true)
@@ -167,9 +167,8 @@ internal object DownloadNotification {
     }
 
     @VisibleForTesting
-    internal fun getDownloadSummary(context: Context, notifications: List<DownloadJobState>): String {
-        val summaryList = notifications.take(2)
-        return summaryList.joinToString("\n") { downloadState ->
+    internal fun getSummaryList(context: Context, notifications: List<DownloadJobState>): List<String> {
+        return notifications.take(2).map { downloadState ->
             "${downloadState.state.fileName} ${downloadState.getStatusDescription(context)}"
         }
     }

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

@@ -294,15 +294,6 @@ class AbstractFetchDownloadServiceTest {
 
     @Test
     fun `broadcastReceiver handles ACTION_CANCEL`() = runBlocking {
-        val service = spy(object : AbstractFetchDownloadService() {
-            override val httpClient = client
-            override fun removeDownloadJob(downloadJobState: DownloadJobState) = Unit
-        })
-
-        doReturn(broadcastManager).`when`(service).broadcastManager
-        doReturn(testContext).`when`(service).context
-        doNothing().`when`(service).useFileStream(any(), anyBoolean(), any())
-
         val download = DownloadState("https://example.com/file.txt", "file.txt")
         val response = Response(
             "https://example.com/file.txt",
@@ -702,8 +693,6 @@ class AbstractFetchDownloadServiceTest {
         service.removeDownloadJob(downloadJobState = downloadState)
 
         assertTrue(service.downloadJobs.isEmpty())
-        verify(service).stopForeground(false)
-        verify(service).clearAllDownloadsNotificationsAndJobs()
         verify(service).stopSelf()
         verify(service, times(0)).updateForegroundNotificationIfNeeded(downloadState)
     }
@@ -993,8 +982,7 @@ class AbstractFetchDownloadServiceTest {
         // Now simulate onTaskRemoved.
         service.onTaskRemoved(null)
 
-        // Assert that all currently shown notifications are gone.
-        assertEquals(0, shadowNotificationService.size())
+        verify(service).stopSelf()
     }
 
     @Test
@@ -1030,22 +1018,19 @@ class AbstractFetchDownloadServiceTest {
     }
 
     @Test
-    fun `onTaskRemoved and onDestroy will remove all download notifications and jobs`() = runBlocking {
+    fun `onDestroy will remove all download notifications, jobs and will call unregisterNotificationActionsReceiver`() = runBlocking {
         val service = spy(object : AbstractFetchDownloadService() {
             override val httpClient = client
         })
 
         doReturn(testContext).`when`(service).context
 
         service.registerNotificationActionsReceiver()
-        service.onTaskRemoved(null)
-
-        service.registerNotificationActionsReceiver()
-        verify(service).clearAllDownloadsNotificationsAndJobs()
 
         service.onDestroy()
 
-        verify(service, times(2)).clearAllDownloadsNotificationsAndJobs()
+        verify(service).clearAllDownloadsNotificationsAndJobs()
+        verify(service).unregisterNotificationActionsReceiver()
     }
 
     @Test

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

@@ -105,7 +105,7 @@ class DownloadNotificationTest {
                 downloadDeleted = false
         )
 
-        val summary = DownloadNotification.getDownloadSummary(testContext, listOf(download1, download2))
-        assertEquals("mozilla.txt 10%\nmozilla2.txt 20%", summary)
+        val summary = DownloadNotification.getSummaryList(testContext, listOf(download1, download2))
+        assertEquals(listOf("mozilla.txt 10%", "mozilla2.txt 20%"), summary)
     }
 }