Commit 7f55d968 authored by MozLando's avatar MozLando
Browse files

Merge #4773



4773: Closes #4749: Fix for bug 1522829 r=pocmo a=csadilek

We basically already have the parent session everywhere we need it, we now just forward the corresponding engine session so we can pass it along to GeckoView e.g. when opening a new (regular or private) tab, and when reacting to window requests.

@pocmo one discussion point is whether or not we have to create an engine session for the parent if it's no longer there (e.g. when re-loading a child tab after a restart). I decided **not** to do that as we'd be loading all parent sessions on startup and the child session should already have the right state.

Also, I didn't want to overload the method as named args with default values make this easy enough to read in Kotlin. 
Co-authored-by: default avatarChristian Sadilek <christian.sadilek@gmail.com>
parents 04ffc01c fc5954f5
......@@ -96,9 +96,9 @@ class GeckoEngineSession(
/**
* See [EngineSession.loadUrl]
*/
override fun loadUrl(url: String, flags: LoadUrlFlags) {
override fun loadUrl(url: String, parent: EngineSession?, flags: LoadUrlFlags) {
requestFromWebContent = false
geckoSession.loadUri(url, flags.value)
geckoSession.loadUri(url, (parent as? GeckoEngineSession)?.geckoSession, flags.value)
}
/**
......
......@@ -373,14 +373,21 @@ class GeckoEngineSessionTest {
@Test
fun loadUrl() {
val engineSession = GeckoEngineSession(mock(),
geckoSessionProvider = geckoSessionProvider)
val engineSession = GeckoEngineSession(mock(), geckoSessionProvider = geckoSessionProvider)
val parentEngineSession = GeckoEngineSession(mock(), geckoSessionProvider = geckoSessionProvider)
engineSession.loadUrl("http://mozilla.org")
verify(geckoSession).loadUri("http://mozilla.org", GeckoSession.LOAD_FLAGS_NONE)
verify(geckoSession).loadUri("http://mozilla.org", null as GeckoSession?, GeckoSession.LOAD_FLAGS_NONE)
engineSession.loadUrl("http://www.mozilla.org", LoadUrlFlags.select(LoadUrlFlags.EXTERNAL))
verify(geckoSession).loadUri("http://www.mozilla.org", GeckoSession.LOAD_FLAGS_EXTERNAL)
engineSession.loadUrl("http://www.mozilla.org", flags = LoadUrlFlags.select(LoadUrlFlags.EXTERNAL))
verify(geckoSession).loadUri("http://www.mozilla.org", null as GeckoSession?, GeckoSession.LOAD_FLAGS_EXTERNAL)
engineSession.loadUrl("http://www.mozilla.org", parent = parentEngineSession)
verify(geckoSession).loadUri(
"http://www.mozilla.org",
parentEngineSession.geckoSession,
GeckoSession.LOAD_FLAGS_NONE
)
}
@Test
......@@ -1187,7 +1194,7 @@ class GeckoEngineSessionTest {
navigationDelegate.value.onLoadRequest(geckoSession, mockLoadRequest("sample:about"))
assertEquals("sample:about", interceptorCalledWithUri)
verify(geckoSession).loadUri("https://mozilla.org", GeckoSession.LOAD_FLAGS_NONE)
verify(geckoSession).loadUri("https://mozilla.org", null as GeckoSession?, GeckoSession.LOAD_FLAGS_NONE)
}
@Test
......@@ -1901,7 +1908,7 @@ class GeckoEngineSessionTest {
// loadUrl(url: String)
engineSession.loadUrl(fakeUrl)
verify(geckoSession).loadUri(fakeUrl, GeckoSession.LOAD_FLAGS_NONE)
verify(geckoSession).loadUri(fakeUrl, null as GeckoSession?, GeckoSession.LOAD_FLAGS_NONE)
fakePageLoad(false)
// subsequent page loads _are_ from web content
......
......@@ -96,9 +96,9 @@ class GeckoEngineSession(
/**
* See [EngineSession.loadUrl]
*/
override fun loadUrl(url: String, flags: LoadUrlFlags) {
override fun loadUrl(url: String, parent: EngineSession?, flags: LoadUrlFlags) {
requestFromWebContent = false
geckoSession.loadUri(url, flags.value)
geckoSession.loadUri(url, (parent as? GeckoEngineSession)?.geckoSession, flags.value)
}
/**
......
......@@ -373,14 +373,21 @@ class GeckoEngineSessionTest {
@Test
fun loadUrl() {
val engineSession = GeckoEngineSession(mock(),
geckoSessionProvider = geckoSessionProvider)
val engineSession = GeckoEngineSession(mock(), geckoSessionProvider = geckoSessionProvider)
val parentEngineSession = GeckoEngineSession(mock(), geckoSessionProvider = geckoSessionProvider)
engineSession.loadUrl("http://mozilla.org")
verify(geckoSession).loadUri("http://mozilla.org", GeckoSession.LOAD_FLAGS_NONE)
verify(geckoSession).loadUri("http://mozilla.org", null as GeckoSession?, GeckoSession.LOAD_FLAGS_NONE)
engineSession.loadUrl("http://www.mozilla.org", LoadUrlFlags.select(LoadUrlFlags.EXTERNAL))
verify(geckoSession).loadUri("http://www.mozilla.org", GeckoSession.LOAD_FLAGS_EXTERNAL)
engineSession.loadUrl("http://www.mozilla.org", flags = LoadUrlFlags.select(LoadUrlFlags.EXTERNAL))
verify(geckoSession).loadUri("http://www.mozilla.org", null as GeckoSession?, GeckoSession.LOAD_FLAGS_EXTERNAL)
engineSession.loadUrl("http://www.mozilla.org", parent = parentEngineSession)
verify(geckoSession).loadUri(
"http://www.mozilla.org",
parentEngineSession.geckoSession,
GeckoSession.LOAD_FLAGS_NONE
)
}
@Test
......@@ -1187,7 +1194,7 @@ class GeckoEngineSessionTest {
navigationDelegate.value.onLoadRequest(geckoSession, mockLoadRequest("sample:about"))
assertEquals("sample:about", interceptorCalledWithUri)
verify(geckoSession).loadUri("https://mozilla.org", GeckoSession.LOAD_FLAGS_NONE)
verify(geckoSession).loadUri("https://mozilla.org", null as GeckoSession?, GeckoSession.LOAD_FLAGS_NONE)
}
@Test
......@@ -1899,7 +1906,7 @@ class GeckoEngineSessionTest {
// loadUrl(url: String)
engineSession.loadUrl(fakeUrl)
verify(geckoSession).loadUri(fakeUrl, GeckoSession.LOAD_FLAGS_NONE)
verify(geckoSession).loadUri(fakeUrl, null as GeckoSession?, GeckoSession.LOAD_FLAGS_NONE)
fakePageLoad(false)
// subsequent page loads _are_ from web content
......
......@@ -93,8 +93,9 @@ class GeckoEngineSession(
/**
* See [EngineSession.loadUrl]
*/
override fun loadUrl(url: String, flags: LoadUrlFlags) {
override fun loadUrl(url: String, parent: EngineSession?, flags: LoadUrlFlags) {
requestFromWebContent = false
// parent / referring session not supported yet in release
geckoSession.loadUri(url, flags.value)
}
......
......@@ -329,14 +329,18 @@ class GeckoEngineSessionTest {
@Test
fun loadUrl() {
val engineSession = GeckoEngineSession(mock(),
geckoSessionProvider = geckoSessionProvider)
val engineSession = GeckoEngineSession(mock(), geckoSessionProvider = geckoSessionProvider)
val parentEngineSession = GeckoEngineSession(mock(), geckoSessionProvider = geckoSessionProvider)
engineSession.loadUrl("http://mozilla.org")
verify(geckoSession).loadUri("http://mozilla.org", GeckoSession.LOAD_FLAGS_NONE)
engineSession.loadUrl("http://www.mozilla.org", LoadUrlFlags.select(LoadUrlFlags.EXTERNAL))
engineSession.loadUrl("http://www.mozilla.org", flags = LoadUrlFlags.select(LoadUrlFlags.EXTERNAL))
verify(geckoSession).loadUri("http://www.mozilla.org", GeckoSession.LOAD_FLAGS_EXTERNAL)
engineSession.loadUrl("http://www.mozilla.org", parent = parentEngineSession)
// parent / referring session not supported yet in release
verify(geckoSession).loadUri("http://www.mozilla.org", GeckoSession.LOAD_FLAGS_NONE)
}
@Test
......
......@@ -68,7 +68,7 @@ class SystemEngineSession(
* See [EngineSession.loadUrl]. Note that [LoadUrlFlags] are ignored in this engine
* implementation.
*/
override fun loadUrl(url: String, flags: LoadUrlFlags) {
override fun loadUrl(url: String, parent: EngineSession?, flags: LoadUrlFlags) {
if (!url.isEmpty()) {
currentUrl = url
webView.loadUrl(url, additionalHeaders)
......
......@@ -290,7 +290,10 @@ class LegacySessionManager(
}
fun link(session: Session, engineSession: EngineSession) {
engineSessionLinker.link(session, engineSession)
val parent = values.find { it.id == session.parentId }?.let {
this.getEngineSession(it)
}
engineSessionLinker.link(session, engineSession, parent)
}
private fun unlink(session: Session) {
......
......@@ -37,14 +37,17 @@ class SessionManager(
* actions when an engine session is linked and unlinked.
*/
class EngineSessionLinker(private val store: BrowserStore?) {
fun link(session: Session, engineSession: EngineSession) {
/**
* Links the provided [Session] and [EngineSession].
*/
fun link(session: Session, engineSession: EngineSession, parentEngineSession: EngineSession?) {
unlink(session)
session.engineSessionHolder.apply {
this.engineSession = engineSession
this.engineObserver = EngineObserver(session, store).also { observer ->
engineSession.register(observer)
engineSession.loadUrl(session.url)
engineSession.loadUrl(session.url, parentEngineSession)
}
}
......
......@@ -601,11 +601,38 @@ class SessionManagerTest {
assertEquals(actualEngineSession, sessionManager.getOrCreateEngineSession(session))
assertEquals(actualEngineSession, sessionManager.getEngineSession(session))
assertEquals(actualEngineSession, sessionManager.getOrCreateEngineSession(session))
assertEquals(actualEngineSession, session.engineSessionHolder.engineSession)
val privateSession = Session("https://www.mozilla.org", true, Session.Source.NONE)
sessionManager.add(privateSession)
assertNull(sessionManager.getEngineSession(privateSession))
assertEquals(privateEngineSession, sessionManager.getOrCreateEngineSession(privateSession))
assertEquals(privateEngineSession, privateSession.engineSessionHolder.engineSession)
}
@Test
fun `session manager considers parent when creating and linking engine session`() {
val engine: Engine = mock()
val parent = Session(id = "parent", initialUrl = "")
val parentEngineSession: EngineSession = mock()
val session = Session("https://www.mozilla.org")
session.parentId = parent.id
val engineSession: EngineSession = mock()
val sessionManager = SessionManager(engine)
sessionManager.add(parent)
sessionManager.add(session)
doReturn(parentEngineSession, engineSession).`when`(engine).createSession(false)
assertEquals(parentEngineSession, sessionManager.getOrCreateEngineSession(parent))
assertEquals(parentEngineSession, parent.engineSessionHolder.engineSession)
assertEquals(engineSession, sessionManager.getOrCreateEngineSession(session))
assertEquals(engineSession, session.engineSessionHolder.engineSession)
verify(engineSession).loadUrl(session.url, parentEngineSession, EngineSession.LoadUrlFlags.none())
}
@Test
......
......@@ -71,7 +71,7 @@ class EngineObserverTest {
notifyObservers { onLoadingStateChange(true) }
notifyObservers { onNavigationStateChange(true, true) }
}
override fun loadUrl(url: String, flags: LoadUrlFlags) {
override fun loadUrl(url: String, parent: EngineSession?, flags: LoadUrlFlags) {
notifyObservers { onLocationChange(url) }
notifyObservers { onProgress(100) }
notifyObservers { onLoadingStateChange(true) }
......@@ -113,7 +113,7 @@ class EngineObserverTest {
override fun saveState(): EngineSessionState = mock()
override fun loadData(data: String, mimeType: String, encoding: String) {}
override fun recoverFromCrash(): Boolean { return false }
override fun loadUrl(url: String, flags: LoadUrlFlags) {
override fun loadUrl(url: String, parent: EngineSession?, flags: LoadUrlFlags) {
if (url.startsWith("https://")) {
notifyObservers { onSecurityChange(true, "host", "issuer") }
} else {
......@@ -151,7 +151,7 @@ class EngineObserverTest {
override fun toggleDesktopMode(enable: Boolean, reload: Boolean) {}
override fun saveState(): EngineSessionState = mock()
override fun loadUrl(url: String, flags: LoadUrlFlags) {}
override fun loadUrl(url: String, parent: EngineSession?, flags: LoadUrlFlags) {}
override fun loadData(data: String, mimeType: String, encoding: String) {}
override fun findAll(text: String) {}
override fun findNext(forward: Boolean) {}
......
......@@ -370,9 +370,11 @@ abstract class EngineSession(
* Loads the given URL.
*
* @param url the url to load.
* @param parent the parent (referring) [EngineSession] i.e. the session that
* triggered creating this one.
* @param flags the [LoadUrlFlags] to use when loading the provider url.
*/
abstract fun loadUrl(url: String, flags: LoadUrlFlags = LoadUrlFlags.none())
abstract fun loadUrl(url: String, parent: EngineSession? = null, flags: LoadUrlFlags = LoadUrlFlags.none())
/**
* Loads the data with the given mimeType.
......
......@@ -720,7 +720,7 @@ open class DummyEngineSession : EngineSession() {
override fun saveState(): EngineSessionState { return mock() }
override fun loadUrl(url: String, flags: LoadUrlFlags) {}
override fun loadUrl(url: String, parent: EngineSession?, flags: LoadUrlFlags) {}
override fun loadData(data: String, mimeType: String, encoding: String) {}
......
......@@ -64,7 +64,7 @@ class CustomTabIntentProcessorTest {
handler.process(intent)
verify(sessionManager).add(anySession(), eq(false), eq(null), eq(null))
verify(engineSession).loadUrl("http://mozilla.org", LoadUrlFlags.external())
verify(engineSession).loadUrl("http://mozilla.org", flags = LoadUrlFlags.external())
verify(intent).putExtra(eq(EXTRA_SESSION_ID), any<String>())
val customTabSession = sessionManager.all[0]
......@@ -93,7 +93,7 @@ class CustomTabIntentProcessorTest {
handler.process(intent)
verify(sessionManager).add(anySession(), eq(false), eq(null), eq(null))
verify(engineSession).loadUrl("http://mozilla.org", LoadUrlFlags.external())
verify(engineSession).loadUrl("http://mozilla.org", flags = LoadUrlFlags.external())
verify(intent).putExtra(eq(EXTRA_SESSION_ID), any<String>())
val customTabSession = sessionManager.all[0]
......
......@@ -70,7 +70,7 @@ class TabIntentProcessorTest {
whenever(intent.dataString).thenReturn("http://mozilla.org")
handler.process(intent)
verify(engineSession).loadUrl("http://mozilla.org", LoadUrlFlags.external())
verify(engineSession).loadUrl("http://mozilla.org", flags = LoadUrlFlags.external())
val session = sessionManager.all[0]
assertNotNull(session)
......@@ -91,7 +91,7 @@ class TabIntentProcessorTest {
whenever(intent.dataString).thenReturn("http://mozilla.org")
handler.process(intent)
verify(engineSession).loadUrl("http://mozilla.org", LoadUrlFlags.external())
verify(engineSession).loadUrl("http://mozilla.org", flags = LoadUrlFlags.external())
}
@Test
......@@ -110,7 +110,7 @@ class TabIntentProcessorTest {
whenever(intent.dataString).thenReturn("http://mozilla.org")
handler.process(intent)
verify(engineSession).loadUrl("http://mozilla.org", LoadUrlFlags.external())
verify(engineSession).loadUrl("http://mozilla.org", flags = LoadUrlFlags.external())
}
@Test
......@@ -132,7 +132,7 @@ class TabIntentProcessorTest {
whenever(intent.dataString).thenReturn("http://mozilla.org")
handler.process(intent)
verify(engineSession).loadUrl("http://mozilla.org", LoadUrlFlags.external())
verify(engineSession).loadUrl("http://mozilla.org", flags = LoadUrlFlags.external())
val session = sessionManager.all[0]
assertNotNull(session)
......@@ -153,7 +153,7 @@ class TabIntentProcessorTest {
whenever(intent.dataString).thenReturn("http://mozilla.org")
handler.process(intent)
verify(engineSession).loadUrl("http://mozilla.org", LoadUrlFlags.external())
verify(engineSession).loadUrl("http://mozilla.org", flags = LoadUrlFlags.external())
}
@Test
......@@ -172,7 +172,7 @@ class TabIntentProcessorTest {
whenever(intent.dataString).thenReturn("http://mozilla.org")
handler.process(intent)
verify(engineSession).loadUrl("http://mozilla.org", LoadUrlFlags.external())
verify(engineSession).loadUrl("http://mozilla.org", flags = LoadUrlFlags.external())
}
@Test
......@@ -186,23 +186,23 @@ class TabIntentProcessorTest {
whenever(intent.getStringExtra(Intent.EXTRA_TEXT)).thenReturn("http://mozilla.org")
handler.process(intent)
verify(engineSession).loadUrl("http://mozilla.org", LoadUrlFlags.external())
verify(engineSession).loadUrl("http://mozilla.org", flags = LoadUrlFlags.external())
whenever(intent.getStringExtra(Intent.EXTRA_TEXT)).thenReturn("see http://getpocket.com")
handler.process(intent)
verify(engineSession).loadUrl("http://getpocket.com", LoadUrlFlags.external())
verify(engineSession).loadUrl("http://getpocket.com", flags = LoadUrlFlags.external())
whenever(intent.getStringExtra(Intent.EXTRA_TEXT)).thenReturn("see http://mozilla.com and http://getpocket.com")
handler.process(intent)
verify(engineSession).loadUrl("http://mozilla.com", LoadUrlFlags.external())
verify(engineSession).loadUrl("http://mozilla.com", flags = LoadUrlFlags.external())
whenever(intent.getStringExtra(Intent.EXTRA_TEXT)).thenReturn("checkout the Tweet: http://tweets.mozilla.com")
handler.process(intent)
verify(engineSession).loadUrl("http://tweets.mozilla.com", LoadUrlFlags.external())
verify(engineSession).loadUrl("http://tweets.mozilla.com", flags = LoadUrlFlags.external())
whenever(intent.getStringExtra(Intent.EXTRA_TEXT)).thenReturn("checkout the Tweet: HTTP://tweets.mozilla.com")
handler.process(intent)
verify(engineSession).loadUrl("http://tweets.mozilla.com", LoadUrlFlags.external())
verify(engineSession).loadUrl("http://tweets.mozilla.com", flags = LoadUrlFlags.external())
}
@Test
......
......@@ -63,7 +63,7 @@ class SessionUseCases(
flags: LoadUrlFlags = LoadUrlFlags.none()
) {
val loadSession = session ?: onNoSession.invoke(url)
sessionManager.getOrCreateEngineSession(loadSession).loadUrl(url, flags)
sessionManager.getOrCreateEngineSession(loadSession).loadUrl(url, flags = flags)
}
}
......
......@@ -43,7 +43,7 @@ class SessionUseCasesTest {
verify(selectedEngineSession).loadUrl("http://mozilla.org")
useCases.loadUrl("http://www.mozilla.org", LoadUrlFlags.select(LoadUrlFlags.EXTERNAL))
verify(selectedEngineSession).loadUrl("http://www.mozilla.org", LoadUrlFlags.select(LoadUrlFlags.EXTERNAL))
verify(selectedEngineSession).loadUrl("http://www.mozilla.org", flags = LoadUrlFlags.select(LoadUrlFlags.EXTERNAL))
useCases.loadUrl("http://getpocket.com", selectedSession)
verify(selectedEngineSession).loadUrl("http://getpocket.com")
......@@ -51,7 +51,7 @@ class SessionUseCasesTest {
useCases.loadUrl.invoke("http://www.getpocket.com", selectedSession,
LoadUrlFlags.select(LoadUrlFlags.BYPASS_PROXY))
verify(selectedEngineSession).loadUrl("http://www.getpocket.com",
LoadUrlFlags.select(LoadUrlFlags.BYPASS_PROXY))
flags = LoadUrlFlags.select(LoadUrlFlags.BYPASS_PROXY))
}
@Test
......
......@@ -84,7 +84,8 @@ class TabsUseCases(
sessionManager.add(session, selected = selectTab, parent = parent)
if (startLoading) {
sessionManager.getOrCreateEngineSession(session).loadUrl(url, flags)
val parentEngineSession = parent?.let { sessionManager.getEngineSession(it) }
sessionManager.getOrCreateEngineSession(session).loadUrl(url, parentEngineSession, flags)
}
return session
......@@ -126,7 +127,8 @@ class TabsUseCases(
sessionManager.add(session, selected = selectTab, parent = parent)
if (startLoading) {
sessionManager.getOrCreateEngineSession(session).loadUrl(url, flags)
val parentEngineSession = parent?.let { sessionManager.getEngineSession(it) }
sessionManager.getOrCreateEngineSession(session).loadUrl(url, parentEngineSession, flags)
}
return session
......
......@@ -109,7 +109,23 @@ class TabsUseCasesTest {
val useCases = TabsUseCases(sessionManager)
useCases.addTab.invoke("https://www.mozilla.org", LoadUrlFlags.select(LoadUrlFlags.EXTERNAL))
verify(engineSession).loadUrl("https://www.mozilla.org", LoadUrlFlags.select(LoadUrlFlags.EXTERNAL))
verify(engineSession).loadUrl("https://www.mozilla.org", flags = LoadUrlFlags.select(LoadUrlFlags.EXTERNAL))
}
@Test
fun `AddNewTabUseCase forwards parent session to engine`() {
val sessionManager = spy(SessionManager(mock()))
val engineSession: EngineSession = mock()
doReturn(engineSession).`when`(sessionManager).getOrCreateEngineSession(any())
val parentSession = Session(id = "parent", initialUrl = "")
sessionManager.add(parentSession)
val parentEngineSession: EngineSession = mock()
doReturn(parentEngineSession).`when`(sessionManager).getEngineSession(parentSession)
val useCases = TabsUseCases(sessionManager)
useCases.addTab.invoke("https://www.mozilla.org", parentId = "parent", startLoading = true)
verify(engineSession).loadUrl("https://www.mozilla.org", parentEngineSession, LoadUrlFlags.none())
}
@Test
......@@ -134,7 +150,23 @@ class TabsUseCasesTest {
val useCases = TabsUseCases(sessionManager)
useCases.addPrivateTab.invoke("https://www.mozilla.org", LoadUrlFlags.select(LoadUrlFlags.EXTERNAL))
verify(engineSession).loadUrl("https://www.mozilla.org", LoadUrlFlags.select(LoadUrlFlags.EXTERNAL))
verify(engineSession).loadUrl("https://www.mozilla.org", flags = LoadUrlFlags.select(LoadUrlFlags.EXTERNAL))
}
@Test
fun `AddNewPrivateTabUseCase forwards parent session to engine`() {
val sessionManager = spy(SessionManager(mock()))
val engineSession: EngineSession = mock()
doReturn(engineSession).`when`(sessionManager).getOrCreateEngineSession(any())
val parentSession = Session(id = "parent", initialUrl = "")
sessionManager.add(parentSession)
val parentEngineSession: EngineSession = mock()
doReturn(parentEngineSession).`when`(sessionManager).getEngineSession(parentSession)
val useCases = TabsUseCases(sessionManager)
useCases.addPrivateTab.invoke("https://www.mozilla.org", parentId = "parent", startLoading = true)
verify(engineSession).loadUrl("https://www.mozilla.org", parentEngineSession, LoadUrlFlags.none())
}
@Test
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment