Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Commit

Permalink
Closes #8312:Adds support for ContentDelegate#onExternalResponse
Browse files Browse the repository at this point in the history
  • Loading branch information
Amejia481 authored and mergify[bot] committed Oct 5, 2020
1 parent c174f59 commit 449abd4
Show file tree
Hide file tree
Showing 15 changed files with 142 additions and 40 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.Job
import kotlinx.coroutines.MainScope
import kotlinx.coroutines.launch
import mozilla.components.browser.engine.gecko.fetch.toResponse
import mozilla.components.browser.engine.gecko.media.GeckoMediaDelegate
import mozilla.components.browser.engine.gecko.permission.GeckoPermissionRequest
import mozilla.components.browser.engine.gecko.prompt.GeckoPromptDelegate
Expand All @@ -30,6 +31,9 @@ import mozilla.components.concept.engine.manifest.WebAppManifestParser
import mozilla.components.concept.engine.request.RequestInterceptor
import mozilla.components.concept.engine.request.RequestInterceptor.InterceptionResponse
import mozilla.components.concept.engine.window.WindowRequest
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.storage.PageVisit
import mozilla.components.concept.storage.RedirectSource
import mozilla.components.concept.storage.VisitType
Expand All @@ -39,7 +43,9 @@ import mozilla.components.support.ktx.kotlin.isExtensionUrl
import mozilla.components.support.ktx.kotlin.isGeoLocation
import mozilla.components.support.ktx.kotlin.isPhone
import mozilla.components.support.ktx.kotlin.tryGetHostFromUrl
import mozilla.components.support.utils.DownloadUtils
import org.json.JSONObject
import org.mozilla.geckoview.WebResponse
import org.mozilla.geckoview.AllowOrDeny
import org.mozilla.geckoview.ContentBlocking
import org.mozilla.geckoview.GeckoResult
Expand Down Expand Up @@ -744,15 +750,30 @@ class GeckoEngineSession(
notifyObservers { onFullScreenChange(fullScreen) }
}

