Skip to content

Commit a97cc4b

Browse files
pocmopocmo
committed
Closes mozilla-mobile#3341: Allow restoring Tab/TabCollection without reusing session ids.
1 parent 081f3f7 commit a97cc4b

File tree

11 files changed

+162
-23
lines changed

11 files changed

+162
-23
lines changed

‎components/browser/session/src/main/java/mozilla/components/browser/session/ext/AtomicFile.kt

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,8 @@ fun AtomicFile.writeSnapshot(
4747
*/
4848
fun AtomicFile.readSnapshotItem(
4949
engine: Engine,
50-
serializer: SnapshotSerializer = SnapshotSerializer()
50+
restoreSessionId: Boolean,
51+
serializer: SnapshotSerializer = SnapshotSerializer(restoreSessionId)
5152
): SessionManager.Snapshot.Item? {
5253
return readAndDeserialize { json ->
5354
serializer.itemFromJSON(engine, JSONObject(json))

‎components/browser/session/src/main/java/mozilla/components/browser/session/storage/SnapshotSerializer.kt

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,14 +11,21 @@ import mozilla.components.concept.engine.Engine
1111
import org.json.JSONArray
1212
import org.json.JSONException
1313
import org.json.JSONObject
14+
import java.util.UUID
1415

1516
// Current version of the format used.
1617
private const val VERSION = 1
1718

1819
/**
1920
* Helper to transform [SessionManager.Snapshot] instances to JSON and back.
21+
*
22+
* @param restoreSessionIds If true the original [Session.id] of [Session]s will be restored. Otherwise a new ID will be
23+
* generated. An app may prefer to use new IDs if it expects sessions to get restored multiple times - otherwise
24+
* breaking the promise of a unique ID.
2025
*/
21-
class SnapshotSerializer {
26+
class SnapshotSerializer(
27+
private val restoreSessionIds: Boolean = true
28+
) {
2229
fun toJSON(snapshot: SessionManager.Snapshot): String {
2330
val json = JSONObject()
2431
json.put(Keys.VERSION_KEY, VERSION)
@@ -66,7 +73,7 @@ class SnapshotSerializer {
6673
}
6774

6875
fun itemFromJSON(engine: Engine, json: JSONObject): SessionManager.Snapshot.Item {
69-
val session = deserializeSession(json.getJSONObject(Keys.SESSION_KEY))
76+
val session = deserializeSession(json.getJSONObject(Keys.SESSION_KEY), restoreSessionIds)
7077
val state = engine.createSessionState(json.getJSONObject(Keys.ENGINE_SESSION_KEY))
7178

7279
return SessionManager.Snapshot.Item(session, engineSession = null, engineSessionState = state)
@@ -88,7 +95,7 @@ internal fun serializeSession(session: Session): JSONObject {
8895

8996
@Throws(JSONException::class)
9097
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
91-
internal fun deserializeSession(json: JSONObject): Session {
98+
internal fun deserializeSession(json: JSONObject, restoreId: Boolean): Session {
9299
val source = try {
93100
Session.Source.valueOf(json.getString(Keys.SESSION_SOURCE_KEY))
94101
} catch (e: IllegalArgumentException) {
@@ -99,7 +106,11 @@ internal fun deserializeSession(json: JSONObject): Session {
99106
// Currently, snapshot cannot contain private sessions.
100107
false,
101108
source,
102-
json.getString(Keys.SESSION_UUID_KEY)
109+
if (restoreId) {
110+
json.getString(Keys.SESSION_UUID_KEY)
111+
} else {
112+
UUID.randomUUID().toString()
113+
}
103114
)
104115
session.parentId = json.getString(Keys.SESSION_PARENT_UUID_KEY).takeIf { it != "" }
105116
session.title = if (json.has(Keys.SESSION_TITLE)) json.getString(Keys.SESSION_TITLE) else ""

‎components/browser/session/src/test/java/mozilla/components/browser/session/ext/AtomicFileKtTest.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,7 @@ class AtomicFileKtTest {
153153

154154
file.writeSnapshotItem(item)
155155

156-
val restoredItem = file.readSnapshotItem(engine)
156+
val restoredItem = file.readSnapshotItem(engine, restoreSessionId = true)
157157
assertNotNull(restoredItem!!)
158158
assertNotNull(restoredItem.engineSessionState!!)
159159

‎components/browser/session/src/test/java/mozilla/components/browser/session/storage/SessionStorageTest.kt

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -445,14 +445,14 @@ class SessionStorageTest {
445445
json.put("url", "testUrl")
446446
json.put("source", Source.NEW_TAB.name)
447447
json.put("parentUuid", "")
448-
assertEquals(Source.NEW_TAB, deserializeSession(json).source)
448+
assertEquals(Source.NEW_TAB, deserializeSession(json, restoreId = true).source)
449449

450450
val jsonInvalid = JSONObject()
451451
jsonInvalid.put("uuid", "testId")
452452
jsonInvalid.put("url", "testUrl")
453453
jsonInvalid.put("source", "invalidSource")
454454
jsonInvalid.put("parentUuid", "")
455-
assertEquals(Source.NONE, deserializeSession(jsonInvalid).source)
455+
assertEquals(Source.NONE, deserializeSession(jsonInvalid, restoreId = true).source)
456456
}
457457

458458
@Suppress("UNCHECKED_CAST")

‎components/browser/session/src/test/java/mozilla/components/browser/session/storage/SnapshotSerializerTest.kt

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import mozilla.components.browser.session.Session
99
import org.json.JSONObject
1010
import org.junit.Assert.assertEquals
1111
import org.junit.Assert.assertFalse
12+
import org.junit.Assert.assertNotEquals
1213
import org.junit.Assert.assertTrue
1314
import org.junit.Test
1415
import org.junit.runner.RunWith
@@ -27,7 +28,7 @@ class SnapshotSerializerTest {
2728
}
2829

2930
val json = serializeSession(originalSession)
30-
val restoredSession = deserializeSession(json)
31+
val restoredSession = deserializeSession(json, restoreId = true)
3132

3233
assertEquals("https://www.mozilla.org", restoredSession.url)
3334
assertEquals(Session.Source.ACTION_VIEW, restoredSession.source)
@@ -45,11 +46,37 @@ class SnapshotSerializerTest {
4546
put("parentUuid", "")
4647
}
4748

48-
val restoredSession = deserializeSession(json)
49+
val restoredSession = deserializeSession(json, restoreId = true)
4950

5051
assertEquals("https://www.mozilla.org", restoredSession.url)
5152
assertEquals(Session.Source.ACTION_VIEW, restoredSession.source)
5253
assertEquals("test-id", restoredSession.id)
5354
assertFalse(restoredSession.readerMode)
5455
}
56+
57+
@Test
58+
fun `Deserialize session without restoring id`() {
59+
val originalSession = Session(
60+
"https://www.mozilla.org",
61+
source = Session.Source.ACTION_VIEW,
62+
id = "test-id").apply {
63+
title = "Hello World"
64+
readerMode = true
65+
}
66+
67+
val json = serializeSession(originalSession)
68+
val restoredSession = deserializeSession(json, restoreId = false)
69+
70+
assertEquals("https://www.mozilla.org", restoredSession.url)
71+
assertEquals(Session.Source.ACTION_VIEW, restoredSession.source)
72+
assertNotEquals("test-id", restoredSession.id)
73+
assertTrue(restoredSession.id.isNotBlank())
74+
assertEquals("Hello World", restoredSession.title)
75+
assertTrue(restoredSession.readerMode)
76+
77+
val restoredSessionAgain = deserializeSession(json, restoreId = false)
78+
assertNotEquals("test-id", restoredSessionAgain.id)
79+
assertNotEquals(restoredSession.id, restoredSessionAgain.id)
80+
assertTrue(restoredSessionAgain.id.isNotBlank())
81+
}
5582
}

‎components/feature/tab-collections/src/androidTest/java/mozilla/components/feature/tab/collections/TabCollectionStorageTest.kt

Lines changed: 54 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import mozilla.components.browser.session.SessionManager
1515
import mozilla.components.concept.engine.DefaultSettings
1616
import mozilla.components.concept.engine.Engine
1717
import mozilla.components.concept.engine.EngineSession
18+
import mozilla.components.concept.engine.EngineSessionState
1819
import mozilla.components.concept.engine.EngineView
1920
import mozilla.components.concept.engine.Settings
2021
import mozilla.components.feature.tab.collections.db.TabCollectionDatabase
@@ -24,13 +25,15 @@ import mozilla.components.support.ktx.java.io.truncateDirectory
2425
import org.json.JSONObject
2526
import org.junit.After
2627
import org.junit.Assert.assertEquals
28+
import org.junit.Assert.assertNotEquals
2729
import org.junit.Assert.assertNotNull
2830
import org.junit.Before
2931
import org.junit.Rule
3032
import org.junit.Test
3133
import java.util.concurrent.ExecutorService
3234
import java.util.concurrent.Executors
3335

36+
@Suppress("LargeClass") // Large test is large
3437
class TabCollectionStorageTest {
3538
private lateinit var context: Context
3639
private lateinit var sessionManager: SessionManager
@@ -168,6 +171,52 @@ class TabCollectionStorageTest {
168171
}
169172
}
170173

174+
@Test
175+
fun testCreatingCollectionAndRestoringState() {
176+
val session1 = Session("https://www.mozilla.org").apply { title = "Mozilla" }
177+
val session2 = Session("https://www.firefox.com").apply { title = "Firefox" }
178+
179+
storage.createCollection("Articles", listOf(session1, session2))
180+
181+
getAllCollections().let { collections ->
182+
assertEquals(1, collections.size)
183+
184+
val collection = collections[0]
185+
186+
val snapshot = collection.restore(context, FakeEngine(), restoreSessionId = true)
187+
assertEquals(2, snapshot.sessions.size)
188+
189+
// We restored the same sessions
190+
assertEquals(session1, snapshot.sessions[0].session)
191+
assertEquals(session2, snapshot.sessions[1].session)
192+
193+
assertEquals(session1.id, snapshot.sessions[0].session.id)
194+
assertEquals(session2.id, snapshot.sessions[1].session.id)
195+
}
196+
197+
getAllCollections().let { collections ->
198+
assertEquals(1, collections.size)
199+
200+
val collection = collections[0]
201+
202+
val snapshot = collection.restore(context, FakeEngine(), restoreSessionId = false)
203+
assertEquals(2, snapshot.sessions.size)
204+
205+
// The sessions are not the same but contain the same data
206+
assertNotEquals(session1, snapshot.sessions[0].session)
207+
assertNotEquals(session2, snapshot.sessions[1].session)
208+
209+
assertNotEquals(session1.id, snapshot.sessions[0].session.id)
210+
assertNotEquals(session2.id, snapshot.sessions[1].session.id)
211+
212+
assertEquals(session1.url, snapshot.sessions[0].session.url)
213+
assertEquals(session2.url, snapshot.sessions[1].session.url)
214+
215+
assertEquals(session1.title, snapshot.sessions[0].session.title)
216+
assertEquals(session2.title, snapshot.sessions[1].session.title)
217+
}
218+
}
219+
171220
@Test
172221
@Suppress("ComplexMethod")
173222
fun testGettingCollectionsWithLimit() {
@@ -308,8 +357,7 @@ class FakeEngine : Engine {
308357
override fun createSession(private: Boolean): EngineSession =
309358
throw UnsupportedOperationException()
310359

311-
override fun createSessionState(json: JSONObject) =
312-
throw UnsupportedOperationException()
360+
override fun createSessionState(json: JSONObject) = FakeEngineSessionState()
313361

314362
override fun name(): String =
315363
throw UnsupportedOperationException()
@@ -319,3 +367,7 @@ class FakeEngine : Engine {
319367

320368
override val settings: Settings = DefaultSettings()
321369
}
370+
371+
class FakeEngineSessionState : EngineSessionState {
372+
override fun toJSON() = JSONObject()
373+
}

‎components/feature/tab-collections/src/main/java/mozilla/components/feature/tab/collections/Tab.kt

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ package mozilla.components.feature.tab.collections
66

77
import android.content.Context
88
import mozilla.components.browser.session.SessionManager
9+
import mozilla.components.browser.session.Session
910
import mozilla.components.concept.engine.Engine
1011

1112
/**
@@ -29,6 +30,15 @@ interface Tab {
2930

3031
/**
3132
* Restores a single tab from this collection and returns a matching [SessionManager.Snapshot].
33+
*
34+
* @param restoreSessionId If true the original [Session.id] of [Session]s will be restored. Otherwise a new ID
35+
* will be generated. An app may prefer to use a new ID if it expects sessions to get restored multiple times -
36+
* otherwise breaking the promise of a unique ID per [Session].
3237
*/
33-
fun restore(context: Context, engine: Engine, tab: Tab): SessionManager.Snapshot
38+
fun restore(
39+
context: Context,
40+
engine: Engine,
41+
tab: Tab,
42+
restoreSessionId: Boolean = false
43+
): SessionManager.Snapshot
3444
}

‎components/feature/tab-collections/src/main/java/mozilla/components/feature/tab/collections/TabCollection.kt

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
package mozilla.components.feature.tab.collections
66

77
import android.content.Context
8+
import mozilla.components.browser.session.Session
89
import mozilla.components.browser.session.SessionManager
910
import mozilla.components.concept.engine.Engine
1011

@@ -29,11 +30,28 @@ interface TabCollection {
2930

3031
/**
3132
* Restores all tabs in this collection and returns a matching [SessionManager.Snapshot].
33+
*
34+
* @param restoreSessionId If true the original [Session.id] of [Session]s will be restored. Otherwise a new ID
35+
* will be generated. An app may prefer to use a new ID if it expects sessions to get restored multiple times -
36+
* otherwise breaking the promise of a unique ID per [Session].
3237
*/
33-
fun restore(context: Context, engine: Engine): SessionManager.Snapshot
38+
fun restore(
39+
context: Context,
40+
engine: Engine,
41+
restoreSessionId: Boolean = false
42+
): SessionManager.Snapshot
3443

3544
/**
3645
* Restores a subset of the tabs in this collection and returns a matching [SessionManager.Snapshot].
46+
*
47+
* @param restoreSessionId If true the original [Session.id] of [Session]s will be restored. Otherwise a new ID
48+
* will be generated. An app may prefer to use a new ID if it expects sessions to get restored multiple times -
49+
* otherwise breaking the promise of a unique ID per [Session].
3750
*/
38-
fun restoreSubset(context: Context, engine: Engine, tabs: List<Tab>): SessionManager.Snapshot
51+
fun restoreSubset(
52+
context: Context,
53+
engine: Engine,
54+
tabs: List<Tab>,
55+
restoreSessionId: Boolean = false
56+
): SessionManager.Snapshot
3957
}

‎components/feature/tab-collections/src/main/java/mozilla/components/feature/tab/collections/adapter/TabAdapter.kt

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,13 @@ internal class TabAdapter(
2626
/**
2727
* Restores a single tab from this collection and returns a matching [SessionManager.Snapshot].
2828
*/
29-
override fun restore(context: Context, engine: Engine, tab: Tab): SessionManager.Snapshot {
30-
val item = entity.getStateFile(context.filesDir).readSnapshotItem(engine)
29+
override fun restore(
30+
context: Context,
31+
engine: Engine,
32+
tab: Tab,
33+
restoreSessionId: Boolean
34+
): SessionManager.Snapshot {
35+
val item = entity.getStateFile(context.filesDir).readSnapshotItem(engine, restoreSessionId)
3136
return SessionManager.Snapshot(if (item == null) emptyList() else listOf(item), SessionManager.NO_SELECTION)
3237
}
3338

‎components/feature/tab-collections/src/main/java/mozilla/components/feature/tab/collections/adapter/TabCollectionAdapter.kt

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -29,18 +29,28 @@ internal class TabCollectionAdapter(
2929
override val id: Long
3030
get() = entity.collection.id!!
3131

32-
override fun restore(context: Context, engine: Engine): SessionManager.Snapshot {
33-
return restore(context, engine, entity.tabs)
32+
override fun restore(context: Context, engine: Engine, restoreSessionId: Boolean): SessionManager.Snapshot {
33+
return restore(context, engine, entity.tabs, restoreSessionId)
3434
}
3535

36-
override fun restoreSubset(context: Context, engine: Engine, tabs: List<Tab>): SessionManager.Snapshot {
36+
override fun restoreSubset(
37+
context: Context,
38+
engine: Engine,
39+
tabs: List<Tab>,
40+
restoreSessionId: Boolean
41+
): SessionManager.Snapshot {
3742
val entities = entity.tabs.filter { candidate -> tabs.find { tab -> tab.id == candidate.id } != null }
38-
return restore(context, engine, entities)
43+
return restore(context, engine, entities, restoreSessionId)
3944
}
4045

41-
private fun restore(context: Context, engine: Engine, tabs: List<TabEntity>): SessionManager.Snapshot {
46+
private fun restore(
47+
context: Context,
48+
engine: Engine,
49+
tabs: List<TabEntity>,
50+
restoreSessionId: Boolean
51+
): SessionManager.Snapshot {
4252
val items = tabs.mapNotNull { tab ->
43-
tab.getStateFile(context.filesDir).readSnapshotItem(engine)
53+
tab.getStateFile(context.filesDir).readSnapshotItem(engine, restoreSessionId)
4454
}
4555
return SessionManager.Snapshot(items, SessionManager.NO_SELECTION)
4656
}

‎docs/changelog.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,11 @@ permalink: /changelog/
2020
* **browser-menu**
2121
* Added `endOfMenuAlwaysVisible` property/parameter to `BrowserMenuBuilder` constructor and to `BrowserMenu.show` function.
2222
When is set to true makes sure the bottom of the menu is always visible, this allows use cases like [#3211](https://github.com/mozilla-mobile/android-components/issues/3211).
23+
24+
* **feature-tab-collections**
25+
* Tabs can now be restored without restoring the ID of the `Session` by using the `restoreSessionId` flag. An app may
26+
prefer to use new IDs if it expects sessions to get restored multiple times - otherwise breaking the promise of a
27+
unique ID.
2328

2429
* **browser-search**
2530
* Added `getProvidedDefaultSearchEngine` to `SearchEngineManager` to return the provided default search engine or the first

0 commit comments

Comments
 (0)