Merge #8250

8250: Close #7476: Persists lastAccess to SnapshotSerializer  r=csadilek a=jonalmeida
Co-authored-by: Jonathan Almeida <jalmeida@mozilla.com>

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

@@ -12,6 +12,7 @@ import mozilla.components.browser.state.action.EngineAction.LinkEngineSessionAct
 import mozilla.components.browser.state.action.EngineAction.UpdateEngineSessionStateAction
 import mozilla.components.browser.state.action.ReaderAction
 import mozilla.components.browser.state.action.TabListAction
+import mozilla.components.browser.state.action.LastAccessAction
 import mozilla.components.browser.state.selector.findTab
 import mozilla.components.browser.state.state.ReaderState
 import mozilla.components.browser.state.store.BrowserStore
@@ -225,6 +226,10 @@ class SessionManager(
                     store?.syncDispatch(ReaderAction.UpdateReaderActiveUrlAction(item.session.id, activeUrl))
                 }
             }
+
+            if (item.lastAccess != 0L) {
+                store?.syncDispatch(LastAccessAction.UpdateLastAccessAction(item.session.id, item.lastAccess))
+            }
         }
     }
 
@@ -302,7 +307,8 @@ class SessionManager(
             val session: Session,
             val engineSession: EngineSession? = null,
             val engineSessionState: EngineSessionState? = null,
-            val readerState: ReaderState? = null
+            val readerState: ReaderState? = null,
+            val lastAccess: Long = 0
         )
 
         companion object {

components/browser/session/src/main/java/mozilla/components/browser/session/engine/EngineMiddleware.kt

@@ -11,6 +11,7 @@ import mozilla.components.browser.session.Session
 import mozilla.components.browser.session.engine.middleware.CrashMiddleware
 import mozilla.components.browser.session.engine.middleware.CreateEngineSessionMiddleware
 import mozilla.components.browser.session.engine.middleware.EngineDelegateMiddleware
+import mozilla.components.browser.session.engine.middleware.LastAccessMiddleware
 import mozilla.components.browser.session.engine.middleware.LinkingMiddleware
 import mozilla.components.browser.session.engine.middleware.SuspendMiddleware
 import mozilla.components.browser.session.engine.middleware.TabsRemovedMiddleware
@@ -57,6 +58,7 @@ object EngineMiddleware {
             SuspendMiddleware(scope),
             WebExtensionMiddleware(),
             TrimMemoryMiddleware(),
+            LastAccessMiddleware(),
             CrashMiddleware(
                 engine,
                 sessionLookup,

components/browser/session/src/main/java/mozilla/components/browser/session/engine/middleware/LastAccessMiddleware.kt

+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+package mozilla.components.browser.session.engine.middleware
+
+import mozilla.components.browser.state.action.BrowserAction
+import mozilla.components.browser.state.action.TabListAction
+import mozilla.components.browser.state.action.LastAccessAction
+import mozilla.components.browser.state.state.BrowserState
+import mozilla.components.browser.state.state.TabSessionState
+import mozilla.components.lib.state.Middleware
+import mozilla.components.lib.state.MiddlewareContext
+
+/**
+ * [Middleware] that handles updating the [TabSessionState.lastAccess] when a tab is selected.
+ */
+internal class LastAccessMiddleware : Middleware<BrowserState, BrowserAction> {
+    override fun invoke(
+        context: MiddlewareContext<BrowserState, BrowserAction>,
+        next: (BrowserAction) -> Unit,
+        action: BrowserAction
+    ) {
+        next(action)
+
+        if (action is TabListAction.SelectTabAction) {
+            context.dispatchUpdateActionForId(action.tabId)
+        } else if (action is TabListAction.AddTabAction && action.select) {
+            context.dispatchUpdateActionForId(action.tab.id)
+        }
+    }
+
+    private fun MiddlewareContext<BrowserState, BrowserAction>.dispatchUpdateActionForId(id: String) {
+        dispatch(LastAccessAction.UpdateLastAccessAction(id, System.currentTimeMillis()))
+    }
+}

components/browser/session/src/main/java/mozilla/components/browser/session/storage/BrowserStateSerializer.kt

@@ -52,6 +52,8 @@ class BrowserStateSerializer {
         if (tab.readerState.active && tab.readerState.activeUrl != null) {
             sessionJson.put(Keys.SESSION_READER_MODE_ACTIVE_URL_KEY, tab.readerState.activeUrl)
         }
+        sessionJson.put(Keys.SESSION_LAST_ACCESS, tab.lastAccess)
+
         itemJson.put(Keys.SESSION_KEY, sessionJson)
 
         val engineSessionState = tab.engineState.engineSessionState

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

@@ -56,6 +56,7 @@ class SnapshotSerializer(
         if (item.readerState?.active == true && item.readerState.activeUrl != null) {
             sessionJson.put(Keys.SESSION_READER_MODE_ACTIVE_URL_KEY, item.readerState.activeUrl)
         }
+        sessionJson.put(Keys.SESSION_LAST_ACCESS, item.lastAccess)
         itemJson.put(Keys.SESSION_KEY, sessionJson)
 
         val engineSessionState = if (item.engineSessionState != null) {
@@ -105,13 +106,15 @@ class SnapshotSerializer(
             active = sessionJson.optBoolean(Keys.SESSION_READER_MODE_KEY, false),
             activeUrl = sessionJson.tryGetString(Keys.SESSION_READER_MODE_ACTIVE_URL_KEY)
         )
+        val lastAccess = sessionJson.optLong(Keys.SESSION_LAST_ACCESS, 0)
         val engineState = engine.createSessionState(engineSessionJson)
 
         return SessionManager.Snapshot.Item(
             session,
             engineSession = null,
             engineSessionState = engineState,
-            readerState = readerState
+            readerState = readerState,
+            lastAccess = lastAccess
         )
     }
 }
@@ -166,6 +169,7 @@ internal object Keys {
     const val SESSION_READER_MODE_KEY = "readerMode"
     const val SESSION_READER_MODE_ACTIVE_URL_KEY = "readerModeArticleUrl"
     const val SESSION_TITLE = "title"
+    const val SESSION_LAST_ACCESS = "lastAccess"
 
     const val SESSION_KEY = "session"
     const val ENGINE_SESSION_KEY = "engineSession"

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

@@ -4,10 +4,15 @@
 
 package mozilla.components.browser.session
 
+import mozilla.components.browser.state.action.BrowserAction
+import mozilla.components.browser.state.action.TabListAction
+import mozilla.components.browser.state.action.LastAccessAction
 import mozilla.components.browser.state.state.CustomTabConfig
+import mozilla.components.browser.state.store.BrowserStore
 import mozilla.components.concept.engine.EngineSession
 import mozilla.components.concept.engine.EngineSessionState
 import mozilla.components.support.test.any
+import mozilla.components.support.test.argumentCaptor
 import mozilla.components.support.test.mock
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertFalse
@@ -17,6 +22,8 @@ import org.junit.Test
 import org.mockito.ArgumentMatchers.anyString
 
 import org.mockito.Mockito.`when`
+import org.mockito.Mockito.calls
+import org.mockito.Mockito.inOrder
 import org.mockito.Mockito.mock
 import org.mockito.Mockito.never
 import org.mockito.Mockito.reset
@@ -1103,4 +1110,26 @@ class SessionManagerTest {
 
         verify(observer).onSessionsRestored()
     }
+
+    @Test
+    fun `WHEN restoring a session THEN dispatch updates to store`() {
+        val store = spy(BrowserStore())
+        val inOrder = inOrder(store)
+        val manager = SessionManager(mock(), store)
+        val session = Session("http://www.mozilla.org")
+        val captor = argumentCaptor<BrowserAction>()
+
+        manager.restore(SessionManager.Snapshot(listOf(
+            SessionManager.Snapshot.Item(
+                session,
+                lastAccess = 123
+            )
+        ), 0))
+
+        inOrder.verify(store, calls(2)).dispatch(captor.capture())
+
+        assertTrue(captor.allValues[0] is TabListAction.RestoreAction)
+        assertTrue(captor.allValues[1] is LastAccessAction.UpdateLastAccessAction)
+        assertEquals(123, store.state.tabs[0].lastAccess)
+    }
 }

components/browser/session/src/test/java/mozilla/components/browser/session/engine/middleware/LastAccessMiddlewareTest.kt

+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+package mozilla.components.browser.session.engine.middleware
+
+import mozilla.components.browser.state.action.BrowserAction
+import mozilla.components.browser.state.action.TabListAction
+import mozilla.components.browser.state.selector.selectedTab
+import mozilla.components.browser.state.state.BrowserState
+import mozilla.components.browser.state.state.createTab
+import mozilla.components.browser.state.store.BrowserStore
+import mozilla.components.lib.state.MiddlewareContext
+import mozilla.components.support.test.ext.joinBlocking
+import mozilla.components.support.test.mock
+import mozilla.components.support.test.whenever
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertNotEquals
+import org.junit.Assert.assertTrue
+import org.junit.Before
+import org.junit.Test
+
+class LastAccessMiddlewareTest {
+    lateinit var store: BrowserStore
+    lateinit var context: MiddlewareContext<BrowserState, BrowserAction>
+
+    @Before
+    fun setup() {
+        store = mock()
+        context = mock()
+
+        whenever(context.store).thenReturn(store)
+    }
+
+    @Test
+    fun `UpdateLastAction is dispatched when tab is selected`() {
+        val store = BrowserStore(
+            initialState = BrowserState(
+                listOf(
+                    createTab("https://mozilla.org", id = "123"),
+                    createTab("https://firefox.com", id = "456")
+                ),
+                selectedTabId = "123"
+            ),
+            middleware = listOf(LastAccessMiddleware())
+        )
+
+        assertEquals(0L, store.state.tabs[0].lastAccess)
+        assertEquals(0L, store.state.tabs[1].lastAccess)
+
+        store.dispatch(TabListAction.SelectTabAction("456")).joinBlocking()
+
+        assertEquals(0L, store.state.tabs[0].lastAccess)
+        assertNotEquals(0L, store.state.tabs[1].lastAccess)
+    }
+
+    @Test
+    fun `UpdateLastAction is dispatched when a new tab is selected`() {
+        val store = BrowserStore(
+            initialState = BrowserState(
+                listOf(
+                    createTab("https://mozilla.org", id = "123")
+                ),
+                selectedTabId = "123"
+            ),
+            middleware = listOf(LastAccessMiddleware())
+        )
+
+        assertEquals(0L, store.state.selectedTab?.lastAccess)
+
+        val newTab = createTab("https://firefox.com", id = "456")
+        store.dispatch(TabListAction.AddTabAction(newTab, select = true)).joinBlocking()
+
+        assertEquals("456", store.state.selectedTabId)
+        assertNotEquals(0L, store.state.selectedTab?.lastAccess)
+    }
+
+    @Test
+    fun `UpdateLastAction is not dispatched when a new tab is not selected`() {
+        val store = BrowserStore(
+            initialState = BrowserState(
+                listOf(
+                    createTab("https://mozilla.org", id = "123")
+                ),
+                selectedTabId = "123"
+            ),
+            middleware = listOf(LastAccessMiddleware())
+        )
+
+        assertEquals(0L, store.state.selectedTab?.lastAccess)
+
+        val newTab = createTab("https://firefox.com", id = "456")
+        store.dispatch(TabListAction.AddTabAction(newTab, select = false)).joinBlocking()
+
+        assertEquals("123", store.state.selectedTabId)
+        assertEquals(0L, store.state.selectedTab?.lastAccess)
+        assertEquals(0L, store.state.tabs[1].lastAccess)
+    }
+
+    @Test
+    fun `sanity check - next is always invoked in the middleware`() {
+        var nextInvoked = false
+        val middleware = LastAccessMiddleware()
+
+        middleware.invoke(context, { nextInvoked = true }, TabListAction.RemoveTabAction("123"))
+
+        assertTrue(nextInvoked)
+    }
+}

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

@@ -73,7 +73,8 @@ class SnapshotSerializerTest {
         val json = serializer.itemToJSON(
             Snapshot.Item(
                 originalSession,
-                engineSession = engineSession
+                engineSession = engineSession,
+                lastAccess = 1
             )
         )
         val restoredItem = serializer.itemFromJSON(engine, json)
@@ -83,6 +84,7 @@ class SnapshotSerializerTest {
         assertEquals("test-id", restoredItem.session.id)
         assertEquals("test-context-id", restoredItem.session.contextId)
         assertEquals("Hello World", restoredItem.session.title)
+        assertEquals(1, restoredItem.lastAccess)
         assertSame(restoredEngineSessionState, restoredItem.engineSessionState)
     }
 

components/browser/state/src/main/java/mozilla/components/browser/state/action/BrowserAction.kt

@@ -80,7 +80,7 @@ sealed class TabListAction : BrowserAction() {
      *
      * @property tabId the ID of the tab to select.
      */
-    data class SelectTabAction(val tabId: String, val timeSelected: Long = System.currentTimeMillis()) : TabListAction()
+    data class SelectTabAction(val tabId: String) : TabListAction()
 
     /**
      * Removes the [TabSessionState] with the given [tabId] from the list of sessions.
@@ -115,6 +115,22 @@ sealed class TabListAction : BrowserAction() {
     object RemoveAllNormalTabsAction : TabListAction()
 }
 
+/**
+ * [BrowserAction] implementations related to updating the [TabSessionState] inside [BrowserState].
+ */
+sealed class LastAccessAction : BrowserAction() {
+    /**
+     * Updates the timestamp of the [TabSessionState] with the given [tabId].
+     *
+     * @property tabId the ID of the tab to update.
+     * @property lastAccess the value to signify when the tab was last accessed; defaults to [System.currentTimeMillis].
+     */
+    data class UpdateLastAccessAction(
+        val tabId: String,
+        val lastAccess: Long = System.currentTimeMillis()
+    ) : LastAccessAction()
+}
+
 /**
  * [BrowserAction] implementations related to updating [BrowserState.customTabs].
  */

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

@@ -16,6 +16,7 @@ import mozilla.components.browser.state.action.ReaderAction
 import mozilla.components.browser.state.action.SearchAction
 import mozilla.components.browser.state.action.SystemAction
 import mozilla.components.browser.state.action.TabListAction
+import mozilla.components.browser.state.action.LastAccessAction
 import mozilla.components.browser.state.action.TrackingProtectionAction
 import mozilla.components.browser.state.action.WebExtensionAction
 import mozilla.components.browser.state.state.BrowserState
@@ -47,6 +48,7 @@ internal object BrowserStateReducer {
             is DownloadAction -> DownloadStateReducer.reduce(state, action)
             is SearchAction -> SearchReducer.reduce(state, action)
             is CrashAction -> CrashReducer.reduce(state, action)
+            is LastAccessAction -> LastAccessReducer.reduce(state, action)
         }
     }
 }

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

+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+package mozilla.components.browser.state.reducer
+
+import mozilla.components.browser.state.action.LastAccessAction
+import mozilla.components.browser.state.action.LastAccessAction.UpdateLastAccessAction
+import mozilla.components.browser.state.state.BrowserState
+import mozilla.components.browser.state.state.TabSessionState
+
+internal object LastAccessReducer {
+
+    /**
+     * [LastAccessAction] Reducer function for modifying [TabSessionState.lastAccess] state.
+     */
+    fun reduce(state: BrowserState, action: LastAccessAction): BrowserState = when (action) {
+        is UpdateLastAccessAction -> {
+            state.updateTabState(action.tabId) { sessionState ->
+                val tabSessionState = sessionState as TabSessionState
+                tabSessionState.copy(lastAccess = action.lastAccess)
+            }
+        }
+    }
+}

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

@@ -62,16 +62,7 @@ internal object TabListReducer {
             }
 
             is TabListAction.SelectTabAction -> {
-
-                val updatedTabsList = state.tabs.map {
-                    if (it.id == action.tabId) {
-                        it.copy(lastAccess = action.timeSelected)
-                    } else {
-                        it
-                    }
-                }
-
-                state.copy(tabs = updatedTabsList, selectedTabId = action.tabId)
+                state.copy(selectedTabId = action.tabId)
             }
 
             is TabListAction.RemoveTabAction -> {

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

+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+package mozilla.components.browser.state.action
+
+import mozilla.components.browser.state.selector.selectedTab
+import mozilla.components.browser.state.state.BrowserState
+import mozilla.components.browser.state.state.createTab
+import mozilla.components.browser.state.store.BrowserStore
+import mozilla.components.support.test.ext.joinBlocking
+import org.junit.Assert.assertEquals
+import org.junit.Test
+
+class LastAccessActionTest {
+    @Test
+    fun `UpdateLastAccessAction - updates the timestamp when the tab was last accessed`() {
+        val existingTab = createTab("https://www.mozilla.org")
+
+        val state = BrowserState(
+            tabs = listOf(existingTab),
+            selectedTabId = existingTab.id
+        )
+
+        val store = BrowserStore(state)
+        val timestamp = System.currentTimeMillis()
+
+        store.dispatch(LastAccessAction.UpdateLastAccessAction(existingTab.id, timestamp)).joinBlocking()
+
+        assertEquals(timestamp, store.state.selectedTab?.lastAccess)
+    }
+}
\ No newline at end of file

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

@@ -4,7 +4,6 @@
 
 package mozilla.components.browser.state.action
 
-import mozilla.components.browser.state.selector.findTab
 import mozilla.components.browser.state.selector.selectedTab
 import mozilla.components.browser.state.state.BrowserState
 import mozilla.components.browser.state.state.SessionState
@@ -147,15 +146,12 @@ class TabListActionTest {
             )
         )
         val store = BrowserStore(state)
-        val tabAccessTimeStamp = 123L
 
         assertNull(store.state.selectedTabId)
 
-        store.dispatch(TabListAction.SelectTabAction("a", tabAccessTimeStamp))
+        store.dispatch(TabListAction.SelectTabAction("a"))
             .joinBlocking()
 
-        // check if we update timestamp correctly upon selecting a tab
-        assertEquals(tabAccessTimeStamp, store.state.findTab("a")?.lastAccess)
         assertEquals("a", store.state.selectedTabId)
     }
 

components/browser/state/src/test/java/mozilla/components/browser/state/selector/SelectorsKtTest.kt

@@ -41,7 +41,7 @@ class SelectorsKtTest {
 
         assertEquals(tab, store.state.selectedTab)
 
-        store.dispatch(TabListAction.SelectTabAction(otherTab.id, tabLastAccessTimeStamp)).joinBlocking()
+        store.dispatch(TabListAction.SelectTabAction(otherTab.id)).joinBlocking()
 
         assertEquals(otherTab, store.state.selectedTab)
     }

components/feature/readerview/src/test/java/mozilla/components/feature/readerview/ReaderViewFeatureTest.kt

@@ -231,36 +231,31 @@ class ReaderViewFeatureTest {
         readerViewFeature.start()
         assertTrue(readerViewStatusChanges.isEmpty())
 
-        /* SelectTabAction triggers a ReaderViewStatusChange because we are updating lastAccess
-           timestamp of the selected tab
-         */
         store.dispatch(TabListAction.SelectTabAction(tab.id)).joinBlocking()
-        assertEquals(1, readerViewStatusChanges.size)
-
         store.dispatch(ReaderAction.UpdateReaderableAction(tab.id, true)).joinBlocking()
         testDispatcher.advanceUntilIdle()
-        assertEquals(2, readerViewStatusChanges.size)
-        assertEquals(Pair(true, false), readerViewStatusChanges[1])
+        assertEquals(1, readerViewStatusChanges.size)
+        assertEquals(Pair(true, false), readerViewStatusChanges[0])
 
         store.dispatch(ReaderAction.UpdateReaderActiveAction(tab.id, true)).joinBlocking()
         testDispatcher.advanceUntilIdle()
-        assertEquals(3, readerViewStatusChanges.size)
-        assertEquals(Pair(true, true), readerViewStatusChanges[2])
+        assertEquals(2, readerViewStatusChanges.size)
+        assertEquals(Pair(true, true), readerViewStatusChanges[1])
 
         store.dispatch(ReaderAction.UpdateReaderableAction(tab.id, true)).joinBlocking()
         testDispatcher.advanceUntilIdle()
         // No change -> No notification should have been sent
-        assertEquals(3, readerViewStatusChanges.size)
+        assertEquals(2, readerViewStatusChanges.size)
 
         store.dispatch(ReaderAction.UpdateReaderActiveAction(tab.id, false)).joinBlocking()
         testDispatcher.advanceUntilIdle()
-        assertEquals(4, readerViewStatusChanges.size)
-        assertEquals(Pair(true, false), readerViewStatusChanges[3])
+        assertEquals(3, readerViewStatusChanges.size)
+        assertEquals(Pair(true, false), readerViewStatusChanges[2])
 
         store.dispatch(ReaderAction.UpdateReaderableAction(tab.id, false)).joinBlocking()
         testDispatcher.advanceUntilIdle()
-        assertEquals(5, readerViewStatusChanges.size)
-        assertEquals(Pair(false, false), readerViewStatusChanges[4])
+        assertEquals(4, readerViewStatusChanges.size)
+        assertEquals(Pair(false, false), readerViewStatusChanges[3])
     }
 
     @Test

docs/changelog.md

@@ -28,8 +28,10 @@ permalink: /changelog/
   * Removed non-essential dependency on `com.google.firebase:firebase-core`.
 
 * **feature-toolbar**
-  * Added `ContainerToolbarFeature` to update the toolbar with the container page action whenever
-    the selected tab changes.
+  * Added `ContainerToolbarFeature` to update the toolbar with the container page action whenever the selected tab changes.
+
+* **browser-state**
+  * Added `LastAccessMiddleware` to dispatch `TabSessionAction.UpdateLastAccessAction` when a tab is selected.
 
 * **feature-prompts**
   * Replaced generic icon in `LoginDialogFragment` with site icon (keep the generic one as fallback)