Issue #3557, #3432: Address review comments.

components/browser/session/src/main/java/mozilla/components/browser/session/SessionManager.kt

@@ -4,6 +4,7 @@
 
 package mozilla.components.browser.session
 
+import androidx.lifecycle.Transformations.map
 import mozilla.components.browser.session.ext.syncDispatch
 import mozilla.components.browser.session.ext.toCustomTabSessionState
 import mozilla.components.browser.session.ext.toTabSessionState
@@ -125,15 +126,15 @@ class SessionManager(
     fun restore(snapshot: Snapshot, updateSelection: Boolean = true) {
         delegate.restore(snapshot, updateSelection)
 
-        val tabs = snapshot.sessions.mapNotNull { item ->
-            if (!item.session.isCustomTabSession()) {
-                item.session.toTabSessionState()
-            } else {
+        val tabs = snapshot.sessions
+            .filter {
                 // A restored snapshot should not contain any custom tab so we should be able to safely ignore
                 // them here.
-                null
+                !it.session.isCustomTabSession()
+            }
+            .map { item ->
+                item.session.toTabSessionState()
             }
-        }
 
         val selectedTabId = if (updateSelection && snapshot.selectedSessionIndex != NO_SELECTION) {
             val index = snapshot.selectedSessionIndex

components/browser/session/src/test/java/mozilla/components/browser/session/SessionManagerMigrationTest.kt

@@ -203,7 +203,7 @@ class SessionManagerMigrationTest {
 
     @Test
     fun `Restoring snapshot with invalid index`() {
-        val store = BrowserStore(BrowserState())
+        val store = BrowserStore()
 
         val manager = SessionManager(engine = mock(), store = store)
         manager.restore(SessionManager.Snapshot(listOf(), selectedSessionIndex = 0))
@@ -219,7 +219,7 @@ class SessionManagerMigrationTest {
     fun `Restoring snapshot without updating selection`() {
         val session: Session
 
-        val store = BrowserStore(BrowserState())
+        val store = BrowserStore()
 
         val manager = SessionManager(engine = mock(), store = store).apply {
             session = Session("https://getpocket.com")
@@ -269,7 +269,7 @@ class SessionManagerMigrationTest {
             selectedSessionIndex = 2
         )
 
-        val store = BrowserStore(BrowserState())
+        val store = BrowserStore()
         val manager = SessionManager(engine = mock(), store = store)
 
         manager.restore(snapshot)

components/browser/session/src/test/java/mozilla/components/browser/session/SessionManagerTest.kt

@@ -374,6 +374,33 @@ class SessionManagerTest {
         verify(observer).onSessionsRestored()
     }
 
+    @Test
+    fun `Restore sessions with already existing session`() {
+        val manager = SessionManager(engine = mock())
+        manager.add(Session("https://www.mozilla.org"))
+
+        assertEquals("https://www.mozilla.org", manager.selectedSessionOrThrow.url)
+
+        val snapshot = SessionManager.Snapshot(
+            listOf(
+                SessionManager.Snapshot.Item(session = Session("https://www.firefox.com")),
+                SessionManager.Snapshot.Item(session = Session("https://www.wikipedia.org")),
+                SessionManager.Snapshot.Item(session = Session("https://getpocket.com"))
+            ),
+            selectedSessionIndex = 1
+        )
+
+        manager.restore(snapshot, updateSelection = false)
+
+        assertEquals(4, manager.size)
+
+        assertEquals("https://www.mozilla.org", manager.selectedSessionOrThrow.url)
+        assertEquals("https://www.firefox.com", manager.sessions[0].url)
+        assertEquals("https://www.wikipedia.org", manager.sessions[1].url)
+        assertEquals("https://getpocket.com", manager.sessions[2].url)
+        assertEquals("https://www.mozilla.org", manager.sessions[3].url)
+    }
+
     @Test
     fun `restore may be used to bulk-add session from a SessionsSnapshot`() {
         val manager = SessionManager(mock())

components/browser/state/src/main/java/mozilla/components/browser/state/reducer/TabListReducer.kt

@@ -54,7 +54,7 @@ internal object TabListReducer {
 
             is TabListAction.RestoreAction -> {
                 // We are adding the restored tabs first and then the already existing tabs. Since restore can or should
-                // happen asynchronously we may already have a tab at this point (e.g. from an `Intent` and so we
+                // happen asynchronously we may already have a tab at this point (e.g. from an `Intent`) and so we
                 // pretend we restored the list of tabs before any tab was added.
                 state.copy(
                     tabs = action.tabs + state.tabs,

components/browser/state/src/test/java/mozilla/components/browser/state/action/TabListActionTest.kt

@@ -261,7 +261,7 @@ class TabListActionTest {
 
     @Test
     fun `RestoreAction - Adds restored tabs and updates selected tab`() {
-        val store = BrowserStore(BrowserState())
+        val store = BrowserStore()
 
         assertEquals(0, store.state.tabs.size)