Fixes mozilla-mobile#3516 - Use response to determine file name

NotWoods · Jul 19, 2019 · fd7d8d1 · fd7d8d1
1 parent 8fdc655
commit fd7d8d1
Show file tree

Hide file tree

Showing 22 changed files with 105 additions and 204 deletions.
diff --git a/...ne-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt b/...ne-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt
@@ -508,7 +508,7 @@ class GeckoEngineSession(
         override fun onExternalResponse(session: GeckoSession, response: GeckoSession.WebResponseInfo) {
             notifyObservers {
                 val fileName = response.filename
-                    ?: DownloadUtils.guessFileName("", response.uri, response.contentType)
+                    ?: DownloadUtils.guessFileName(null, response.uri, response.contentType)
                 onExternalResource(
                         url = response.uri,
                         contentLength = response.contentLength,

diff --git a/...ecko-beta/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt b/...ecko-beta/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt
@@ -1545,37 +1545,6 @@ class GeckoEngineSessionTest {
         assertTrue(engineSession.geckoSession != oldGeckoSession)
     }
 
-    @Test
-    fun whenOnExternalResponseDoNotProvideAFileNameMustProvideMeaningFulFileNameToTheSessionObserver() {
-        val engineSession = GeckoEngineSession(mock())
-        var meaningFulFileName = ""
-
-        val observer = object : EngineSession.Observer {
-            override fun onExternalResource(
-                url: String,
-                fileName: String,
-                contentLength: Long?,
-                contentType: String?,
-                cookie: String?,
-                userAgent: String?
-            ) {
-                meaningFulFileName = fileName
-            }
-        }
-        engineSession.register(observer)
-
-        val info: GeckoSession.WebResponseInfo = MockWebResponseInfo(
-            uri = "http://ipv4.download.thinkbroadband.com/1MB.zip",
-            contentLength = 0,
-            contentType = "",
-            filename = null
-        )
-
-        engineSession.geckoSession.contentDelegate!!.onExternalResponse(mock(), info)
-
-        assertEquals("1MB.zip", meaningFulFileName)
-    }
-
     @Test
     fun `Closing engine session should close underlying gecko session`() {
         val geckoSession = mockGeckoSession()

diff --git a/...gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt b/...gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt
@@ -27,7 +27,6 @@ import mozilla.components.support.ktx.android.util.Base64
 import mozilla.components.support.ktx.kotlin.isEmail
 import mozilla.components.support.ktx.kotlin.isGeoLocation
 import mozilla.components.support.ktx.kotlin.isPhone
-import mozilla.components.support.utils.DownloadUtils
 import org.json.JSONObject
 import org.mozilla.geckoview.AllowOrDeny
 import org.mozilla.geckoview.ContentBlocking
@@ -513,13 +512,11 @@ class GeckoEngineSession(
 
         override fun onExternalResponse(session: GeckoSession, response: GeckoSession.WebResponseInfo) {
             notifyObservers {
-                val fileName = response.filename
-                    ?: DownloadUtils.guessFileName("", response.uri, response.contentType)
                 onExternalResource(
                         url = response.uri,
                         contentLength = response.contentLength,
                         contentType = response.contentType,
-                        fileName = fileName)
+                        fileName = response.filename)
             }
         }
 

diff --git a/...o-nightly/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt b/...o-nightly/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt
@@ -1542,37 +1542,6 @@ class GeckoEngineSessionTest {
         assertTrue(engineSession.geckoSession != oldGeckoSession)
     }
 
-    @Test
-    fun whenOnExternalResponseDoNotProvideAFileNameMustProvideMeaningFulFileNameToTheSessionObserver() {
-        val engineSession = GeckoEngineSession(mock())
-        var meaningFulFileName = ""
-
-        val observer = object : EngineSession.Observer {
-            override fun onExternalResource(
-                url: String,
-                fileName: String,
-                contentLength: Long?,
-                contentType: String?,
-                cookie: String?,
-                userAgent: String?
-            ) {
-                meaningFulFileName = fileName
-            }
-        }
-        engineSession.register(observer)
-
-        val info: GeckoSession.WebResponseInfo = MockWebResponseInfo(
-            uri = "http://ipv4.download.thinkbroadband.com/1MB.zip",
-            contentLength = 0,
-            contentType = "",
-            filename = null
-        )
-
-        engineSession.geckoSession.contentDelegate!!.onExternalResponse(mock(), info)
-
-        assertEquals("1MB.zip", meaningFulFileName)
-    }
-
     @Test
     fun `Closing engine session should close underlying gecko session`() {
         val geckoSession = mockGeckoSession()

diff --git a/.../engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt b/.../engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt
@@ -27,7 +27,6 @@ import mozilla.components.support.ktx.android.util.Base64
 import mozilla.components.support.ktx.kotlin.isEmail
 import mozilla.components.support.ktx.kotlin.isGeoLocation
 import mozilla.components.support.ktx.kotlin.isPhone
-import mozilla.components.support.utils.DownloadUtils
 import org.mozilla.geckoview.AllowOrDeny
 import org.mozilla.geckoview.ContentBlocking
 import org.mozilla.geckoview.GeckoResult
@@ -509,13 +508,11 @@ class GeckoEngineSession(
 
         override fun onExternalResponse(session: GeckoSession, response: GeckoSession.WebResponseInfo) {
             notifyObservers {
-                val fileName = response.filename
-                    ?: DownloadUtils.guessFileName("", response.uri, response.contentType)
                 onExternalResource(
                         url = response.uri,
                         contentLength = response.contentLength,
                         contentType = response.contentType,
-                        fileName = fileName)
+                        fileName = response.filename)
             }
         }
 

diff --git a/...ine-gecko/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt b/...ine-gecko/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt
@@ -234,8 +234,8 @@ class GeckoEngineSessionTest {
         val engineSession = GeckoEngineSession(mock(),
                 geckoSessionProvider = geckoSessionProvider)
 
-        var observedContentPermissionRequests: MutableList<PermissionRequest> = mutableListOf()
-        var observedAppPermissionRequests: MutableList<PermissionRequest> = mutableListOf()
+        val observedContentPermissionRequests: MutableList<PermissionRequest> = mutableListOf()
+        val observedAppPermissionRequests: MutableList<PermissionRequest> = mutableListOf()
         engineSession.register(object : EngineSession.Observer {
             override fun onContentPermissionRequest(permissionRequest: PermissionRequest) {
                 observedContentPermissionRequests.add(permissionRequest)
@@ -1518,37 +1518,6 @@ class GeckoEngineSessionTest {
         assertTrue(engineSession.geckoSession != oldGeckoSession)
     }
 
-    @Test
-    fun whenOnExternalResponseDoNotProvideAFileNameMustProvideMeaningFulFileNameToTheSessionObserver() {
-        val engineSession = GeckoEngineSession(mock())
-        var meaningFulFileName = ""
-
-        val observer = object : EngineSession.Observer {
-            override fun onExternalResource(
-                url: String,
-                fileName: String,
-                contentLength: Long?,
-                contentType: String?,
-                cookie: String?,
-                userAgent: String?
-            ) {
-                meaningFulFileName = fileName
-            }
-        }
-        engineSession.register(observer)
-
-        val info: GeckoSession.WebResponseInfo = MockWebResponseInfo(
-            uri = "http://ipv4.download.thinkbroadband.com/1MB.zip",
-            contentLength = 0,
-            contentType = "",
-            filename = null
-        )
-
-        engineSession.geckoSession.contentDelegate!!.onExternalResponse(mock(), info)
-
-        assertEquals("1MB.zip", meaningFulFileName)
-    }
-
     @Test
     fun `Closing engine session should close underlying gecko session`() {
         val geckoSession = mockGeckoSession()

diff --git a/...ine-system/src/test/java/mozilla/components/browser/engine/system/SystemEngineViewTest.kt b/...ine-system/src/test/java/mozilla/components/browser/engine/system/SystemEngineViewTest.kt
@@ -438,7 +438,7 @@ class SystemEngineViewTest {
         engineSession.register(object : EngineSession.Observer {
             override fun onExternalResource(
                 url: String,
-                fileName: String,
+                fileName: String?,
                 contentLength: Long?,
                 contentType: String?,
                 cookie: String?,

diff --git a/components/browser/session/src/main/java/mozilla/components/browser/session/Download.kt b/components/browser/session/src/main/java/mozilla/components/browser/session/Download.kt
@@ -19,7 +19,7 @@ import android.os.Environment
  */
 data class Download(
     val url: String,
-    val fileName: String,
+    val fileName: String? = null,
     val contentType: String? = null,
     val contentLength: Long? = null,
     val userAgent: String? = null,

diff --git a/...browser/session/src/main/java/mozilla/components/browser/session/engine/EngineObserver.kt b/...browser/session/src/main/java/mozilla/components/browser/session/engine/EngineObserver.kt
@@ -111,7 +111,7 @@ internal class EngineObserver(
 
     override fun onExternalResource(
         url: String,
-        fileName: String,
+        fileName: String?,
         contentLength: Long?,
         contentType: String?,
         cookie: String?,

diff --git a/components/concept/engine/src/main/java/mozilla/components/concept/engine/EngineSession.kt b/components/concept/engine/src/main/java/mozilla/components/concept/engine/EngineSession.kt
@@ -17,7 +17,6 @@ import mozilla.components.concept.engine.prompt.PromptRequest
 import mozilla.components.concept.engine.window.WindowRequest
 import mozilla.components.support.base.observer.Observable
 import mozilla.components.support.base.observer.ObserverRegistry
-import java.lang.UnsupportedOperationException
 
 /**
  * Class representing a single engine session.
@@ -71,7 +70,7 @@ abstract class EngineSession(
         @Suppress("LongParameterList")
         fun onExternalResource(
             url: String,
-            fileName: String,
+            fileName: String? = null,
             contentLength: Long? = null,
             contentType: String? = null,
             cookie: String? = null,

diff --git a/components/concept/fetch/src/main/java/mozilla/components/concept/fetch/Headers.kt b/components/concept/fetch/src/main/java/mozilla/components/concept/fetch/Headers.kt
@@ -159,3 +159,5 @@ class MutableHeaders(headers: List<Header>) : Headers, MutableIterable<Header> {
 
     override fun hashCode() = headers.hashCode()
 }
+
+fun List<Header>.toMutableHeaders() = MutableHeaders(this)
diff --git a/.../contextmenu/src/main/java/mozilla/components/feature/contextmenu/ContextMenuCandidate.kt b/.../contextmenu/src/main/java/mozilla/components/feature/contextmenu/ContextMenuCandidate.kt
@@ -15,7 +15,6 @@ import mozilla.components.browser.session.Session
 import mozilla.components.concept.engine.HitResult
 import mozilla.components.feature.tabs.TabsUseCases
 import mozilla.components.support.base.observer.Consumable
-import mozilla.components.support.utils.DownloadUtils
 
 /**
  * A candidate for an item to be displayed in the context menu.
@@ -149,9 +148,7 @@ data class ContextMenuCandidate(
             label = context.getString(R.string.mozac_feature_contextmenu_save_image),
             showFor = { _, hitResult -> hitResult.isImage() },
             action = { session, hitResult ->
-                session.download = Consumable.from(Download(
-                    hitResult.src,
-                    DownloadUtils.guessFileName(null, hitResult.src, null)))
+                session.download = Consumable.from(Download(hitResult.src))
             }
         )
 

diff --git a/...nloads/src/main/java/mozilla/components/feature/downloads/AbstractFetchDownloadService.kt b/...nloads/src/main/java/mozilla/components/feature/downloads/AbstractFetchDownloadService.kt
@@ -28,16 +28,14 @@ import kotlinx.coroutines.withContext
 import mozilla.components.browser.session.Download
 import mozilla.components.concept.fetch.Client
 import mozilla.components.concept.fetch.Header
-import mozilla.components.concept.fetch.Headers.Names.CONTENT_DISPOSITION
 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.MutableHeaders
 import mozilla.components.concept.fetch.Request
-import mozilla.components.concept.fetch.Response
+import mozilla.components.concept.fetch.toMutableHeaders
 import mozilla.components.feature.downloads.ext.addCompletedDownload
 import mozilla.components.feature.downloads.ext.getDownloadExtra
-import mozilla.components.feature.downloads.manager.getFileName
+import mozilla.components.feature.downloads.ext.withResponse
 import mozilla.components.support.base.ids.NotificationIds
 import java.io.File
 import java.io.FileOutputStream
@@ -83,15 +81,14 @@ abstract class AbstractFetchDownloadService(
             REFERRER to download.referrerUrl
         ).mapNotNull { (name, value) ->
             if (value.isNullOrBlank()) null else Header(name, value)
-        }
+        }.toMutableHeaders()
 
-        val request = Request(download.url, headers = MutableHeaders(headers))
+        val request = Request(download.url, headers = headers)
 
         val response = httpClient.fetch(request)
-        val filename = download.getFileName(response.headers[CONTENT_DISPOSITION])
 
         response.body.useStream { inStream ->
-            useFileStream(download, response, filename) { outStream ->
+            useFileStream(download.withResponse(response.headers, inStream)) { outStream ->
                 inStream.copyTo(outStream)
             }
         }
@@ -115,32 +112,21 @@ abstract class AbstractFetchDownloadService(
      */
     internal fun useFileStream(
         download: Download,
-        response: Response,
-        filename: String,
         block: (OutputStream) -> Unit
     ) {
         if (SDK_INT >= Build.VERSION_CODES.Q) {
-            useFileStreamScopedStorage(download, response, filename, block)
+            useFileStreamScopedStorage(download, block)
         } else {
-            useFileStreamLegacy(download, response, filename, block)
+            useFileStreamLegacy(download, block)
         }
     }
 
-    @Suppress("LongMethod")
     @TargetApi(Build.VERSION_CODES.Q)
-    private fun useFileStreamScopedStorage(
-        download: Download,
-        response: Response,
-        filename: String,
-        block: (OutputStream) -> Unit
-    ) {
-        val contentType = response.headers[CONTENT_TYPE]
-        val contentLength = response.headers[CONTENT_LENGTH]?.toLongOrNull()
-
+    private fun useFileStreamScopedStorage(download: Download, block: (OutputStream) -> Unit) {
         val values = ContentValues().apply {
-            put(MediaStore.Downloads.DISPLAY_NAME, filename)
-            put(MediaStore.Downloads.MIME_TYPE, contentType ?: download.contentType ?: "*/*")
-            put(MediaStore.Downloads.SIZE, contentLength ?: download.contentLength)
+            put(MediaStore.Downloads.DISPLAY_NAME, download.fileName)
+            put(MediaStore.Downloads.MIME_TYPE, download.contentType ?: "*/*")
+            put(MediaStore.Downloads.SIZE, download.contentLength)
             put(MediaStore.Downloads.IS_PENDING, 1)
         }
 
@@ -157,27 +143,19 @@ abstract class AbstractFetchDownloadService(
     }
 
     @TargetApi(Build.VERSION_CODES.P)
-    @Suppress("Deprecation", "LongMethod")
-    private fun useFileStreamLegacy(
-        download: Download,
-        response: Response,
-        filename: String,
-        block: (OutputStream) -> Unit
-    ) {
+    @Suppress("Deprecation")
+    private fun useFileStreamLegacy(download: Download, block: (OutputStream) -> Unit) {
         val dir = Environment.getExternalStoragePublicDirectory(download.destinationDirectory)
-        val file = File(dir, filename)
+        val file = File(dir, download.fileName!!)
         FileOutputStream(file).use(block)
 
-        val contentType = response.headers[CONTENT_TYPE]
-        val contentLength = response.headers[CONTENT_LENGTH]?.toLongOrNull()
-
         addCompletedDownload(
-            title = filename,
-            description = filename,
+            title = download.fileName!!,
+            description = download.fileName!!,
             isMediaScannerScannable = true,
-            mimeType = contentType ?: download.contentType ?: "*/*",
+            mimeType = download.contentType ?: "*/*",
             path = file.absolutePath,
-            length = contentLength ?: download.contentLength ?: file.length(),
+            length = download.contentLength ?: file.length(),
             showNotification = true,
             uri = download.url.toUri(),
             referer = download.referrerUrl?.toUri()

diff --git a/...re/downloads/src/main/java/mozilla/components/feature/downloads/DownloadDialogFragment.kt b/...re/downloads/src/main/java/mozilla/components/feature/downloads/DownloadDialogFragment.kt
@@ -7,6 +7,7 @@ package mozilla.components.feature.downloads
 import android.os.Bundle
 import androidx.fragment.app.DialogFragment
 import mozilla.components.browser.session.Download
+import mozilla.components.support.utils.DownloadUtils
 
 /**
  * This is a general representation of a dialog meant to be used in collaboration with [DownloadsFeature]
@@ -28,7 +29,8 @@ abstract class DownloadDialogFragment : DialogFragment() {
      */
     fun setDownload(download: Download) {
         val args = arguments ?: Bundle()
-        args.putString(KEY_FILE_NAME, download.fileName)
+        args.putString(KEY_FILE_NAME, download.fileName
+            ?: DownloadUtils.guessFileName(null, download.url, download.contentType))
         args.putString(KEY_URL, download.url)
         args.putLong(KEY_CONTENT_LENGTH, download.contentLength ?: 0)
         arguments = args
Original file line number	Diff line number	Diff line change
Expand Up		@@ -159,3 +159,5 @@ class MutableHeaders(headers: List<Header>) : Headers, MutableIterable<Header> {

		override fun hashCode() = headers.hashCode()
		}

		fun List<Header>.toMutableHeaders() = MutableHeaders(this)