[components] Closes mozilla-mobile/android-components#163: Remove duplicated state in SessionProvider/Storage

Author: csadilek

android-components/components/browser/session/src/main/java/mozilla/components/browser/session/Session.kt

@@ -4,13 +4,15 @@
 
 package mozilla.components.browser.session
 
+import java.util.UUID
 import kotlin.properties.Delegates
 
 /**
  * Value type that represents the state of a browser session. Changes can be observed.
  */
 class Session(
-    initialUrl: String
+    initialUrl: String,
+    val id: String = UUID.randomUUID().toString()
 ) {
     /**
      * Interface to be implemented by classes that want to observe a session.

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

@@ -26,8 +26,12 @@ class SessionManager(initialSession: Session) {
     /**
      * Adds the provided session.
      */
-    fun add(session: Session) {
+    fun add(session: Session, selected: Boolean = false) {
         sessions.add(session)
+
+        if (selected) {
+            select(session)
+        }
     }
 
     /**

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

@@ -15,10 +15,11 @@ class SessionManagerTest {
     @Test
     fun `session can be added`() {
         val manager = SessionManager(Session("http://www.mozilla.org"))
-        manager.add(Session("http://www.firefox.com"))
+        manager.add(Session("http://getpocket.com"))
+        manager.add(Session("http://www.firefox.com"), true)
 
-        assertEquals(2, manager.size)
-        assertEquals("http://www.mozilla.org", manager.selectedSession.url)
+        assertEquals(3, manager.size)
+        assertEquals("http://www.firefox.com", manager.selectedSession.url)
     }
 
     @Test

android-components/components/browser/session/src/test/java/mozilla/components/browser/session/SessionTest.kt

@@ -5,6 +5,7 @@
 package mozilla.components.browser.session
 
 import org.junit.Assert.assertEquals
+import org.junit.Assert.assertNotNull
 import org.junit.Test
 import org.mockito.Mockito.mock
 import org.mockito.Mockito.never
@@ -128,4 +129,14 @@ class SessionTest {
 
         assertEquals("https://www.mozilla.org", session.url)
     }
+
+    @Test
+    fun `Session always has an ID`() {
+        var session = Session("https://www.mozilla.org")
+        assertNotNull(session.id)
+
+        session = Session("https://www.mozilla.org", "s1")
+        assertNotNull(session.id)
+        assertEquals("s1", session.id)
+    }
 }

android-components/components/feature/session/src/main/java/mozilla/components/feature/session/DefaultSessionStorage.kt

@@ -8,9 +8,9 @@ import android.content.Context
 import mozilla.components.browser.session.Session
 import mozilla.components.concept.engine.Engine
 import mozilla.components.concept.engine.EngineSession
+import org.json.JSONException
 import org.json.JSONObject
 import java.io.FileNotFoundException
-import java.util.UUID
 
 const val SELECTED_SESSION_KEY = "selectedSession"
 const val SESSION_KEY = "session"
@@ -38,36 +38,11 @@ const val FILE_NAME = "mozilla_components_session_storage.json"
  * }
  */
 class DefaultSessionStorage(private val context: Context) : SessionStorage {
-    private val sessions = mutableMapOf<String, SessionProxy>()
-    private var selectedSession: Session? = null
 
     @Synchronized
-    override fun add(session: SessionProxy): String {
-        val id = UUID.randomUUID().toString()
-        sessions.put(id, session)
-        return id
-    }
-
-    @Synchronized
-    override fun remove(id: String): Boolean {
-        return sessions.remove(id) != null
-    }
-
-    @Synchronized
-    override fun get(id: String): SessionProxy? {
-        return sessions[id]
-    }
-
-    @Synchronized
-    override fun getSelected(): Session? {
-        return selectedSession
-    }
-
-    @Synchronized
-    override fun restore(engine: Engine): List<SessionProxy> {
+    override fun restore(engine: Engine): Pair<Map<Session, EngineSession>, String> {
+        val sessions = mutableMapOf<Session, EngineSession>()
         return try {
-            sessions.clear()
-
             context.openFileInput(FILE_NAME).use {
                 val json = it.bufferedReader().use {
                     it.readText()
@@ -84,44 +59,39 @@ class DefaultSessionStorage(private val context: Context) : SessionStorage {
                     val jsonSession = jsonRoot.getJSONObject(it)
                     val session = deserializeSession(jsonSession.getJSONObject(SESSION_KEY))
                     val engineSession = deserializeEngineSession(engine, jsonSession.getJSONObject(ENGINE_SESSION_KEY))
-                    sessions.put(it, SessionProxy(session, engineSession))
+                    sessions.put(session, engineSession)
                 }
-                selectedSession = sessions[selectedSessionId]?.session
+                Pair(sessions, selectedSessionId)
             }
-            sessions.values.toList()
         } catch (_: FileNotFoundException) {
-            emptyList()
+            Pair(emptyMap(), "")
+        } catch (_: JSONException) {
+            Pair(emptyMap(), "")
         }
     }
 
     @Synchronized
-    override fun persist(selectedSession: Session?): Boolean {
+    override fun persist(sessions: Map<Session, EngineSession>, selectedSession: String): Boolean {
         return try {
             val json = JSONObject()
             json.put(VERSION_KEY, VERSION)
+            json.put(SELECTED_SESSION_KEY, selectedSession)
 
-            var selectedSessionId = ""
-            for ((key, value) in sessions) {
-                if (value.session == selectedSession) {
-                    selectedSessionId = key
-                }
-            }
-            json.put(SELECTED_SESSION_KEY, selectedSessionId)
-
-            sessions.forEach({ (key, value) ->
+            sessions.forEach({ (session, engineSession) ->
                 val sessionJson = JSONObject()
-                sessionJson.put(SESSION_KEY, serializeSession(value.session))
-                sessionJson.put(ENGINE_SESSION_KEY, serializeEngineSession(value.engineSession))
-                json.put(key, sessionJson)
+                sessionJson.put(SESSION_KEY, serializeSession(session))
+                sessionJson.put(ENGINE_SESSION_KEY, serializeEngineSession(engineSession))
+                json.put(session.id, sessionJson)
             })
 
             context.openFileOutput(FILE_NAME, Context.MODE_PRIVATE).use {
-                println(json.toString())
                 it.write(json.toString().toByteArray())
             }
             true
         } catch (_: FileNotFoundException) {
             false
+        } catch (_: JSONException) {
+            false
         }
     }
 
@@ -136,10 +106,12 @@ class DefaultSessionStorage(private val context: Context) : SessionStorage {
     }
 
     private fun serializeEngineSession(engineSession: EngineSession): JSONObject {
+        // We don't currently persist any engine session state
         return JSONObject()
     }
 
     private fun deserializeEngineSession(engine: Engine, json: JSONObject): EngineSession {
+        // We don't currently persist any engine session state
         return engine.createSession()
     }
 }

android-components/components/feature/session/src/main/java/mozilla/components/feature/session/SessionProvider.kt

@@ -9,7 +9,6 @@ import mozilla.components.browser.session.Session
 import mozilla.components.browser.session.SessionManager
 import mozilla.components.concept.engine.Engine
 import mozilla.components.concept.engine.EngineSession
-import java.util.WeakHashMap
 import java.util.concurrent.Executors
 import java.util.concurrent.ScheduledExecutorService
 import java.util.concurrent.TimeUnit
@@ -20,45 +19,47 @@ import java.util.concurrent.TimeUnit
 class SessionProvider(
     private val context: Context,
     initialSession: Session = Session(""),
-    private val storage: SessionStorage = DefaultSessionStorage(context),
+    private val sessionStorage: SessionStorage = DefaultSessionStorage(context),
     private val savePeriodically: Boolean = false,
     private val saveIntervalInSeconds: Long = 300,
     private val scheduler: ScheduledExecutorService = Executors.newSingleThreadScheduledExecutor()
 ) {
-    private val sessions = WeakHashMap<Session, EngineSession>()
+    private val sessions = mutableMapOf<Session, EngineSession>()
+
     val sessionManager: SessionManager = SessionManager(initialSession)
 
     val selectedSession
         get() = sessionManager.selectedSession
 
     /**
-     * Starts this provider and schedules periodic saves.
+     * Restores persisted session state and schedules periodic saves.
      */
     fun start(engine: Engine) {
-        storage.restore(engine).forEach {
-            sessionManager.add(it.session)
-            sessions.put(it.session, it.engineSession)
+        val (restoredSessions, restoredSelectedSession) = sessionStorage.restore(engine)
+        sessions.putAll(restoredSessions)
+        sessions.keys.forEach {
+            sessionManager.add(it, it.id == restoredSelectedSession)
         }
-        storage.getSelected()?.let { sessionManager.select(it) }
 
         if (savePeriodically) {
             scheduler.scheduleAtFixedRate(
-                { storage.persist(sessionManager.selectedSession) },
+                { sessionStorage.persist(sessions, sessionManager.selectedSession?.id) },
                 saveIntervalInSeconds,
                 saveIntervalInSeconds,
                 TimeUnit.SECONDS)
         }
     }
 
     /**
-     * Returns the engine session corresponding to the current or given browser session. A new
-     * engine session will be created if none exists for the given browser session.
+     * Returns the engine session corresponding to the given (or currently selected)
+     * browser session. A new engine session will be created if none exists for the
+     * given browser session.
      */
     @Synchronized
     fun getOrCreateEngineSession(engine: Engine, session: Session = selectedSession): EngineSession {
         return sessions.getOrPut(session, {
             val engineSession = engine.createSession()
-            storage.add(SessionProxy(session, engineSession))
+            SessionProxy(session, engineSession)
 
             engineSession.loadUrl(session.url)
             engineSession
@@ -69,6 +70,7 @@ class SessionProvider(
      * Stops this provider and periodic saves.
      */
     fun stop() {
+        sessions.clear()
         scheduler.shutdown()
     }
 }

android-components/components/feature/session/src/main/java/mozilla/components/feature/session/SessionProxy.kt

@@ -8,9 +8,8 @@ import mozilla.components.concept.engine.EngineSession
 import mozilla.components.browser.session.Session
 
 /**
- * Represents a combination of a browser and engine session and acts as a proxy that
- * will subscribe to an EngineSession and update the Session object whenever new
- * data is available.
+ * Proxy class that will subscribe to an engine session and update the browser session object
+ * whenever new data is available.
  */
 class SessionProxy(
     val session: Session,

android-components/components/feature/session/src/main/java/mozilla/components/feature/session/SessionStorage.kt

@@ -6,59 +6,27 @@ package mozilla.components.feature.session
 
 import mozilla.components.browser.session.Session
 import mozilla.components.concept.engine.Engine
+import mozilla.components.concept.engine.EngineSession
 
 /**
  * Storage component for browser and engine sessions.
  */
 interface SessionStorage {
 
     /**
-     * Persists the current storage state (all active sessions).
+     * Persists the state of the provided sessions.
      *
-     * @param selectedSession the currently selected session, optional.
+     * @param sessions a map from browser session to engine session
+     * @param selectedSession an optional ID of the currently selected session.
      * @return true if the state was persisted, otherwise false.
      */
-    fun persist(selectedSession: Session? = null): Boolean
+    fun persist(sessions: Map<Session, EngineSession>, selectedSession: String = ""): Boolean
 
     /**
-     * Restores the storage state by reading from the latest persisted version.
+     * Restores the session storage state by reading from the latest persisted version.
      *
-     * @param engine the engine instance to use. Required to create new engine
-     * sessions.
-     * @return list of all restored sessions
+     * @param engine the engine instance to use when creating new engine sessions.
+     * @return map of all restored sessions, and the currently selected session id.
      */
-    fun restore(engine: Engine): List<SessionProxy>
-
-    /**
-     * Adds the provided session. The state change is not persisted until
-     * [persist] is called.
-     *
-     * @param session the session to add.
-     * @return unique ID of the session.
-     */
-    fun add(session: SessionProxy): String
-
-    /**
-     * Removes the session with the provided ID. The state change is not persisted until
-     * [persist] is called.
-     *
-     * @param id the ID of the session to remove.
-     * @return true if the session was found and removed, otherwise false.
-     */
-    fun remove(id: String): Boolean
-
-    /**
-     * Returns the session for the provided ID.
-     *
-     * @param id the ID of the session.
-     * @return the session if found, otherwise null.
-     */
-    fun get(id: String): SessionProxy?
-
-    /**
-     * Returns the selected session.
-     *
-     * @return the selected session, null if no session is selected.
-     */
-    fun getSelected(): Session?
+    fun restore(engine: Engine): Pair<Map<Session, EngineSession>, String>
 }