override fun onExternalResponse(session: GeckoSession, response: GeckoSession.WebResponseInfo) {
notifyObservers {
onExternalResource(
url = response.uri,
contentLength = response.contentLength,
contentType = response.contentType,
fileName = response.filename,
isPrivate = privateMode
override fun onExternalResponse(session: GeckoSession, webResponse: WebResponse) {
with(webResponse) {
val contentType = headers[CONTENT_TYPE]?.trim()
val contentLength = headers[CONTENT_LENGTH]?.trim()?.toLong()
val contentDisposition = headers[CONTENT_DISPOSITION]?.trim()
val url = uri
val fileName = DownloadUtils.guessFileName(
contentDisposition,
destinationDirectory = null,
url = url,
mimeType = contentType
)
val response = webResponse.toResponse(isBlobUri = url.startsWith("blob:"))

notifyObservers {
onExternalResource(
url = url,
contentLength = contentLength,
contentType = contentType,
fileName = fileName,
response = response,
isPrivate = privateMode
)
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@ private fun WebRequest.Builder.addBodyFrom(request: Request): WebRequest.Builder
return this
}

@VisibleForTesting
internal fun WebResponse.toResponse(isBlobUri: Boolean): Response {
val headers = translateHeaders(this)
// We use the same API for blobs and HTTP requests, but blobs won't receive a status code.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ import mozilla.components.concept.engine.manifest.WebAppManifest
import mozilla.components.concept.engine.permission.PermissionRequest
import mozilla.components.concept.engine.request.RequestInterceptor
import mozilla.components.concept.engine.window.WindowRequest
import mozilla.components.concept.fetch.Headers
import mozilla.components.concept.fetch.Response
import mozilla.components.concept.storage.PageVisit
import mozilla.components.concept.storage.RedirectSource
import mozilla.components.concept.storage.VisitType
Expand All @@ -39,6 +41,7 @@ import mozilla.components.support.test.mock
import mozilla.components.support.test.whenever
import mozilla.components.support.utils.ThreadUtils
import mozilla.components.test.ReflectionUtils
import mozilla.components.support.test.argumentCaptor
import org.json.JSONObject
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
Expand All @@ -61,6 +64,7 @@ import org.mockito.Mockito.spy
import org.mockito.Mockito.times
import org.mockito.Mockito.verify
import org.mockito.Mockito.verifyZeroInteractions
import org.mozilla.geckoview.WebResponse
import org.mozilla.geckoview.AllowOrDeny
import org.mozilla.geckoview.ContentBlocking
import org.mozilla.geckoview.ContentBlockingController
Expand All @@ -73,7 +77,6 @@ import org.mozilla.geckoview.GeckoSession.ContentDelegate.ContextElement.TYPE_NO
import org.mozilla.geckoview.GeckoSession.ContentDelegate.ContextElement.TYPE_VIDEO
import org.mozilla.geckoview.GeckoSession.ProgressDelegate.SecurityInformation
import org.mozilla.geckoview.GeckoSessionSettings
import org.mozilla.geckoview.MockWebResponseInfo
import org.mozilla.geckoview.SessionFinder
import org.mozilla.geckoview.WebRequestError
import org.mozilla.geckoview.WebRequestError.ERROR_CATEGORY_UNKNOWN
Expand Down Expand Up @@ -271,24 +274,27 @@ class GeckoEngineSessionTest {
val observer: EngineSession.Observer = mock()
engineSession.register(observer)

val info: GeckoSession.WebResponseInfo = MockWebResponseInfo(
uri = "https://download.mozilla.org",
contentLength = 42,
contentType = "image/png",
filename = "image.png"
)
val response = WebResponse.Builder("https://download.mozilla.org/image.png")
.addHeader(Headers.Names.CONTENT_TYPE, "image/png")
.addHeader(Headers.Names.CONTENT_LENGTH, "42")
.body(mock())
.build()

val captor = argumentCaptor<Response>()
captureDelegates()
contentDelegate.value.onExternalResponse(mock(), info)
contentDelegate.value.onExternalResponse(mock(), response)

verify(observer).onExternalResource(
url = "https://download.mozilla.org",
fileName = "image.png",
contentLength = 42,
contentType = "image/png",
isPrivate = true,
userAgent = null,
cookie = null)
url = eq("https://download.mozilla.org/image.png"),
fileName = eq("image.png"),
contentLength = eq(42),
contentType = eq("image/png"),
userAgent = eq(null),
cookie = eq(null),
isPrivate = eq(true),
response = captor.capture()
)
assertNotNull(captor.value)
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ import org.robolectric.RuntimeEnvironment
import org.robolectric.annotation.Config
import mozilla.components.concept.engine.EngineSession.TrackingProtectionPolicy.TrackingCategory
import mozilla.components.concept.engine.content.blocking.Tracker
import mozilla.components.concept.fetch.Response
import java.io.StringReader

@RunWith(AndroidJUnit4::class)
Expand Down Expand Up @@ -442,7 +443,8 @@ class SystemEngineViewTest {
contentType: String?,
cookie: String?,
userAgent: String?,
isPrivate: Boolean
isPrivate: Boolean,
response: Response?
) {
assertEquals("https://download.mozilla.org", url)
assertEquals("image.png", fileName)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import mozilla.components.concept.engine.media.RecordingDevice
import mozilla.components.concept.engine.permission.PermissionRequest
import mozilla.components.concept.engine.prompt.PromptRequest
import mozilla.components.concept.engine.window.WindowRequest
import mozilla.components.concept.fetch.Response
import mozilla.components.lib.state.Store
import mozilla.components.support.base.observer.Consumable
import mozilla.components.support.ktx.android.net.isInScope
Expand Down Expand Up @@ -203,7 +204,8 @@ internal class EngineObserver(
contentType: String?,
cookie: String?,
userAgent: String?,
isPrivate: Boolean
isPrivate: Boolean,
response: Response?
) {
// We want to avoid negative contentLength values
// For more info see https://bugzilla.mozilla.org/show_bug.cgi?id=1632594
Expand All @@ -217,7 +219,8 @@ internal class EngineObserver(
INITIATED,
userAgent,
Environment.DIRECTORY_DOWNLOADS,
private = isPrivate
private = isPrivate,
response = response
)

store?.dispatch(ContentAction.UpdateDownloadAction(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import mozilla.components.concept.engine.media.Media
import mozilla.components.concept.engine.permission.PermissionRequest
import mozilla.components.concept.engine.prompt.PromptRequest
import mozilla.components.concept.engine.window.WindowRequest
import mozilla.components.concept.fetch.Response
import mozilla.components.support.base.observer.Consumable
import mozilla.components.support.test.any
import mozilla.components.support.test.libstate.ext.waitUntilIdle
Expand Down Expand Up @@ -700,7 +701,7 @@ class EngineObserverTest {
@Test
fun `onExternalResource will update the store`() {
val store = BrowserStore()

val response = mock<Response>()
val sessionManager = SessionManager(engine = mock(), store = store)

val session = Session("https://www.mozilla.org", id = "test-tab").also {
Expand All @@ -715,7 +716,8 @@ class EngineObserverTest {
userAgent = "userAgent",
contentType = "text/plain",
isPrivate = true,
contentLength = 100L)
contentLength = 100L,
response = response)

store.waitUntilIdle()

Expand All @@ -727,6 +729,7 @@ class EngineObserverTest {
assertEquals("text/plain", tab.content.download?.contentType)
assertEquals(100L, tab.content.download?.contentLength)
assertEquals(true, tab.content.download?.private)
assertEquals(response, tab.content.download?.response)
}

@Test
Expand Down
4 changes: 4 additions & 0 deletions components/browser/state/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ dependencies {
implementation project(':support-utils')
implementation project(':support-ktx')

// We expose this as API because we are using Response in our public API and do not want every
// consumer to have to manually import "concept-fetch".
api project(':concept-fetch')

implementation Dependencies.androidx_browser
implementation Dependencies.kotlin_coroutines
implementation Dependencies.kotlin_stdlib
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@
package mozilla.components.browser.state.state.content

import android.os.Environment
import android.os.Parcelable
import kotlinx.android.parcel.Parcelize
import mozilla.components.concept.fetch.Response
import java.util.UUID

/**
Expand All @@ -26,10 +25,10 @@ import java.util.UUID
* @property id The unique identifier of this download.
* @property private Indicates if the download was created from a private session.
* @property createdTime A timestamp when the download was created.
* @
* @property response A response object associated with this request, when provided can be
* used instead of performing a manual a download.
*/
@Suppress("Deprecation")
@Parcelize
data class DownloadState(
val url: String,
val fileName: String? = null,
Expand All @@ -44,8 +43,9 @@ data class DownloadState(
val id: String = UUID.randomUUID().toString(),
val sessionId: String? = null,
val private: Boolean = false,
val createdTime: Long = System.currentTimeMillis()
) : Parcelable {
val createdTime: Long = System.currentTimeMillis(),
val response: Response? = null
) {
val filePath: String get() =
Environment.getExternalStoragePublicDirectory(destinationDirectory).path + "/" + fileName

Expand Down
2 changes: 1 addition & 1 deletion components/concept/engine/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ android {

dependencies {
implementation project(':support-ktx')

implementation Dependencies.kotlin_stdlib
implementation Dependencies.androidx_annotation
implementation Dependencies.kotlin_coroutines
Expand All @@ -37,6 +36,7 @@ dependencies {
api project(':support-base')
api project(':browser-errorpages')
api project(':concept-storage')
api project(':concept-fetch')

testImplementation Dependencies.androidx_test_core
testImplementation Dependencies.androidx_test_junit
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import mozilla.components.concept.engine.media.RecordingDevice
import mozilla.components.concept.engine.permission.PermissionRequest
import mozilla.components.concept.engine.prompt.PromptRequest
import mozilla.components.concept.engine.window.WindowRequest
import mozilla.components.concept.fetch.Response
import mozilla.components.support.base.observer.Observable
import mozilla.components.support.base.observer.ObserverRegistry

Expand Down Expand Up @@ -133,6 +134,8 @@ abstract class EngineSession(
* @param cookie The cookie related to request.
* @param userAgent The user agent of the engine.
* @param isPrivate Indicates if the download was requested from a private session.
* @param response A response object associated with this request, when provided can be
* used instead of performing a manual a download.
*/
@Suppress("LongParameterList")
fun onExternalResource(
Expand All @@ -142,7 +145,8 @@ abstract class EngineSession(
contentType: String? = null,
cookie: String? = null,
userAgent: String? = null,
isPrivate: Boolean = false
isPrivate: Boolean = false,
response: Response? = null
) = Unit

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -607,7 +607,7 @@ abstract class AbstractFetchDownloadService : Service() {
}

val request = Request(download.url.sanitizeURL(), headers = headers, cookiePolicy = cookiePolicy)
val response = httpClient.fetch(request)
val response = download.response ?: httpClient.fetch(request)
logger.debug("Fetching download for ${currentDownloadJobState.state.id} ")

// If we are resuming a download and the response does not contain a CONTENT_RANGE
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,14 +69,17 @@ import org.mockito.ArgumentMatchers.anyString
import org.mockito.ArgumentMatchers.anyLong
import org.mockito.ArgumentMatchers.isNull
import org.mockito.Mock
import org.mockito.Mockito.atLeastOnce
import org.mockito.Mockito.doCallRealMethod
import org.mockito.Mockito.doNothing
import org.mockito.Mockito.doReturn
import org.mockito.Mockito.doThrow
import org.mockito.Mockito.never
import org.mockito.Mockito.spy
import org.mockito.Mockito.times
import org.mockito.Mockito.verify
import org.mockito.Mockito.never
import org.mockito.Mockito.verifyZeroInteractions
import org.mockito.MockitoAnnotations.initMocks
import org.robolectric.Shadows.shadowOf
import org.robolectric.annotation.Config
Expand Down Expand Up @@ -1028,6 +1031,36 @@ class AbstractFetchDownloadServiceTest {
assertEquals(Request.CookiePolicy.INCLUDE, providedRequest.value.cookiePolicy)
}

@Test
fun `performDownload - use the download response when available`() {
val responseFromDownloadState = mock<Response>()
val responseFromClient = mock<Response>()
val download = DownloadState("https://example.com/file.txt", "file.txt", response = responseFromDownloadState)
val downloadJob = DownloadJobState(state = download, status = DOWNLOADING)

doReturn(404).`when`(responseFromDownloadState).status
doReturn(responseFromClient).`when`(client).fetch(any())

service.performDownload(downloadJob)

verify(responseFromDownloadState, atLeastOnce()).status
verifyZeroInteractions(client)
}

@Test
fun `performDownload - use the client response when the download response NOT available`() {
val responseFromClient = mock<Response>()
val download = spy(DownloadState("https://example.com/file.txt", "file.txt", response = null))
val downloadJob = DownloadJobState(state = download, status = DOWNLOADING)

doReturn(404).`when`(responseFromClient).status
doReturn(responseFromClient).`when`(client).fetch(any())

service.performDownload(downloadJob)

verify(responseFromClient, atLeastOnce()).status
}

@Test
fun `onDestroy cancels all running jobs`() = runBlocking {
val download = DownloadState("https://example.com/file.txt", "file.txt")
Expand Down
Loading

0 comments on commit 449abd4

Please sign in to comment.