From afbfbde7f7e378b1b18ff3f401c16044f7feb195 Mon Sep 17 00:00:00 2001 From: Christian Sadilek Date: Tue, 26 May 2020 17:44:16 -0400 Subject: [PATCH] Closes #7103 #5217: DownloadService: Read initial download state from store --- .../browser/state/action/BrowserAction.kt | 20 +++ .../state/reducer/BrowserStateReducer.kt | 2 + .../state/reducer/DownloadStateReducer.kt | 28 ++++ .../browser/state/state/BrowserState.kt | 5 +- .../state/action/DownloadActionTest.kt | 61 +++++++++ .../downloads/AbstractFetchDownloadService.kt | 13 +- .../feature/downloads/DownloadMiddleware.kt | 40 ++++++ .../feature/downloads/DownloadsFeature.kt | 4 +- .../feature/downloads/ext/Intent.kt | 54 -------- .../manager/AndroidDownloadManager.kt | 41 +++--- .../downloads/manager/FetchDownloadManager.kt | 33 +++-- .../AbstractFetchDownloadServiceTest.kt | 123 +++++++++--------- .../downloads/DownloadMiddlewareTest.kt | 41 ++++++ .../manager/AndroidDownloadManagerTest.kt | 62 ++++++--- .../manager/FetchDownloadManagerTest.kt | 113 +++++++++------- docs/changelog.md | 35 +++++ .../samples/browser/BaseBrowserFragment.kt | 1 + .../samples/browser/DefaultComponents.kt | 3 + .../browser/downloads/DownloadService.kt | 2 + 19 files changed, 455 insertions(+), 226 deletions(-) create mode 100644 components/browser/state/src/main/java/mozilla/components/browser/state/reducer/DownloadStateReducer.kt create mode 100644 components/browser/state/src/test/java/mozilla/components/browser/state/action/DownloadActionTest.kt create mode 100644 components/feature/downloads/src/main/java/mozilla/components/feature/downloads/DownloadMiddleware.kt delete mode 100644 components/feature/downloads/src/main/java/mozilla/components/feature/downloads/ext/Intent.kt create mode 100644 components/feature/downloads/src/test/java/mozilla/components/feature/downloads/DownloadMiddlewareTest.kt diff --git a/components/browser/state/src/main/java/mozilla/components/browser/state/action/BrowserAction.kt b/components/browser/state/src/main/java/mozilla/components/browser/state/action/BrowserAction.kt index 3ec25e49167..c977635e9e0 100644 --- a/components/browser/state/src/main/java/mozilla/components/browser/state/action/BrowserAction.kt +++ b/components/browser/state/src/main/java/mozilla/components/browser/state/action/BrowserAction.kt @@ -558,3 +558,23 @@ sealed class MediaAction : BrowserAction() { val aggregate: MediaState.Aggregate ) : MediaAction() } + +/** + * [BrowserAction] implementations related to updating the global download state. + */ +sealed class DownloadAction : BrowserAction() { + /** + * Updates the [BrowserState] to track the provided [download] as queued. + */ + data class QueueDownloadAction(val download: DownloadState) : DownloadAction() + + /** + * Updates the [BrowserState] to remove the queued download with the provided [downloadId]. + */ + data class RemoveQueuedDownloadAction(val downloadId: Long) : DownloadAction() + + /** + * Updates the [BrowserState] to remove all queued downloads. + */ + object RemoveAllQueuedDownloadsAction : DownloadAction() +} diff --git a/components/browser/state/src/main/java/mozilla/components/browser/state/reducer/BrowserStateReducer.kt b/components/browser/state/src/main/java/mozilla/components/browser/state/reducer/BrowserStateReducer.kt index b4bea1405aa..1eaab8165c8 100644 --- a/components/browser/state/src/main/java/mozilla/components/browser/state/reducer/BrowserStateReducer.kt +++ b/components/browser/state/src/main/java/mozilla/components/browser/state/reducer/BrowserStateReducer.kt @@ -7,6 +7,7 @@ package mozilla.components.browser.state.reducer import mozilla.components.browser.state.action.BrowserAction import mozilla.components.browser.state.action.ContentAction import mozilla.components.browser.state.action.CustomTabListAction +import mozilla.components.browser.state.action.DownloadAction import mozilla.components.browser.state.action.EngineAction import mozilla.components.browser.state.action.MediaAction import mozilla.components.browser.state.action.ReaderAction @@ -39,6 +40,7 @@ internal object BrowserStateReducer { is TrackingProtectionAction -> TrackingProtectionStateReducer.reduce(state, action) is WebExtensionAction -> WebExtensionReducer.reduce(state, action) is MediaAction -> MediaReducer.reduce(state, action) + is DownloadAction -> DownloadStateReducer.reduce(state, action) } } } diff --git a/components/browser/state/src/main/java/mozilla/components/browser/state/reducer/DownloadStateReducer.kt b/components/browser/state/src/main/java/mozilla/components/browser/state/reducer/DownloadStateReducer.kt new file mode 100644 index 00000000000..af625861944 --- /dev/null +++ b/components/browser/state/src/main/java/mozilla/components/browser/state/reducer/DownloadStateReducer.kt @@ -0,0 +1,28 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package mozilla.components.browser.state.reducer + +import mozilla.components.browser.state.action.DownloadAction +import mozilla.components.browser.state.state.BrowserState + +internal object DownloadStateReducer { + + /** + * [DownloadAction] Reducer function for modifying [BrowserState.queuedDownloads]. + */ + fun reduce(state: BrowserState, action: DownloadAction): BrowserState { + return when (action) { + is DownloadAction.QueueDownloadAction -> { + state.copy(queuedDownloads = state.queuedDownloads + (action.download.id to action.download)) + } + is DownloadAction.RemoveQueuedDownloadAction -> { + state.copy(queuedDownloads = state.queuedDownloads - action.downloadId) + } + is DownloadAction.RemoveAllQueuedDownloadsAction -> { + state.copy(queuedDownloads = emptyMap()) + } + } + } +} diff --git a/components/browser/state/src/main/java/mozilla/components/browser/state/state/BrowserState.kt b/components/browser/state/src/main/java/mozilla/components/browser/state/state/BrowserState.kt index 6f10e738393..36859f6f3ea 100644 --- a/components/browser/state/src/main/java/mozilla/components/browser/state/state/BrowserState.kt +++ b/components/browser/state/src/main/java/mozilla/components/browser/state/state/BrowserState.kt @@ -4,6 +4,7 @@ package mozilla.components.browser.state.state +import mozilla.components.browser.state.state.content.DownloadState import mozilla.components.lib.state.State /** @@ -16,11 +17,13 @@ import mozilla.components.lib.state.State * The extensions here represent the default values for all [BrowserState.extensions] and can * be overridden per [SessionState]. * @property media The state of all media elements and playback states for all tabs. + * @property queuedDownloads queued downloads ([DownloadState]s) mapped to their IDs. */ data class BrowserState( val tabs: List = emptyList(), val selectedTabId: String? = null, val customTabs: List = emptyList(), val extensions: Map = emptyMap(), - val media: MediaState = MediaState() + val media: MediaState = MediaState(), + val queuedDownloads: Map = emptyMap() ) : State diff --git a/components/browser/state/src/test/java/mozilla/components/browser/state/action/DownloadActionTest.kt b/components/browser/state/src/test/java/mozilla/components/browser/state/action/DownloadActionTest.kt new file mode 100644 index 00000000000..a22f4cab6ec --- /dev/null +++ b/components/browser/state/src/test/java/mozilla/components/browser/state/action/DownloadActionTest.kt @@ -0,0 +1,61 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package mozilla.components.browser.state.action + +import mozilla.components.browser.state.state.BrowserState +import mozilla.components.browser.state.state.content.DownloadState +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.assertTrue +import org.junit.Test + +class DownloadActionTest { + + @Test + fun `QueueDownloadAction adds download`() { + val store = BrowserStore(BrowserState()) + + val download1 = DownloadState("https://mozilla.org/download1", destinationDirectory = "") + store.dispatch(DownloadAction.QueueDownloadAction(download1)).joinBlocking() + assertEquals(download1, store.state.queuedDownloads[download1.id]) + assertEquals(1, store.state.queuedDownloads.size) + + val download2 = DownloadState("https://mozilla.org/download2", destinationDirectory = "") + store.dispatch(DownloadAction.QueueDownloadAction(download2)).joinBlocking() + assertEquals(download2, store.state.queuedDownloads[download2.id]) + assertEquals(2, store.state.queuedDownloads.size) + } + + @Test + fun `RemoveQueuedDownloadAction removes download`() { + val store = BrowserStore(BrowserState()) + + val download = DownloadState("https://mozilla.org/download1", destinationDirectory = "") + store.dispatch(DownloadAction.QueueDownloadAction(download)).joinBlocking() + assertEquals(download, store.state.queuedDownloads[download.id]) + assertFalse(store.state.queuedDownloads.isEmpty()) + + store.dispatch(DownloadAction.RemoveQueuedDownloadAction(download.id)).joinBlocking() + assertTrue(store.state.queuedDownloads.isEmpty()) + } + + @Test + fun `RemoveAllQueuedDownloadsAction removes all downloads`() { + val store = BrowserStore(BrowserState()) + + val download = DownloadState("https://mozilla.org/download1", destinationDirectory = "") + val download2 = DownloadState("https://mozilla.org/download2", destinationDirectory = "") + store.dispatch(DownloadAction.QueueDownloadAction(download)).joinBlocking() + store.dispatch(DownloadAction.QueueDownloadAction(download2)).joinBlocking() + + assertFalse(store.state.queuedDownloads.isEmpty()) + assertEquals(2, store.state.queuedDownloads.size) + + store.dispatch(DownloadAction.RemoveAllQueuedDownloadsAction).joinBlocking() + assertTrue(store.state.queuedDownloads.isEmpty()) + } +} diff --git a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/AbstractFetchDownloadService.kt b/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/AbstractFetchDownloadService.kt index 924b7d0622b..610ee61192e 100644 --- a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/AbstractFetchDownloadService.kt +++ b/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/AbstractFetchDownloadService.kt @@ -40,7 +40,9 @@ import kotlinx.coroutines.cancel import kotlinx.coroutines.delay import kotlinx.coroutines.isActive import kotlinx.coroutines.launch +import mozilla.components.browser.state.action.DownloadAction import mozilla.components.browser.state.state.content.DownloadState +import mozilla.components.browser.state.store.BrowserStore import mozilla.components.concept.fetch.Client import mozilla.components.concept.fetch.Headers.Names.CONTENT_RANGE import mozilla.components.concept.fetch.Headers.Names.RANGE @@ -53,7 +55,6 @@ import mozilla.components.feature.downloads.AbstractFetchDownloadService.Downloa import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobStatus.PAUSED import mozilla.components.feature.downloads.DownloadNotification.NOTIFICATION_DOWNLOAD_GROUP_ID import mozilla.components.feature.downloads.ext.addCompletedDownload -import mozilla.components.feature.downloads.ext.getDownloadExtra import mozilla.components.feature.downloads.ext.withResponse import mozilla.components.feature.downloads.facts.emitNotificationResumeFact import mozilla.components.feature.downloads.facts.emitNotificationPauseFact @@ -77,6 +78,7 @@ import kotlin.random.Random */ @Suppress("TooManyFunctions", "LargeClass") abstract class AbstractFetchDownloadService : Service() { + protected abstract val store: BrowserStore private val notificationUpdateScope = MainScope() @@ -90,6 +92,8 @@ abstract class AbstractFetchDownloadService : Service() { internal var downloadJobs = mutableMapOf() + // TODO Move this to browser store and make immutable: + // https://github.com/mozilla-mobile/android-components/issues/7050 internal data class DownloadJobState( var job: Job? = null, @Volatile var state: DownloadState, @@ -230,7 +234,9 @@ abstract class AbstractFetchDownloadService : Service() { } override fun onStartCommand(intent: Intent?, flags: Int, startId: Int): Int { - val download = intent?.getDownloadExtra() ?: return START_REDELIVER_INTENT + val download = intent?.getLongExtra(EXTRA_DOWNLOAD_ID, -1)?.let { + store.state.queuedDownloads[it] + } ?: return START_REDELIVER_INTENT // 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() @@ -376,6 +382,7 @@ abstract class AbstractFetchDownloadService : Service() { @VisibleForTesting internal fun removeDownloadJob(downloadJobState: DownloadJobState) { downloadJobs.remove(downloadJobState.state.id) + store.dispatch(DownloadAction.RemoveQueuedDownloadAction(downloadJobState.state.id)) if (downloadJobs.isEmpty()) { stopSelf() } else { @@ -570,7 +577,6 @@ abstract class AbstractFetchDownloadService : Service() { val intent = Intent(ACTION_DOWNLOAD_COMPLETE) intent.putExtra(EXTRA_DOWNLOAD_STATUS, getDownloadJobStatus(downloadState)) - intent.putExtra(EXTRA_DOWNLOAD, downloadState.state) intent.putExtra(EXTRA_DOWNLOAD_ID, downloadState.state.id) broadcastManager.sendBroadcast(intent) @@ -725,7 +731,6 @@ abstract class AbstractFetchDownloadService : Service() { */ internal const val PROGRESS_UPDATE_INTERVAL = 750L - const val EXTRA_DOWNLOAD = "mozilla.components.feature.downloads.extras.DOWNLOAD" const val EXTRA_DOWNLOAD_STATUS = "mozilla.components.feature.downloads.extras.DOWNLOAD_STATUS" const val ACTION_OPEN = "mozilla.components.feature.downloads.OPEN" const val ACTION_PAUSE = "mozilla.components.feature.downloads.PAUSE" diff --git a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/DownloadMiddleware.kt b/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/DownloadMiddleware.kt new file mode 100644 index 00000000000..2124ac6cb84 --- /dev/null +++ b/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/DownloadMiddleware.kt @@ -0,0 +1,40 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package mozilla.components.feature.downloads + +import android.app.DownloadManager +import android.content.Context +import android.content.Intent +import mozilla.components.browser.state.action.BrowserAction +import mozilla.components.browser.state.action.DownloadAction +import mozilla.components.browser.state.state.BrowserState +import mozilla.components.lib.state.Middleware +import mozilla.components.lib.state.MiddlewareStore + +/** + * [Middleware] implementation for managing downloads via the provided download service. It's + * purpose is to react to global download state changes (e.g. of [BrowserState.queuedDownloads]) + * and notify the download service, as needed. + */ +class DownloadMiddleware( + private val applicationContext: Context, + private val downloadServiceClass: Class<*> +) : Middleware { + + override fun invoke( + store: MiddlewareStore, + next: (BrowserAction) -> Unit, + action: BrowserAction + ) { + next(action) + when (action) { + is DownloadAction.QueueDownloadAction -> { + val intent = Intent(applicationContext, downloadServiceClass) + intent.putExtra(DownloadManager.EXTRA_DOWNLOAD_ID, action.download.id) + applicationContext.startService(intent) + } + } + } +} diff --git a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/DownloadsFeature.kt b/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/DownloadsFeature.kt index 8007504ed82..e934bf534b7 100644 --- a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/DownloadsFeature.kt +++ b/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/DownloadsFeature.kt @@ -52,14 +52,14 @@ import mozilla.components.support.ktx.kotlinx.coroutines.flow.ifChanged * @property dialog a reference to a [DownloadDialogFragment]. If not provided, an * instance of [SimpleDownloadDialogFragment] will be used. */ -@Suppress("TooManyFunctions") +@Suppress("TooManyFunctions", "LongParameterList") class DownloadsFeature( private val applicationContext: Context, private val store: BrowserStore, private val useCases: DownloadsUseCases, override var onNeedToRequestPermissions: OnNeedToRequestPermissions = { }, onDownloadStopped: onDownloadStopped = noop, - private val downloadManager: DownloadManager = AndroidDownloadManager(applicationContext), + private val downloadManager: DownloadManager = AndroidDownloadManager(applicationContext, store), private val tabId: String? = null, private val fragmentManager: FragmentManager? = null, private val promptsStyling: PromptsStyling? = null, diff --git a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/ext/Intent.kt b/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/ext/Intent.kt deleted file mode 100644 index 8133ed339e0..00000000000 --- a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/ext/Intent.kt +++ /dev/null @@ -1,54 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ - -package mozilla.components.feature.downloads.ext - -import android.app.DownloadManager.EXTRA_DOWNLOAD_ID -import android.content.Intent -import androidx.core.os.bundleOf -import mozilla.components.browser.state.state.content.DownloadState -import mozilla.components.concept.fetch.Headers.Names.CONTENT_LENGTH -import mozilla.components.concept.fetch.Headers.Names.CONTENT_TYPE -import mozilla.components.concept.fetch.Headers.Names.REFERRER -import mozilla.components.concept.fetch.Headers.Names.USER_AGENT - -private const val INTENT_DOWNLOAD = "mozilla.components.feature.downloads.DOWNLOAD" -private const val INTENT_URL = "mozilla.components.feature.downloads.URL" -private const val INTENT_FILE_NAME = "mozilla.components.feature.downloads.FILE_NAME" -private const val INTENT_DESTINATION = "mozilla.components.feature.downloads.DESTINATION" - -fun Intent.putDownloadExtra(download: DownloadState) { - download.apply { - putExtra(INTENT_DOWNLOAD, bundleOf( - INTENT_URL to url, - INTENT_FILE_NAME to fileName, - CONTENT_TYPE to contentType, - CONTENT_LENGTH to contentLength, - USER_AGENT to userAgent, - INTENT_DESTINATION to destinationDirectory, - REFERRER to referrerUrl, - EXTRA_DOWNLOAD_ID to id - )) - } -} - -fun Intent.getDownloadExtra(): DownloadState? = - getBundleExtra(INTENT_DOWNLOAD)?.run { - val url = getString(INTENT_URL) - val fileName = getString(INTENT_FILE_NAME) - val destination = getString(INTENT_DESTINATION) - val id = getLong(EXTRA_DOWNLOAD_ID) - if (url == null || destination == null) return null - - DownloadState( - url = url, - fileName = fileName, - contentType = getString(CONTENT_TYPE), - contentLength = get(CONTENT_LENGTH) as? Long?, - userAgent = getString(USER_AGENT), - destinationDirectory = destination, - referrerUrl = getString(REFERRER), - id = id - ) - } diff --git a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/manager/AndroidDownloadManager.kt b/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/manager/AndroidDownloadManager.kt index dea4b3fcf99..cc3034a88dc 100644 --- a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/manager/AndroidDownloadManager.kt +++ b/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/manager/AndroidDownloadManager.kt @@ -18,7 +18,9 @@ import androidx.annotation.RequiresPermission import androidx.core.content.getSystemService import androidx.core.net.toUri import androidx.core.util.set +import mozilla.components.browser.state.action.DownloadAction import mozilla.components.browser.state.state.content.DownloadState +import mozilla.components.browser.state.store.BrowserStore import mozilla.components.concept.fetch.Headers.Names.COOKIE import mozilla.components.concept.fetch.Headers.Names.REFERRER import mozilla.components.concept.fetch.Headers.Names.USER_AGENT @@ -36,20 +38,13 @@ typealias SystemRequest = android.app.DownloadManager.Request */ class AndroidDownloadManager( private val applicationContext: Context, + private val store: BrowserStore, override var onDownloadStopped: onDownloadStopped = noop ) : BroadcastReceiver(), DownloadManager { - private val queuedDownloads = LongSparseArray() + private val downloadRequests = LongSparseArray() private var isSubscribedReceiver = false - /** - * Holds both the state and the Android DownloadManager.Request for the queued download - */ - data class DownloadStateWithRequest( - val state: DownloadState, - val request: SystemRequest - ) - override val permissions = arrayOf(INTERNET, WRITE_EXTERNAL_STORAGE) /** @@ -60,7 +55,6 @@ class AndroidDownloadManager( */ @RequiresPermission(allOf = [INTERNET, WRITE_EXTERNAL_STORAGE]) override fun download(download: DownloadState, cookie: String): Long? { - val androidDownloadManager: SystemDownloadManager = applicationContext.getSystemService()!! if (!download.isScheme(listOf("http", "https"))) { @@ -73,22 +67,16 @@ class AndroidDownloadManager( validatePermissionGranted(applicationContext) val request = download.toAndroidRequest(cookie) - val downloadID = androidDownloadManager.enqueue(request) - - queuedDownloads[downloadID] = DownloadStateWithRequest( - state = download, - request = request - ) - + store.dispatch(DownloadAction.QueueDownloadAction(download.copy(id = downloadID))) + downloadRequests[downloadID] = request registerBroadcastReceiver() - return downloadID } override fun tryAgain(downloadId: Long) { val androidDownloadManager: SystemDownloadManager = applicationContext.getSystemService()!! - androidDownloadManager.enqueue(queuedDownloads[downloadId].request) + androidDownloadManager.enqueue(downloadRequests[downloadId]) } /** @@ -98,7 +86,8 @@ class AndroidDownloadManager( if (isSubscribedReceiver) { applicationContext.unregisterReceiver(this) isSubscribedReceiver = false - queuedDownloads.clear() + store.dispatch(DownloadAction.RemoveAllQueuedDownloadsAction) + downloadRequests.clear() } } @@ -111,17 +100,21 @@ class AndroidDownloadManager( } /** - * Invoked when a download is complete. Calls [onDownloadStopped] and unregisters the - * broadcast receiver if there are no more queued downloads. + * Invoked when a download is complete. Notifies [onDownloadStopped] and removes the queued + * download if it's complete. */ override fun onReceive(context: Context, intent: Intent) { val downloadID = intent.getLongExtra(EXTRA_DOWNLOAD_ID, -1) - val download = queuedDownloads[downloadID] + val download = store.state.queuedDownloads[downloadID] val downloadStatus = intent.getSerializableExtra(AbstractFetchDownloadService.EXTRA_DOWNLOAD_STATUS) as AbstractFetchDownloadService.DownloadJobStatus + if (downloadStatus == AbstractFetchDownloadService.DownloadJobStatus.COMPLETED) { + store.dispatch(DownloadAction.RemoveQueuedDownloadAction(downloadID)) + } + if (download != null) { - onDownloadStopped(download.state, downloadID, downloadStatus) + onDownloadStopped(download, downloadID, downloadStatus) } } } diff --git a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/manager/FetchDownloadManager.kt b/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/manager/FetchDownloadManager.kt index 11d464d21e0..016c96d6c99 100644 --- a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/manager/FetchDownloadManager.kt +++ b/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/manager/FetchDownloadManager.kt @@ -15,15 +15,13 @@ import android.content.Intent import android.content.IntentFilter import android.os.Build.VERSION.SDK_INT import android.os.Build.VERSION_CODES.P -import android.util.LongSparseArray -import androidx.core.util.set import androidx.localbroadcastmanager.content.LocalBroadcastManager +import mozilla.components.browser.state.action.DownloadAction import mozilla.components.browser.state.state.content.DownloadState +import mozilla.components.browser.state.store.BrowserStore import mozilla.components.feature.downloads.AbstractFetchDownloadService -import mozilla.components.feature.downloads.AbstractFetchDownloadService.Companion.EXTRA_DOWNLOAD import mozilla.components.feature.downloads.AbstractFetchDownloadService.Companion.EXTRA_DOWNLOAD_STATUS import mozilla.components.feature.downloads.ext.isScheme -import mozilla.components.feature.downloads.ext.putDownloadExtra import kotlin.reflect.KClass /** @@ -34,12 +32,12 @@ import kotlin.reflect.KClass */ class FetchDownloadManager( private val applicationContext: Context, + private val store: BrowserStore, private val service: KClass, private val broadcastManager: LocalBroadcastManager = LocalBroadcastManager.getInstance(applicationContext), override var onDownloadStopped: onDownloadStopped = noop ) : BroadcastReceiver(), DownloadManager { - private val queuedDownloads = LongSparseArray() private var isSubscribedReceiver = false override val permissions = if (SDK_INT >= P) { @@ -58,25 +56,21 @@ class FetchDownloadManager( if (!download.isScheme(listOf("http", "https", "data", "blob"))) { return null } - validatePermissionGranted(applicationContext) - queuedDownloads[download.id] = download - - val intent = Intent(applicationContext, service.java) - intent.putDownloadExtra(download) - applicationContext.startService(intent) + // The middleware will notify the service to start the download + // once this action is processed. + store.dispatch(DownloadAction.QueueDownloadAction(download)) registerBroadcastReceiver() - return download.id } override fun tryAgain(downloadId: Long) { - val download = queuedDownloads[downloadId] ?: return + val download = store.state.queuedDownloads[downloadId] ?: return val intent = Intent(applicationContext, service.java) - intent.putDownloadExtra(download) + intent.putExtra(EXTRA_DOWNLOAD_ID, download.id) applicationContext.startService(intent) registerBroadcastReceiver() @@ -89,7 +83,7 @@ class FetchDownloadManager( if (isSubscribedReceiver) { broadcastManager.unregisterReceiver(this) isSubscribedReceiver = false - queuedDownloads.clear() + store.dispatch(DownloadAction.RemoveAllQueuedDownloadsAction) } } @@ -102,15 +96,18 @@ class FetchDownloadManager( } /** - * Invoked when a download is complete. Calls [onDownloadStopped] and unregisters the - * broadcast receiver if there are no more queued downloads. + * Invoked when a download is complete. Notifies [onDownloadStopped] and removes the queued + * download if it's complete. */ override fun onReceive(context: Context, intent: Intent) { - val download = intent.getParcelableExtra(EXTRA_DOWNLOAD) val downloadID = intent.getLongExtra(EXTRA_DOWNLOAD_ID, -1) + val download = store.state.queuedDownloads[downloadID] val downloadStatus = intent.getSerializableExtra(EXTRA_DOWNLOAD_STATUS) as AbstractFetchDownloadService.DownloadJobStatus + if (downloadStatus == AbstractFetchDownloadService.DownloadJobStatus.COMPLETED) { + store.dispatch(DownloadAction.RemoveQueuedDownloadAction(downloadID)) + } if (download != null) { onDownloadStopped(download, downloadID, downloadStatus) } diff --git a/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/AbstractFetchDownloadServiceTest.kt b/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/AbstractFetchDownloadServiceTest.kt index 424a0c2ab12..a7650d2e6c0 100644 --- a/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/AbstractFetchDownloadServiceTest.kt +++ b/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/AbstractFetchDownloadServiceTest.kt @@ -4,6 +4,7 @@ package mozilla.components.feature.downloads +import android.app.DownloadManager.EXTRA_DOWNLOAD_ID import android.app.Notification import android.app.NotificationManager import android.app.Service @@ -22,7 +23,9 @@ import kotlinx.coroutines.launch import kotlinx.coroutines.test.TestCoroutineDispatcher import kotlinx.coroutines.test.resetMain import kotlinx.coroutines.test.setMain +import mozilla.components.browser.state.action.DownloadAction import mozilla.components.browser.state.state.content.DownloadState +import mozilla.components.browser.state.store.BrowserStore import mozilla.components.concept.fetch.Client import mozilla.components.concept.fetch.MutableHeaders import mozilla.components.concept.fetch.Request @@ -39,12 +42,12 @@ import mozilla.components.feature.downloads.AbstractFetchDownloadService.Downloa import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobStatus.FAILED import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobStatus.PAUSED import mozilla.components.feature.downloads.DownloadNotification.NOTIFICATION_DOWNLOAD_GROUP_ID -import mozilla.components.feature.downloads.ext.putDownloadExtra import mozilla.components.feature.downloads.facts.DownloadsFacts.Items.NOTIFICATION import mozilla.components.support.base.facts.Action import mozilla.components.support.base.facts.processor.CollectionProcessor import mozilla.components.support.test.any import mozilla.components.support.test.argumentCaptor +import mozilla.components.support.test.ext.joinBlocking import mozilla.components.support.test.mock import mozilla.components.support.test.robolectric.testContext import org.junit.After @@ -83,6 +86,7 @@ class AbstractFetchDownloadServiceTest { val folder = TemporaryFolder() @Mock private lateinit var client: Client + private lateinit var browserStore: BrowserStore @Mock private lateinit var broadcastManager: LocalBroadcastManager private lateinit var service: AbstractFetchDownloadService @@ -93,8 +97,10 @@ class AbstractFetchDownloadServiceTest { fun setup() { Dispatchers.setMain(testDispatcher) initMocks(this) + browserStore = BrowserStore() service = spy(object : AbstractFetchDownloadService() { override val httpClient = client + override val store = browserStore }) doReturn(broadcastManager).`when`(service).broadcastManager @@ -121,10 +127,10 @@ class AbstractFetchDownloadServiceTest { ) doReturn(response).`when`(client).fetch(Request("https://example.com/file.txt")) - val downloadIntent = Intent("ACTION_DOWNLOAD").apply { - putDownloadExtra(download) - } + val downloadIntent = Intent("ACTION_DOWNLOAD") + downloadIntent.putExtra(EXTRA_DOWNLOAD_ID, download.id) + browserStore.dispatch(DownloadAction.QueueDownloadAction(download)).joinBlocking() service.onStartCommand(downloadIntent, 0, 0) service.downloadJobs.values.forEach { it.job?.join() } @@ -264,10 +270,10 @@ class AbstractFetchDownloadServiceTest { ) doReturn(response).`when`(client).fetch(Request("https://example.com/file.txt")) - val downloadIntent = Intent("ACTION_DOWNLOAD").apply { - putDownloadExtra(download) - } + val downloadIntent = Intent("ACTION_DOWNLOAD") + downloadIntent.putExtra(EXTRA_DOWNLOAD_ID, download.id) + browserStore.dispatch(DownloadAction.QueueDownloadAction(download)).joinBlocking() service.onStartCommand(downloadIntent, 0, 0) service.downloadJobs.values.forEach { it.job?.join() } @@ -303,10 +309,10 @@ class AbstractFetchDownloadServiceTest { ) doReturn(response).`when`(client).fetch(Request("https://example.com/file.txt")) - val downloadIntent = Intent("ACTION_DOWNLOAD").apply { - putDownloadExtra(download) - } + val downloadIntent = Intent("ACTION_DOWNLOAD") + downloadIntent.putExtra(EXTRA_DOWNLOAD_ID, download.id) + browserStore.dispatch(DownloadAction.QueueDownloadAction(download)).joinBlocking() service.onStartCommand(downloadIntent, 0, 0) service.downloadJobs.values.forEach { it.job?.join() } @@ -350,10 +356,10 @@ class AbstractFetchDownloadServiceTest { doReturn(resumeResponse).`when`(client) .fetch(Request("https://example.com/file.txt", headers = MutableHeaders("Range" to "bytes=1-"))) - val downloadIntent = Intent("ACTION_DOWNLOAD").apply { - putDownloadExtra(download) - } + val downloadIntent = Intent("ACTION_DOWNLOAD") + downloadIntent.putExtra(EXTRA_DOWNLOAD_ID, download.id) + browserStore.dispatch(DownloadAction.QueueDownloadAction(download)).joinBlocking() service.onStartCommand(downloadIntent, 0, 0) service.downloadJobs.values.forEach { it.job?.join() } @@ -401,10 +407,10 @@ class AbstractFetchDownloadServiceTest { ) doReturn(response).`when`(client).fetch(Request("https://example.com/file.txt")) - val downloadIntent = Intent("ACTION_DOWNLOAD").apply { - putDownloadExtra(download) - } + val downloadIntent = Intent("ACTION_DOWNLOAD") + downloadIntent.putExtra(EXTRA_DOWNLOAD_ID, download.id) + browserStore.dispatch(DownloadAction.QueueDownloadAction(download)).joinBlocking() service.onStartCommand(downloadIntent, 0, 0) service.downloadJobs.values.forEach { it.job?.join() } @@ -452,10 +458,10 @@ class AbstractFetchDownloadServiceTest { ) doReturn(response).`when`(client).fetch(Request("https://example.com/file.txt")) - val downloadIntent = Intent("ACTION_DOWNLOAD").apply { - putDownloadExtra(download) - } + val downloadIntent = Intent("ACTION_DOWNLOAD") + downloadIntent.putExtra(EXTRA_DOWNLOAD_ID, download.id) + browserStore.dispatch(DownloadAction.QueueDownloadAction(download)).joinBlocking() service.onStartCommand(downloadIntent, 0, 0) service.downloadJobs.values.forEach { it.job?.join() } @@ -506,10 +512,10 @@ class AbstractFetchDownloadServiceTest { ) doReturn(response).`when`(client).fetch(Request("https://example.com/file.txt")) - val downloadIntent = Intent("ACTION_DOWNLOAD").apply { - putDownloadExtra(download) - } + val downloadIntent = Intent("ACTION_DOWNLOAD") + downloadIntent.putExtra(EXTRA_DOWNLOAD_ID, download.id) + browserStore.dispatch(DownloadAction.QueueDownloadAction(download)).joinBlocking() service.onStartCommand(downloadIntent, 0, 0) service.downloadJobs.values.forEach { it.job?.join() } @@ -531,12 +537,12 @@ class AbstractFetchDownloadServiceTest { fun `onStartCommand sets the notification foreground`() = runBlocking { val download = DownloadState("https://example.com/file.txt", "file.txt") - val downloadIntent = Intent("ACTION_DOWNLOAD").apply { - putDownloadExtra(download) - } + val downloadIntent = Intent("ACTION_DOWNLOAD") + downloadIntent.putExtra(EXTRA_DOWNLOAD_ID, download.id) doNothing().`when`(service).performDownload(any()) + browserStore.dispatch(DownloadAction.QueueDownloadAction(download)).joinBlocking() service.onStartCommand(downloadIntent, 0, 0) verify(service).setForegroundNotification(any()) @@ -608,9 +614,8 @@ class AbstractFetchDownloadServiceTest { fun `getForegroundId in devices that support notification group will return NOTIFICATION_DOWNLOAD_GROUP_ID`() { val download = DownloadState(id = 1, url = "https://example.com/file.txt", fileName = "file.txt") - val downloadIntent = Intent("ACTION_DOWNLOAD").apply { - putDownloadExtra(download) - } + val downloadIntent = Intent("ACTION_DOWNLOAD") + downloadIntent.putExtra(EXTRA_DOWNLOAD_ID, download.id) doNothing().`when`(service).performDownload(any()) @@ -624,12 +629,12 @@ class AbstractFetchDownloadServiceTest { fun `getForegroundId in devices that support DO NOT notification group will return the latest active download`() { val download = DownloadState(id = 1, url = "https://example.com/file.txt", fileName = "file.txt") - val downloadIntent = Intent("ACTION_DOWNLOAD").apply { - putDownloadExtra(download) - } + val downloadIntent = Intent("ACTION_DOWNLOAD") + downloadIntent.putExtra(EXTRA_DOWNLOAD_ID, download.id) doNothing().`when`(service).performDownload(any()) + browserStore.dispatch(DownloadAction.QueueDownloadAction(download)).joinBlocking() service.onStartCommand(downloadIntent, 0, 0) val foregroundId = service.downloadJobs.values.first().foregroundServiceId @@ -769,10 +774,10 @@ class AbstractFetchDownloadServiceTest { ) doReturn(response).`when`(client).fetch(Request("https://example.com/file.txt")) - val downloadIntent = Intent("ACTION_DOWNLOAD").apply { - putDownloadExtra(download) - } + val downloadIntent = Intent("ACTION_DOWNLOAD") + downloadIntent.putExtra(EXTRA_DOWNLOAD_ID, download.id) + browserStore.dispatch(DownloadAction.QueueDownloadAction(download)).joinBlocking() service.onStartCommand(downloadIntent, 0, 0) service.downloadJobs.values.forEach { it.job?.join() } @@ -801,10 +806,10 @@ class AbstractFetchDownloadServiceTest { ) doReturn(response).`when`(client).fetch(Request("https://example.com/file.txt")) - val downloadIntent = Intent("ACTION_DOWNLOAD").apply { - putDownloadExtra(download) - } + val downloadIntent = Intent("ACTION_DOWNLOAD") + downloadIntent.putExtra(EXTRA_DOWNLOAD_ID, download.id) + browserStore.dispatch(DownloadAction.QueueDownloadAction(download)).joinBlocking() service.onStartCommand(downloadIntent, 0, 0) service.downloadJobs.values.forEach { it.job?.join() } @@ -832,10 +837,10 @@ class AbstractFetchDownloadServiceTest { ) doReturn(response).`when`(client).fetch(Request("https://example.com/file.txt")) - val downloadIntent = Intent("ACTION_DOWNLOAD").apply { - putDownloadExtra(download) - } + val downloadIntent = Intent("ACTION_DOWNLOAD") + downloadIntent.putExtra(EXTRA_DOWNLOAD_ID, download.id) + browserStore.dispatch(DownloadAction.QueueDownloadAction(download)).joinBlocking() service.onStartCommand(downloadIntent, 0, 0) service.downloadJobs.values.forEach { it.job?.join() } @@ -864,10 +869,10 @@ class AbstractFetchDownloadServiceTest { ) doReturn(response).`when`(client).fetch(Request("https://example.com/file.txt")) - val downloadIntent = Intent("ACTION_DOWNLOAD").apply { - putDownloadExtra(download) - } + val downloadIntent = Intent("ACTION_DOWNLOAD") + downloadIntent.putExtra(EXTRA_DOWNLOAD_ID, download.id) + browserStore.dispatch(DownloadAction.QueueDownloadAction(download)).joinBlocking() service.onStartCommand(downloadIntent, 0, 0) service.downloadJobs.values.forEach { it.job?.join() } @@ -890,10 +895,10 @@ class AbstractFetchDownloadServiceTest { doThrow(IOException()).`when`(client).fetch(any()) val download = DownloadState("https://example.com/file.txt", "file.txt") - val downloadIntent = Intent("ACTION_DOWNLOAD").apply { - putDownloadExtra(download) - } + val downloadIntent = Intent("ACTION_DOWNLOAD") + downloadIntent.putExtra(EXTRA_DOWNLOAD_ID, download.id) + browserStore.dispatch(DownloadAction.QueueDownloadAction(download)).joinBlocking() service.onStartCommand(downloadIntent, 0, 0) service.downloadJobs.values.forEach { it.job?.join() } @@ -923,10 +928,10 @@ class AbstractFetchDownloadServiceTest { doCallRealMethod().`when`(service).useFileStream(any(), anyBoolean(), any()) doReturn(response).`when`(client).fetch(Request("https://example.com/file.txt")) - val downloadIntent = Intent("ACTION_DOWNLOAD").apply { - putDownloadExtra(download) - } + val downloadIntent = Intent("ACTION_DOWNLOAD") + downloadIntent.putExtra(EXTRA_DOWNLOAD_ID, download.id) + browserStore.dispatch(DownloadAction.QueueDownloadAction(download)).joinBlocking() service.registerNotificationActionsReceiver() service.onStartCommand(downloadIntent, 0, 0) service.downloadJobs.values.forEach { assertTrue(it.job!!.isActive) } @@ -963,10 +968,10 @@ class AbstractFetchDownloadServiceTest { ) doReturn(response).`when`(client).fetch(Request("https://example.com/file.txt")) - val downloadIntent = Intent("ACTION_DOWNLOAD").apply { - putDownloadExtra(download) - } + val downloadIntent = Intent("ACTION_DOWNLOAD") + downloadIntent.putExtra(EXTRA_DOWNLOAD_ID, download.id) + browserStore.dispatch(DownloadAction.QueueDownloadAction(download)).joinBlocking() service.registerNotificationActionsReceiver() service.onStartCommand(downloadIntent, 0, 0) service.downloadJobs.values.forEach { it.job?.join() } @@ -1022,6 +1027,7 @@ class AbstractFetchDownloadServiceTest { fun `onDestroy will remove all download notifications, jobs and will call unregisterNotificationActionsReceiver`() = runBlocking { val service = spy(object : AbstractFetchDownloadService() { override val httpClient = client + override val store = browserStore }) doReturn(testContext).`when`(service).context @@ -1038,6 +1044,7 @@ class AbstractFetchDownloadServiceTest { fun `register and unregister notification actions receiver`() { val service = spy(object : AbstractFetchDownloadService() { override val httpClient = client + override val store = browserStore }) doReturn(testContext).`when`(service).context @@ -1062,10 +1069,10 @@ class AbstractFetchDownloadServiceTest { ) doReturn(response).`when`(client).fetch(Request("https://example.com/file.txt")) - val cancelledDownloadIntent = Intent("ACTION_DOWNLOAD").apply { - putDownloadExtra(cancelledDownload) - } + val cancelledDownloadIntent = Intent("ACTION_DOWNLOAD") + cancelledDownloadIntent.putExtra(EXTRA_DOWNLOAD_ID, cancelledDownload.id) + browserStore.dispatch(DownloadAction.QueueDownloadAction(cancelledDownload)).joinBlocking() service.onStartCommand(cancelledDownloadIntent, 0, 0) service.downloadJobs.values.forEach { it.job?.join() } @@ -1083,11 +1090,11 @@ class AbstractFetchDownloadServiceTest { assertEquals(1, shadowNotificationService.size()) val download = DownloadState("https://example.com/file.txt", "file.txt") - val downloadIntent = Intent("ACTION_DOWNLOAD").apply { - putDownloadExtra(download) - } + val downloadIntent = Intent("ACTION_DOWNLOAD") + downloadIntent.putExtra(EXTRA_DOWNLOAD_ID, download.id) // Start another download to ensure its notifications are presented + browserStore.dispatch(DownloadAction.QueueDownloadAction(download)).joinBlocking() service.onStartCommand(downloadIntent, 0, 0) service.downloadJobs.values.forEach { it.job?.join() } verify(service, times(2)).performDownload(providedDownload.capture()) diff --git a/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/DownloadMiddlewareTest.kt b/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/DownloadMiddlewareTest.kt new file mode 100644 index 00000000000..4588d7e0162 --- /dev/null +++ b/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/DownloadMiddlewareTest.kt @@ -0,0 +1,41 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package mozilla.components.feature.downloads + +import android.app.DownloadManager.EXTRA_DOWNLOAD_ID +import android.content.Context +import android.content.Intent +import androidx.test.ext.junit.runners.AndroidJUnit4 +import mozilla.components.browser.state.action.DownloadAction +import mozilla.components.browser.state.state.BrowserState +import mozilla.components.browser.state.state.content.DownloadState +import mozilla.components.browser.state.store.BrowserStore +import mozilla.components.support.test.argumentCaptor +import mozilla.components.support.test.ext.joinBlocking +import mozilla.components.support.test.mock +import org.junit.Assert.assertEquals +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.Mockito.verify + +@RunWith(AndroidJUnit4::class) +class DownloadMiddlewareTest { + + @Test + fun `service is started when download is queued`() { + val applicationContext: Context = mock() + val store = BrowserStore( + initialState = BrowserState(), + middleware = listOf(DownloadMiddleware(applicationContext, AbstractFetchDownloadService::class.java)) + ) + + val download = DownloadState("https://mozilla.org/download", destinationDirectory = "") + store.dispatch(DownloadAction.QueueDownloadAction(download)).joinBlocking() + + val intentCaptor = argumentCaptor() + verify(applicationContext).startService(intentCaptor.capture()) + assertEquals(download.id, intentCaptor.value.getLongExtra(EXTRA_DOWNLOAD_ID, -1)) + } +} diff --git a/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/manager/AndroidDownloadManagerTest.kt b/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/manager/AndroidDownloadManagerTest.kt index a2329824e57..8e4f3e54273 100644 --- a/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/manager/AndroidDownloadManagerTest.kt +++ b/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/manager/AndroidDownloadManagerTest.kt @@ -11,6 +11,7 @@ import android.app.DownloadManager.Request import android.content.Intent import androidx.test.ext.junit.runners.AndroidJUnit4 import mozilla.components.browser.state.state.content.DownloadState +import mozilla.components.browser.state.store.BrowserStore import mozilla.components.support.test.mock import mozilla.components.support.test.robolectric.grantPermission import mozilla.components.support.test.robolectric.testContext @@ -24,11 +25,13 @@ import org.mockito.Mockito.verify import org.mockito.Mockito.verifyZeroInteractions import mozilla.components.feature.downloads.AbstractFetchDownloadService.Companion.EXTRA_DOWNLOAD_STATUS import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobStatus +import mozilla.components.support.test.libstate.ext.waitUntilIdle import org.junit.Assert.assertEquals @RunWith(AndroidJUnit4::class) class AndroidDownloadManagerTest { + private lateinit var store: BrowserStore private lateinit var download: DownloadState private lateinit var downloadManager: AndroidDownloadManager @@ -39,7 +42,8 @@ class AndroidDownloadManagerTest { "", "application/zip", 5242880, "Mozilla/5.0 (Linux; Android 7.1.1) AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Focus/8.0 Chrome/69.0.3497.100 Mobile Safari/537.36" ) - downloadManager = AndroidDownloadManager(testContext) + store = BrowserStore() + downloadManager = AndroidDownloadManager(testContext, store) } @Test(expected = SecurityException::class) @@ -55,44 +59,39 @@ class AndroidDownloadManagerTest { grantPermissions() + assertTrue(store.state.queuedDownloads.isEmpty()) val id = downloadManager.download(download)!! + store.waitUntilIdle() + assertEquals(download.copy(id = id), store.state.queuedDownloads[id]) notifyDownloadCompleted(id) - assertTrue(downloadCompleted) } @Test fun `calling tryAgain starts the download again`() { - var downloadCompleted = false - - downloadManager.onDownloadStopped = { _, _, _ -> downloadCompleted = true } + var downloadStopped = false + downloadManager.onDownloadStopped = { _, _, _ -> downloadStopped = true } grantPermissions() val id = downloadManager.download(download)!! + store.waitUntilIdle() + notifyDownloadFailed(id) + assertTrue(downloadStopped) - notifyDownloadCompleted(id) - - assertTrue(downloadCompleted) - - downloadCompleted = false - + downloadStopped = false downloadManager.tryAgain(id) - notifyDownloadCompleted(id) - assertTrue(downloadCompleted) + assertTrue(downloadStopped) } @Test fun `trying to download a file with invalid protocol must NOT triggered a download`() { - val invalidDownload = download.copy(url = "ftp://ipv4.download.thinkbroadband.com/5MB.zip") - grantPermissions() val id = downloadManager.download(invalidDownload) - assertNull(id) } @@ -108,6 +107,7 @@ class AndroidDownloadManagerTest { downloadWithFileName, cookie = "yummy_cookie=choco" )!! + store.waitUntilIdle() downloadManager.onDownloadStopped = { _, _, status -> downloadStatus = status @@ -120,6 +120,29 @@ class AndroidDownloadManagerTest { assertEquals(DownloadJobStatus.COMPLETED, downloadStatus) } + @Test + fun `sendBroadcast with completed download removes queued download from store`() { + var downloadStatus: DownloadJobStatus? = null + val downloadWithFileName = download.copy(fileName = "5MB.zip") + grantPermissions() + + downloadManager.onDownloadStopped = { _, _, status -> + downloadStatus = status + } + + val id = downloadManager.download( + downloadWithFileName, + cookie = "yummy_cookie=choco" + )!! + store.waitUntilIdle() + assertEquals(downloadWithFileName.copy(id = id), store.state.queuedDownloads[id]) + + notifyDownloadCompleted(id) + store.waitUntilIdle() + assertEquals(DownloadJobStatus.COMPLETED, downloadStatus) + assertTrue(store.state.queuedDownloads.isEmpty()) + } + @Test fun `no null or empty headers can be added to the DownloadManager`() { val mockRequest: Request = mock() @@ -139,6 +162,13 @@ class AndroidDownloadManagerTest { verify(mockRequest).addRequestHeader(anyString(), anyString()) } + private fun notifyDownloadFailed(id: Long) { + val intent = Intent(ACTION_DOWNLOAD_COMPLETE) + intent.putExtra(android.app.DownloadManager.EXTRA_DOWNLOAD_ID, id) + intent.putExtra(EXTRA_DOWNLOAD_STATUS, DownloadJobStatus.FAILED) + testContext.sendBroadcast(intent) + } + private fun notifyDownloadCompleted(id: Long) { val intent = Intent(ACTION_DOWNLOAD_COMPLETE) intent.putExtra(android.app.DownloadManager.EXTRA_DOWNLOAD_ID, id) diff --git a/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/manager/FetchDownloadManagerTest.kt b/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/manager/FetchDownloadManagerTest.kt index 0423877748c..94fdc9ba266 100644 --- a/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/manager/FetchDownloadManagerTest.kt +++ b/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/manager/FetchDownloadManagerTest.kt @@ -14,9 +14,9 @@ import android.content.Intent import androidx.localbroadcastmanager.content.LocalBroadcastManager import androidx.test.ext.junit.runners.AndroidJUnit4 import mozilla.components.browser.state.state.content.DownloadState +import mozilla.components.browser.state.store.BrowserStore import mozilla.components.concept.fetch.Client import mozilla.components.feature.downloads.AbstractFetchDownloadService -import mozilla.components.feature.downloads.AbstractFetchDownloadService.Companion.EXTRA_DOWNLOAD import mozilla.components.feature.downloads.AbstractFetchDownloadService.Companion.EXTRA_DOWNLOAD_STATUS import mozilla.components.support.test.any import mozilla.components.support.test.mock @@ -27,12 +27,12 @@ import org.junit.Test import org.junit.runner.RunWith import org.mockito.Mockito.verify import mozilla.components.feature.downloads.AbstractFetchDownloadService.DownloadJobStatus -import org.junit.Assert.assertFalse +import mozilla.components.support.test.libstate.ext.waitUntilIdle import org.junit.Assert.assertTrue import org.junit.Assert.assertNull import org.junit.Assert.assertNotNull import org.junit.Assert.assertEquals -import org.mockito.Mockito.times +import org.mockito.Mockito.never @RunWith(AndroidJUnit4::class) class FetchDownloadManagerTest { @@ -41,17 +41,19 @@ class FetchDownloadManagerTest { private lateinit var service: MockDownloadService private lateinit var download: DownloadState private lateinit var downloadManager: FetchDownloadManager + private lateinit var store: BrowserStore @Before fun setup() { broadcastManager = LocalBroadcastManager.getInstance(testContext) service = MockDownloadService() + store = BrowserStore() download = DownloadState( "http://ipv4.download.thinkbroadband.com/5MB.zip", "", "application/zip", 5242880, "Mozilla/5.0 (Linux; Android 7.1.1) AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Focus/8.0 Chrome/69.0.3497.100 Mobile Safari/537.36" ) - downloadManager = FetchDownloadManager(testContext, MockDownloadService::class, broadcastManager) + downloadManager = FetchDownloadManager(testContext, store, MockDownloadService::class, broadcastManager) } @Test(expected = SecurityException::class) @@ -60,80 +62,64 @@ class FetchDownloadManagerTest { } @Test - fun `calling download must download the file`() { + fun `calling download must queue the download`() { val context: Context = mock() - downloadManager = FetchDownloadManager(context, MockDownloadService::class, broadcastManager) - var downloadCompleted = false + downloadManager = FetchDownloadManager(context, store, MockDownloadService::class, broadcastManager) + var downloadStopped = false - downloadManager.onDownloadStopped = { _, _, _ -> downloadCompleted = true } + downloadManager.onDownloadStopped = { _, _, _ -> downloadStopped = true } grantPermissions() + assertTrue(store.state.queuedDownloads.isEmpty()) val id = downloadManager.download(download)!! - - verify(context).startService(any()) + store.waitUntilIdle() + assertEquals(download, store.state.queuedDownloads[download.id]) notifyDownloadCompleted(id) - - assertTrue(downloadCompleted) + assertTrue(downloadStopped) } @Test fun `calling tryAgain starts the download again`() { val context: Context = mock() - downloadManager = FetchDownloadManager(context, MockDownloadService::class, broadcastManager) - var downloadCompleted = false + downloadManager = FetchDownloadManager(context, store, MockDownloadService::class, broadcastManager) + var downloadStopped = false - downloadManager.onDownloadStopped = { _, _, _ -> downloadCompleted = true } + downloadManager.onDownloadStopped = { _, _, _ -> downloadStopped = true } grantPermissions() val id = downloadManager.download(download)!! + store.waitUntilIdle() + notifyDownloadFailed(id) + assertTrue(downloadStopped) - verify(context).startService(any()) - notifyDownloadCompleted(id) - assertTrue(downloadCompleted) - - downloadCompleted = false - + downloadStopped = false downloadManager.tryAgain(id) - - verify(context, times(2)).startService(any()) + verify(context).startService(any()) notifyDownloadCompleted(id) - assertTrue(downloadCompleted) + assertTrue(downloadStopped) } @Test fun `try again should not crash when download does not exist`() { val context: Context = mock() - downloadManager = FetchDownloadManager(context, MockDownloadService::class, broadcastManager) - var downloadCompleted = false - - downloadManager.onDownloadStopped = { _, _, _ -> downloadCompleted = true } + downloadManager = FetchDownloadManager(context, store, MockDownloadService::class, broadcastManager) grantPermissions() - val id = downloadManager.download(download)!! - verify(context).startService(any()) - notifyDownloadCompleted(id) - assertTrue(downloadCompleted) - - downloadCompleted = false downloadManager.tryAgain(id + 1) - assertFalse(downloadCompleted) - verify(context, times(1)).startService(any()) + verify(context, never()).startService(any()) } @Test fun `trying to download a file with invalid protocol must NOT triggered a download`() { - val invalidDownload = download.copy(url = "ftp://ipv4.download.thinkbroadband.com/5MB.zip") - grantPermissions() val id = downloadManager.download(invalidDownload) - assertNull(id) } @@ -148,7 +134,7 @@ class FetchDownloadManagerTest { @Test fun `sendBroadcast with valid downloadID must call onDownloadStopped after download`() { - var downloadCompleted = false + var downloadStopped = false var downloadStatus: DownloadJobStatus? = null val downloadWithFileName = download.copy(fileName = "5MB.zip") @@ -156,24 +142,46 @@ class FetchDownloadManagerTest { downloadManager.onDownloadStopped = { _, _, status -> downloadStatus = status - downloadCompleted = true + downloadStopped = true } val id = downloadManager.download( downloadWithFileName, cookie = "yummy_cookie=choco" )!! + store.waitUntilIdle() notifyDownloadCompleted(id) + assertTrue(downloadStopped) + assertEquals(DownloadJobStatus.COMPLETED, downloadStatus) + } + + @Test + fun `sendBroadcast with completed download removes queued download from store`() { + var downloadStatus: DownloadJobStatus? = null + val downloadWithFileName = download.copy(fileName = "5MB.zip") + grantPermissions() + + downloadManager.onDownloadStopped = { _, _, status -> + downloadStatus = status + } - assertTrue(downloadCompleted) + val id = downloadManager.download( + downloadWithFileName, + cookie = "yummy_cookie=choco" + )!! + store.waitUntilIdle() + assertEquals(downloadWithFileName, store.state.queuedDownloads[downloadWithFileName.id]) + notifyDownloadCompleted(id) + store.waitUntilIdle() assertEquals(DownloadJobStatus.COMPLETED, downloadStatus) + assertTrue(store.state.queuedDownloads.isEmpty()) } @Test fun `onReceive properly gets download object form sendBroadcast`() { - var downloadCompleted = false + var downloadStopped = false var downloadStatus: DownloadJobStatus? = null var downloadName = "" var downloadSize = 0L @@ -183,26 +191,32 @@ class FetchDownloadManagerTest { downloadManager.onDownloadStopped = { download, _, status -> downloadStatus = status - downloadCompleted = true + downloadStopped = true downloadName = download.fileName ?: "" downloadSize = download.contentLength ?: 0 } val id = downloadManager.download(downloadWithFileName)!! + store.waitUntilIdle() + notifyDownloadCompleted(id) - notifyDownloadCompleted(id, downloadWithFileName) - - assertTrue(downloadCompleted) + assertTrue(downloadStopped) assertEquals("5MB.zip", downloadName) assertEquals(5L, downloadSize) assertEquals(DownloadJobStatus.COMPLETED, downloadStatus) } - private fun notifyDownloadCompleted(id: Long, download: DownloadState = DownloadState(url = "")) { + private fun notifyDownloadFailed(id: Long) { + val intent = Intent(ACTION_DOWNLOAD_COMPLETE) + intent.putExtra(EXTRA_DOWNLOAD_ID, id) + intent.putExtra(EXTRA_DOWNLOAD_STATUS, DownloadJobStatus.FAILED) + broadcastManager.sendBroadcast(intent) + } + + private fun notifyDownloadCompleted(id: Long) { val intent = Intent(ACTION_DOWNLOAD_COMPLETE) intent.putExtra(EXTRA_DOWNLOAD_ID, id) intent.putExtra(EXTRA_DOWNLOAD_STATUS, DownloadJobStatus.COMPLETED) - intent.putExtra(EXTRA_DOWNLOAD, download) broadcastManager.sendBroadcast(intent) } @@ -213,5 +227,6 @@ class FetchDownloadManagerTest { class MockDownloadService : AbstractFetchDownloadService() { override val httpClient: Client = mock() + override val store: BrowserStore = mock() } } diff --git a/docs/changelog.md b/docs/changelog.md index 7aaf31b1ccd..2f4a7bcb705 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -13,6 +13,41 @@ permalink: /changelog/ * [Configuration](https://github.com/mozilla-mobile/android-components/blob/master/buildSrc/src/main/java/Config.kt) * **feature-downloads** + * ⚠️ **This is a breaking change**: DownloadManager and DownloadService are now using the browser store to keep track of queued downloads. Therefore, an instance of the store needs to be provided when constructing manager and service. There's also a new DownloadMiddleware which needs to be provided to the store. + ```kotlin + val store by lazy { + BrowserStore(middleware = listOf( + MediaMiddleware(applicationContext, MediaService::class.java), + DownloadMiddleware(applicationContext, DownloadService::class.java), + ... + )) + } + ) + + val feature = DownloadsFeature( + requireContext().applicationContext, + store = components.store, + useCases = components.downloadsUseCases, + fragmentManager = childFragmentManager, + onDownloadStopped = { download, id, status -> + Logger.debug("Download done. ID#$id $download with status $status") + }, + downloadManager = FetchDownloadManager( + requireContext().applicationContext, + components.store, // Store needs to be provided now + DownloadService::class + ), + tabId = sessionId, + onNeedToRequestPermissions = { permissions -> + requestPermissions(permissions, REQUEST_CODE_DOWNLOAD_PERMISSIONS) + } + ) + + class DownloadService : AbstractFetchDownloadService() { + override val httpClient by lazy { components.core.client } + override val store: BrowserStore by lazy { components.core.store } // Store needs to be provided now + } + ``` * Fixed issue [#6893](https://github.com/mozilla-mobile/android-components/issues/6893). * Add notification grouping to downloads Fenix issue [#4910](https://github.com/mozilla-mobile/android-components/issues/4910). diff --git a/samples/browser/src/main/java/org/mozilla/samples/browser/BaseBrowserFragment.kt b/samples/browser/src/main/java/org/mozilla/samples/browser/BaseBrowserFragment.kt index 99338721d92..50249324060 100644 --- a/samples/browser/src/main/java/org/mozilla/samples/browser/BaseBrowserFragment.kt +++ b/samples/browser/src/main/java/org/mozilla/samples/browser/BaseBrowserFragment.kt @@ -117,6 +117,7 @@ abstract class BaseBrowserFragment : Fragment(), UserInteractionHandler { }, downloadManager = FetchDownloadManager( requireContext().applicationContext, + components.store, DownloadService::class ), tabId = sessionId, diff --git a/samples/browser/src/main/java/org/mozilla/samples/browser/DefaultComponents.kt b/samples/browser/src/main/java/org/mozilla/samples/browser/DefaultComponents.kt index 7506cf61a7f..da2e5b1008d 100644 --- a/samples/browser/src/main/java/org/mozilla/samples/browser/DefaultComponents.kt +++ b/samples/browser/src/main/java/org/mozilla/samples/browser/DefaultComponents.kt @@ -44,6 +44,7 @@ import mozilla.components.feature.app.links.AppLinksUseCases import mozilla.components.feature.contextmenu.ContextMenuUseCases import mozilla.components.feature.customtabs.CustomTabIntentProcessor import mozilla.components.feature.customtabs.store.CustomTabsServiceStore +import mozilla.components.feature.downloads.DownloadMiddleware import mozilla.components.feature.downloads.DownloadsUseCases import mozilla.components.feature.intent.processing.TabIntentProcessor import mozilla.components.feature.media.RecordingDevicesNotificationFeature @@ -62,6 +63,7 @@ import mozilla.components.feature.webnotifications.WebNotificationFeature import mozilla.components.lib.fetch.httpurlconnection.HttpURLConnectionClient import mozilla.components.lib.nearby.NearbyConnection import org.mozilla.samples.browser.addons.AddonsActivity +import org.mozilla.samples.browser.downloads.DownloadService import org.mozilla.samples.browser.ext.components import org.mozilla.samples.browser.integration.FindInPageIntegration import org.mozilla.samples.browser.media.MediaService @@ -112,6 +114,7 @@ open class DefaultComponents(private val applicationContext: Context) { val store by lazy { BrowserStore(middleware = listOf( MediaMiddleware(applicationContext, MediaService::class.java), + DownloadMiddleware(applicationContext, DownloadService::class.java), ReaderViewMiddleware() )) } diff --git a/samples/browser/src/main/java/org/mozilla/samples/browser/downloads/DownloadService.kt b/samples/browser/src/main/java/org/mozilla/samples/browser/downloads/DownloadService.kt index be5218e51aa..189d42ec6ef 100644 --- a/samples/browser/src/main/java/org/mozilla/samples/browser/downloads/DownloadService.kt +++ b/samples/browser/src/main/java/org/mozilla/samples/browser/downloads/DownloadService.kt @@ -4,9 +4,11 @@ package org.mozilla.samples.browser.downloads +import mozilla.components.browser.state.store.BrowserStore import mozilla.components.feature.downloads.AbstractFetchDownloadService import org.mozilla.samples.browser.ext.components class DownloadService : AbstractFetchDownloadService() { override val httpClient by lazy { components.client } + override val store: BrowserStore by lazy { components.store } }