Commit 464f2b0c authored by MozLando's avatar MozLando
Browse files

Merge #4901



4901: Closes #4553: Migrate WindowFeature to browser-state r=grigoryk a=csadilek

This migrates our `WindowFeature` to `browser-state` and deletes the relevant code from `browser-session`.

Instead of keeping two distinct states for open and close window requests, this uses just one, which simplifies store observers (e.g the feature implementations). This wasn't really useful before anyway, so I decided to simplify here and introduce a type instead.

The `WindowFeature` now also uses our `TabsUseCases` to open and close tabs in response to window requests, instead of interacting with the `SessionManager` directly. This follows the same pattern as our other migrations. The use cases will be the final classes to migrate once everything else is done. For this to work, we need to move `WindowFeature` from `feature-session` to `feature-tabs` because otherwise we'd be introducing a dependency cycle. This is a better fit anyway, as the feature reacts to window request by opening and closing tabs, and a separate feature module seems unnecessary.  
Co-authored-by: default avatarChristian Sadilek <christian.sadilek@gmail.com>
parents 6f38790d 30f41b8e
......@@ -25,6 +25,7 @@ import mozilla.components.concept.engine.history.HistoryTrackingDelegate
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.storage.PageVisit
import mozilla.components.concept.storage.RedirectSource
import mozilla.components.concept.storage.VisitType
......@@ -396,7 +397,7 @@ class GeckoEngineSession(
val newEngineSession = GeckoEngineSession(runtime, privateMode, defaultSettings, openGeckoSession = false)
notifyObservers {
MainScope().launch {
onOpenWindowRequest(GeckoWindowRequest(uri, newEngineSession))
onWindowRequest(GeckoWindowRequest(uri, newEngineSession))
}
}
return GeckoResult.fromValue(newEngineSession.geckoSession)
......@@ -607,7 +608,15 @@ class GeckoEngineSession(
}
}
override fun onCloseRequest(session: GeckoSession) = Unit
override fun onCloseRequest(session: GeckoSession) {
notifyObservers {
onWindowRequest(GeckoWindowRequest(
engineSession = this@GeckoEngineSession,
type = WindowRequest.Type.CLOSE
)
)
}
}
override fun onTitleChange(session: GeckoSession, title: String?) {
if (!privateMode) {
......
......@@ -12,8 +12,9 @@ import mozilla.components.concept.engine.window.WindowRequest
* Gecko-based implementation of [WindowRequest].
*/
class GeckoWindowRequest(
override val url: String,
private val engineSession: GeckoEngineSession
override val url: String = "",
private val engineSession: GeckoEngineSession,
override val type: WindowRequest.Type = WindowRequest.Type.OPEN
) : WindowRequest {
override fun prepare(): EngineSession {
......
......@@ -40,6 +40,7 @@ import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertNotNull
import org.junit.Assert.assertNull
import org.junit.Assert.assertSame
import org.junit.Assert.assertTrue
import org.junit.Before
import org.junit.Test
......@@ -2019,8 +2020,9 @@ class GeckoEngineSessionTest {
var receivedWindowRequest: WindowRequest? = null
engineSession.register(object : EngineSession.Observer {
override fun onOpenWindowRequest(windowRequest: WindowRequest) {
override fun onWindowRequest(windowRequest: WindowRequest) {
receivedWindowRequest = windowRequest
assertEquals(WindowRequest.Type.OPEN, windowRequest.type)
}
})
......@@ -2030,6 +2032,27 @@ class GeckoEngineSessionTest {
assertEquals("mozilla.org", receivedWindowRequest!!.url)
}
@Test
fun `onCloseRequest creates window request`() {
val engineSession = GeckoEngineSession(mock(), geckoSessionProvider = geckoSessionProvider)
captureDelegates()
var receivedWindowRequest: WindowRequest? = null
engineSession.register(object : EngineSession.Observer {
override fun onWindowRequest(windowRequest: WindowRequest) {
receivedWindowRequest = windowRequest
}
})
contentDelegate.value.onCloseRequest(geckoSession)
assertNotNull(receivedWindowRequest)
assertSame(engineSession, receivedWindowRequest!!.prepare())
assertEquals(WindowRequest.Type.CLOSE, receivedWindowRequest!!.type)
}
private fun mockGeckoSession(): GeckoSession {
val session = mock<GeckoSession>()
whenever(session.settings).thenReturn(
......
......@@ -25,6 +25,7 @@ import mozilla.components.concept.engine.history.HistoryTrackingDelegate
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.storage.PageVisit
import mozilla.components.concept.storage.RedirectSource
import mozilla.components.concept.storage.VisitType
......@@ -396,7 +397,7 @@ class GeckoEngineSession(
val newEngineSession = GeckoEngineSession(runtime, privateMode, defaultSettings, openGeckoSession = false)
notifyObservers {
MainScope().launch {
onOpenWindowRequest(GeckoWindowRequest(uri, newEngineSession))
onWindowRequest(GeckoWindowRequest(uri, newEngineSession))
}
}
return GeckoResult.fromValue(newEngineSession.geckoSession)
......@@ -607,7 +608,15 @@ class GeckoEngineSession(
}
}
override fun onCloseRequest(session: GeckoSession) = Unit
override fun onCloseRequest(session: GeckoSession) {
notifyObservers {
onWindowRequest(GeckoWindowRequest(
engineSession = this@GeckoEngineSession,
type = WindowRequest.Type.CLOSE
)
)
}
}
override fun onTitleChange(session: GeckoSession, title: String?) {
if (!privateMode) {
......
......@@ -12,8 +12,9 @@ import mozilla.components.concept.engine.window.WindowRequest
* Gecko-based implementation of [WindowRequest].
*/
class GeckoWindowRequest(
override val url: String,
private val engineSession: GeckoEngineSession
override val url: String = "",
private val engineSession: GeckoEngineSession,
override val type: WindowRequest.Type = WindowRequest.Type.OPEN
) : WindowRequest {
override fun prepare(): EngineSession {
......
......@@ -40,6 +40,7 @@ import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertNotNull
import org.junit.Assert.assertNull
import org.junit.Assert.assertSame
import org.junit.Assert.assertTrue
import org.junit.Before
import org.junit.Test
......@@ -2019,7 +2020,7 @@ class GeckoEngineSessionTest {
var receivedWindowRequest: WindowRequest? = null
engineSession.register(object : EngineSession.Observer {
override fun onOpenWindowRequest(windowRequest: WindowRequest) {
override fun onWindowRequest(windowRequest: WindowRequest) {
receivedWindowRequest = windowRequest
}
})
......@@ -2028,6 +2029,28 @@ class GeckoEngineSessionTest {
assertNotNull(receivedWindowRequest)
assertEquals("mozilla.org", receivedWindowRequest!!.url)
assertEquals(WindowRequest.Type.OPEN, receivedWindowRequest!!.type)
}
@Test
fun `onCloseRequest creates window request`() {
val engineSession = GeckoEngineSession(mock(), geckoSessionProvider = geckoSessionProvider)
captureDelegates()
var receivedWindowRequest: WindowRequest? = null
engineSession.register(object : EngineSession.Observer {
override fun onWindowRequest(windowRequest: WindowRequest) {
receivedWindowRequest = windowRequest
}
})
contentDelegate.value.onCloseRequest(geckoSession)
assertNotNull(receivedWindowRequest)
assertSame(engineSession, receivedWindowRequest!!.prepare())
assertEquals(WindowRequest.Type.CLOSE, receivedWindowRequest!!.type)
}
private fun mockGeckoSession(): GeckoSession {
......
......@@ -25,6 +25,7 @@ import mozilla.components.concept.engine.history.HistoryTrackingDelegate
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.storage.PageVisit
import mozilla.components.concept.storage.RedirectSource
import mozilla.components.concept.storage.VisitType
......@@ -397,7 +398,7 @@ class GeckoEngineSession(
val newEngineSession = GeckoEngineSession(runtime, privateMode, defaultSettings, openGeckoSession = false)
notifyObservers {
MainScope().launch {
onOpenWindowRequest(GeckoWindowRequest(uri, newEngineSession))
onWindowRequest(GeckoWindowRequest(uri, newEngineSession))
}
}
return GeckoResult.fromValue(newEngineSession.geckoSession)
......@@ -608,7 +609,15 @@ class GeckoEngineSession(
}
}
override fun onCloseRequest(session: GeckoSession) = Unit
override fun onCloseRequest(session: GeckoSession) {
notifyObservers {
onWindowRequest(GeckoWindowRequest(
engineSession = this@GeckoEngineSession,
type = WindowRequest.Type.CLOSE
)
)
}
}
override fun onTitleChange(session: GeckoSession, title: String?) {
if (!privateMode) {
......
......@@ -12,8 +12,9 @@ import mozilla.components.concept.engine.window.WindowRequest
* Gecko-based implementation of [WindowRequest].
*/
class GeckoWindowRequest(
override val url: String,
private val engineSession: GeckoEngineSession
override val url: String = "",
private val engineSession: GeckoEngineSession,
override val type: WindowRequest.Type = WindowRequest.Type.OPEN
) : WindowRequest {
override fun prepare(): EngineSession {
......
......@@ -40,6 +40,7 @@ import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertNotNull
import org.junit.Assert.assertNull
import org.junit.Assert.assertSame
import org.junit.Assert.assertTrue
import org.junit.Before
import org.junit.Test
......@@ -2021,8 +2022,9 @@ class GeckoEngineSessionTest {
var receivedWindowRequest: WindowRequest? = null
engineSession.register(object : EngineSession.Observer {
override fun onOpenWindowRequest(windowRequest: WindowRequest) {
override fun onWindowRequest(windowRequest: WindowRequest) {
receivedWindowRequest = windowRequest
assertEquals(WindowRequest.Type.OPEN, windowRequest.type)
}
})
......@@ -2032,6 +2034,27 @@ class GeckoEngineSessionTest {
assertEquals("mozilla.org", receivedWindowRequest!!.url)
}
@Test
fun `onCloseRequest creates window request`() {
val engineSession = GeckoEngineSession(mock(), geckoSessionProvider = geckoSessionProvider)
captureDelegates()
var receivedWindowRequest: WindowRequest? = null
engineSession.register(object : EngineSession.Observer {
override fun onWindowRequest(windowRequest: WindowRequest) {
receivedWindowRequest = windowRequest
}
})
contentDelegate.value.onCloseRequest(geckoSession)
assertNotNull(receivedWindowRequest)
assertSame(engineSession, receivedWindowRequest!!.prepare())
assertEquals(WindowRequest.Type.CLOSE, receivedWindowRequest!!.type)
}
private fun mockGeckoSession(): GeckoSession {
val session = mock<GeckoSession>()
whenever(session.settings).thenReturn(
......
......@@ -55,6 +55,7 @@ import mozilla.components.concept.engine.HitResult
import mozilla.components.concept.engine.content.blocking.Tracker
import mozilla.components.concept.engine.prompt.PromptRequest
import mozilla.components.concept.engine.request.RequestInterceptor.InterceptionResponse
import mozilla.components.concept.engine.window.WindowRequest
import mozilla.components.concept.storage.PageVisit
import mozilla.components.concept.storage.RedirectSource
import mozilla.components.concept.storage.VisitType
......@@ -548,7 +549,7 @@ class SystemEngineView @JvmOverloads constructor(
): Boolean {
session?.internalNotifyObservers {
val newEngineSession = SystemEngineSession(context, session?.settings)
onOpenWindowRequest(SystemWindowRequest(
onWindowRequest(SystemWindowRequest(
view, newEngineSession, NestedWebView(context), isDialog, isUserGesture, resultMsg
))
}
......@@ -556,7 +557,9 @@ class SystemEngineView @JvmOverloads constructor(
}
override fun onCloseWindow(window: WebView) {
session?.internalNotifyObservers { onCloseWindowRequest(SystemWindowRequest(window)) }
session?.internalNotifyObservers {
onWindowRequest(SystemWindowRequest(window, type = WindowRequest.Type.CLOSE))
}
}
}
......
......@@ -26,7 +26,8 @@ class SystemWindowRequest(
private val newWebView: WebView? = null,
val openAsDialog: Boolean = false,
val triggeredByUser: Boolean = false,
private val resultMsg: Message? = null
private val resultMsg: Message? = null,
override val type: WindowRequest.Type = WindowRequest.Type.OPEN
) : WindowRequest {
override val url: String = ""
......
......@@ -1092,19 +1092,20 @@ class SystemEngineViewTest {
var createWindowRequest: WindowRequest? = null
var closeWindowRequest: WindowRequest? = null
engineSession.register(object : EngineSession.Observer {
override fun onOpenWindowRequest(windowRequest: WindowRequest) {
createWindowRequest = windowRequest
}
override fun onCloseWindowRequest(windowRequest: WindowRequest) {
closeWindowRequest = windowRequest
override fun onWindowRequest(windowRequest: WindowRequest) {
if (windowRequest.type == WindowRequest.Type.OPEN) {
createWindowRequest = windowRequest
} else {
closeWindowRequest = windowRequest
}
}
})
engineSession.webView.webChromeClient!!.onCreateWindow(mock<WebView>(), false, false, null)
engineSession.webView.webChromeClient!!.onCreateWindow(mock(), false, false, null)
assertNotNull(createWindowRequest)
assertNull(closeWindowRequest)
engineSession.webView.webChromeClient!!.onCloseWindow(mock<WebView>())
engineSession.webView.webChromeClient!!.onCloseWindow(mock())
assertNotNull(closeWindowRequest)
}
......
......@@ -37,7 +37,6 @@ import mozilla.components.concept.engine.manifest.WebAppManifest
import mozilla.components.concept.engine.media.Media
import mozilla.components.concept.engine.media.RecordingDevice
import mozilla.components.concept.engine.permission.PermissionRequest
import mozilla.components.concept.engine.window.WindowRequest
import mozilla.components.support.base.observer.Consumable
import mozilla.components.support.base.observer.Observable
import mozilla.components.support.base.observer.ObserverRegistry
......@@ -100,8 +99,6 @@ class Session(
fun onThumbnailChanged(session: Session, bitmap: Bitmap?) = Unit
fun onContentPermissionRequested(session: Session, permissionRequest: PermissionRequest): Boolean = false
fun onAppPermissionRequested(session: Session, permissionRequest: PermissionRequest): Boolean = false
fun onOpenWindowRequested(session: Session, windowRequest: WindowRequest): Boolean = false
fun onCloseWindowRequested(session: Session, windowRequest: WindowRequest): Boolean = false
fun onMediaRemoved(session: Session, media: List<Media>, removed: Media) = Unit
fun onMediaAdded(session: Session, media: List<Media>, added: Media) = Unit
fun onCrashStateChanged(session: Session, crashed: Boolean) = Unit
......@@ -445,22 +442,6 @@ class Session(
!request.consumeBy(consumers)
}
/**
* [Consumable] request to open/create a window.
*/
var openWindowRequest: Consumable<WindowRequest> by Delegates.vetoable(Consumable.empty()) { _, _, request ->
val consumers = wrapConsumers<WindowRequest> { onOpenWindowRequested(this@Session, it) }
!request.consumeBy(consumers)
}
/**
* [Consumable] request to close a window.
*/
var closeWindowRequest: Consumable<WindowRequest> by Delegates.vetoable(Consumable.empty()) { _, _, request ->
val consumers = wrapConsumers<WindowRequest> { onCloseWindowRequested(this@Session, it) }
!request.consumeBy(consumers)
}
/**
* Whether this [Session] has crashed.
*
......
......@@ -192,12 +192,13 @@ internal class EngineObserver(
))
}
override fun onOpenWindowRequest(windowRequest: WindowRequest) {
session.openWindowRequest = Consumable.from(windowRequest)
}
override fun onCloseWindowRequest(windowRequest: WindowRequest) {
session.closeWindowRequest = Consumable.from(windowRequest)
override fun onWindowRequest(windowRequest: WindowRequest) {
store?.dispatch(
ContentAction.UpdateWindowRequestAction(
store.state.selectedTabId ?: session.id,
windowRequest
)
)
}
override fun onMediaAdded(media: Media) {
......
......@@ -27,7 +27,6 @@ import mozilla.components.concept.engine.manifest.WebAppManifest
import mozilla.components.concept.engine.media.Media
import mozilla.components.concept.engine.media.RecordingDevice
import mozilla.components.concept.engine.permission.PermissionRequest
import mozilla.components.concept.engine.window.WindowRequest
import mozilla.components.support.base.observer.Consumable
import mozilla.components.support.test.any
import mozilla.components.support.test.argumentCaptor
......@@ -684,7 +683,6 @@ class SessionTest {
val defaultObserver = object : Session.Observer {}
val contentPermissionRequest: PermissionRequest = mock()
val appPermissionRequest: PermissionRequest = mock()
val windowRequest: WindowRequest = mock()
defaultObserver.onUrlChanged(session, "")
defaultObserver.onTitleChanged(session, "")
......@@ -703,8 +701,6 @@ class SessionTest {
defaultObserver.onThumbnailChanged(session, spy(Bitmap::class.java))
defaultObserver.onContentPermissionRequested(session, contentPermissionRequest)
defaultObserver.onAppPermissionRequested(session, appPermissionRequest)
defaultObserver.onOpenWindowRequested(session, windowRequest)
defaultObserver.onCloseWindowRequested(session, windowRequest)
defaultObserver.onWebAppManifestChanged(session, mock())
defaultObserver.onMediaAdded(session, emptyList(), mock())
defaultObserver.onMediaRemoved(session, emptyList(), mock())
......@@ -768,62 +764,6 @@ class SessionTest {
assertTrue(session.appPermissionRequest.isConsumed())
}
@Test
fun `window requests will be set on session if no observer consumes them`() {
val openWindowRequest: WindowRequest = mock()
val closeWindowRequest: WindowRequest = mock()
val session = Session("https://www.mozilla.org")
session.openWindowRequest = Consumable.from(openWindowRequest)
session.closeWindowRequest = Consumable.from(closeWindowRequest)
assertFalse(session.openWindowRequest.isConsumed())
assertFalse(session.closeWindowRequest.isConsumed())
var createWindowRequestIsSet = false
var closeWindowRequestIsSet = false
session.openWindowRequest.consume {
createWindowRequestIsSet = true
true
}
session.closeWindowRequest.consume {
closeWindowRequestIsSet = true
true
}
assertTrue(createWindowRequestIsSet)
assertTrue(closeWindowRequestIsSet)
}
@Test
fun `window requests will not be set on session if consumed by observer`() {
var openWindowRequestExecuted = false
var closeWindowRequestExecuted = false
val session = Session("https://www.mozilla.org")
session.register(object : Session.Observer {
override fun onOpenWindowRequested(session: Session, windowRequest: WindowRequest): Boolean {
openWindowRequestExecuted = true
return true
}
override fun onCloseWindowRequested(session: Session, windowRequest: WindowRequest): Boolean {
closeWindowRequestExecuted = true
return true
}
})
val createWindowRequest: WindowRequest = mock()
session.openWindowRequest = Consumable.from(createWindowRequest)
val closeWindowRequest: WindowRequest = mock()
session.closeWindowRequest = Consumable.from(closeWindowRequest)
assertTrue(openWindowRequestExecuted)
assertTrue(session.openWindowRequest.isConsumed())
assertTrue(closeWindowRequestExecuted)
assertTrue(session.closeWindowRequest.isConsumed())
}
@Test
fun `handle empty blocked trackers list race conditions`() {
val observer = mock(Session.Observer::class.java)
......
......@@ -420,18 +420,18 @@ class EngineObserverTest {
}
@Test
fun engineSessionObserverWithWindowRequests() {
fun engineObserverHandlesWindowRequest() {
val windowRequest = mock(WindowRequest::class.java)
val session = Session("")
val observer = EngineObserver(session)
assertTrue(session.openWindowRequest.isConsumed())
observer.onOpenWindowRequest(windowRequest)
assertFalse(session.openWindowRequest.isConsumed())
val store = mock(BrowserStore::class.java)
whenever(store.state).thenReturn(mock())
val observer = EngineObserver(session, store)
assertTrue(session.closeWindowRequest.isConsumed())
observer.onCloseWindowRequest(windowRequest)
assertFalse(session.closeWindowRequest.isConsumed())
observer.onWindowRequest(windowRequest)
verify(store).dispatch(ContentAction.UpdateWindowRequestAction(
session.id,
windowRequest
))
}
@Test
......
......@@ -21,6 +21,7 @@ import mozilla.components.concept.engine.EngineSessionState
import mozilla.components.concept.engine.HitResult
import mozilla.components.concept.engine.content.blocking.Tracker
import mozilla.components.concept.engine.prompt.PromptRequest
import mozilla.components.concept.engine.window.WindowRequest
import mozilla.components.lib.state.Action
typealias WebExtensionBrowserAction = mozilla.components.concept.engine.webextension.BrowserAction
......@@ -215,6 +216,16 @@ sealed class ContentAction : BrowserAction() {
* Removes all [FindResultState]s of the [ContentState] with the given [sessionId].
*/
data class ClearFindResultsAction(val sessionId: String) : ContentAction()
/**
* Updates the [WindowRequest] of the [ContentState] with the given [sessionId].
*/
data class UpdateWindowRequestAction(val sessionId: String, val windowRequest: WindowRequest) : ContentAction()
/**
* Removes the [WindowRequest] of the [ContentState] with the given [sessionId].
*/