Closes #1126: Rust Places backed implementation of concept-storage #1164

grigoryk · 2018-10-25T00:59:45Z

A quick pass to see how things might fit together. It works!

grigoryk · 2018-10-25T01:29:48Z

mozilla/application-services#327 adds an API we'd need here for the system engine, and fixes a bug around titles not being recorded.

grigoryk · 2018-10-26T02:24:34Z

Changes required from app-services are done, and scaffolding for this is coming together in #1155.

pocmo · 2018-10-26T08:16:19Z

samples/browser/src/geckoBeta/java/org/mozilla/samples/browser/Components.kt

+//            historyTrackingDelegate = PlacesHistoryTrackingDelegate(applicationContext)
+//        }
+//        GeckoEngine(applicationContext, defaultSettings)
+//    }


FYI: In the bottom left corner in Android Studio there should be a "build variant" side bar. There you should be able to switch the build variant of samples-browser. In this case you can just use systemUniversalDebug.

grigoryk · 2018-10-31T05:55:00Z

@pocmo @csadilek this is ready for a review. This works for me locally with a HistoryTrackingFeature enabled in the sample browser with a SystemEngine, and will track history with GeckoView once #1238 lands.

To get tests to work for PlacesHistoryStorage - or anything that uses PlacesConnection - we'd probably first need to change how it loads the native library, e.g. wrap it in a try/catch to let tests work and mock it, like how we do in the FxaClient. Otherwise, as long as PlacesConnection is initialized at all during a test, it fails since it can't load the library.

Alternatively, we could supply library artifacts for the test machine architectures, and wire that up...? Not entirely sure how that would look.

grigoryk · 2018-10-31T06:00:04Z

Relevant app services issue: mozilla/application-services#342

grigoryk · 2018-10-31T06:53:11Z

Pushed out an app-services PR to help out with tests and docs. mozilla/application-services#343

grigoryk · 2018-10-31T21:14:23Z

Still wrapping my head around coroutines, dispatchers and whatnot. Regarding the Deferred change, it complicates the implementations a little bit. The onus is on the implementations to schedule jobs. Places layer doesn't inherit a CoroutineScope (and the dispatcher) of the caller, and instead uses Dispatchers.IO (or some other thread pool in the future). While this helps with predictability of behaviour, it removes flexibility from the callers around scheduling and cancelling work. On another hand, it helps prevent some footguns of doing blocking IO on a UI thread, it simplifies interacting with the storage layer, and forces use of suspend functions on the callers whenever appropriate.

We don't always care about what happens with the work we schedule. For recording a visit, we really just want to kick off that work and move on. For retrieving visits, we obviously need to wait for it to complete. It's possible we might also want to be able to cancel that work (if an engine view is destroyed?). For autocomplete results, we care about the results and definitely want to have cancellation machinery in place (how deep it should go is another question... down into the places rust lib..? see mozilla/application-services#265). But, this can also be accomplished at the consumer level, and not by using Deferred or callbacks. E.g. that's the approach taken in SuggestionProvider.onInputChanged - see pocmo@e9df44a.

For now, I think it makes most sense to approach this on a method-by-method basis.

csadilek · 2018-10-31T23:07:18Z

While this helps with predictability of behaviour, it removes flexibility from the callers around scheduling and cancelling work

@grigoryk @pocmo and I talked about this as well today. It seems to me that it should be perfectly reasonable for a library API to return Deferred, as opposed to making all calls synchronous. Maybe the sweet spot here is that the library should use a default coroutine scope and dispatcher, but also allow callers to specify an alternative? Still wrapping my head around it as well...

csadilek · 2018-11-01T03:46:27Z

.../browser/storage-sync-places/src/main/java/mozilla/components/browser/storage/places/Conn.kt

+ */
+internal object Conn {
+    @Volatile
+    private var placesConnection: PlacesConnection? = null


It seems we need to call close on PlacesConnection. Need to think about how we do this otherwise we'd leak every time the app process is killed/restarted. Maybe expose a close method on Conn?

@csadilek yup, good call. I've updated HistoryStorage to let us do that.

codecov · 2018-11-02T08:33:55Z

Codecov Report

Merging #1164 into master will decrease coverage by 1.57%.
The diff coverage is 56.36%.

@@             Coverage Diff             @@
##             master   #1164      +/-   ##
===========================================
- Coverage     82.57%     81%   -1.58%     
+ Complexity     1628     905     -723     
===========================================
  Files           202     115      -87     
  Lines          6033    3511    -2522     
  Branches        970     565     -405     
===========================================
- Hits           4982    2844    -2138     
+ Misses          596     400     -196     
+ Partials        455     267     -188

Impacted Files	Coverage Δ	Complexity Δ
...illa/components/browser/storage/sync/Connection.kt	`0% <0%> (ø)`	`0 <0> (?)`
...s/browser/storage/memory/InMemoryHistoryStorage.kt	`88% <100%> (ø)`	`25 <12> (ø)`	⬇️
...mponents/browser/engine/system/SystemEngineView.kt	`79.23% <11.11%> (-2.54%)`	`29 <0> (ø)`
...nents/browser/storage/sync/PlacesHistoryStorage.kt	`80.64% <80.64%> (ø)`	`11 <11> (?)`
...java/mozilla/components/concept/toolbar/Toolbar.kt	`61.42% <0%> (-24.69%)`	`0% <0%> (ø)`
...zilla/components/browser/toolbar/BrowserToolbar.kt	`89.83% <0%> (-10.17%)`	`42% <0%> (-11%)`
...lla/components/support/base/observer/Consumable.kt
...lla/telemetry/measurement/SequenceMeasurement.java
...in/java/mozilla/components/lib/jexl/ast/AstNode.kt
...tboard/source/kinto/HttpURLConnectionHttpClient.kt
... and 93 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18ed4fc...9a28351. Read the comment docs.

pocmo

Some things here and there. But as this is a big new thing we want to get in I'm fine with only addressing what you think you can/want address now and then iterate on it in the tree.

pocmo · 2018-11-02T09:27:55Z

samples/browser/src/main/java/org/mozilla/samples/browser/BrowserActivity.kt

+        super.onDestroy()
+
+        components.historyStorage.cleanup()
+    }


What does it mean to close this (or not close this) for the Rust implementation?

Reading/Writing data is not limited to this activity (e.g. other activities, background jobs, ..). Is this something that only needs to be done if everything in this process is done with the storage?

Activity.onDestroy() is not guaranteed to get called. See "Killable" column: https://developer.android.com/reference/android/app/Activity.html#ActivityLifecycle - What happens if this does not get called?

pocmo · 2018-11-02T09:29:37Z

components/concept/storage/src/main/java/mozilla/components/concept/storage/HistoryStorage.kt

     * corresponding page URI from [uris].
     */
-    fun getVisited(uris: List<String>, callback: (List<Boolean>) -> Unit)
+    fun getVisited(uris: List<String>): Deferred<List<Boolean>>


Deferred is definitely better than the callback.

But it means that a storage implementation will launch coroutines internally. And those are not in the control of the caller.

For the sake of and example imagine this would be called from the awesomebar:

AwesomeBar -> SuggestionProvider -> HistoryStorage -> getVisited()

The AwesomeBar launches scoped coroutines for all providers. And the Storage launches one inside getVisited(). The user updates the text and the AwesomeBar cancels the coroutines querying the providers. getVisited() continues to run and doesn't notice that because it runs its coroutine on a different scope.

So I wonder if Deferred is the best option here. For a caller it's simple to wrap the call in its own async {} block in a scope of its choice. I'm not sure if we win much if we do it internally here?

I'm really not sure what the best pattern for such cases is. I'll try to investigate a bit.

pocmo · 2018-11-02T09:30:56Z

...-sync-places/src/main/java/mozilla/components/browser/storage/places/PlacesHistoryStorage.kt

+ * Implementation of the [HistoryStorage] which is backed by a Rust Places lib via [PlacesConnection].
+ */
+@VisibleForTesting
+open class TestablePlacesHistoryStorage(private val places: PlacesAPI) : HistoryStorage {


Can the visibility be internal?

pocmo · 2018-11-02T09:42:06Z

...ngine-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt

+                settings.historyTrackingDelegate?.let { delegate ->
+                    runBlocking {
+                        delegate.onTitleChanged(url, title, privateMode)
+                    }


Does it need to be blocking?

pocmo · 2018-11-02T09:46:18Z

...age-memory/src/main/java/mozilla/components/browser/storage/memory/InMemoryHistoryStorage.kt

+
+    override fun cleanup() {
+        // GC will take care of our internal data structures, so there's nothing to do here.
+    }


Right now we call cleanup() from an Activity. With the activity going away this doesn't mean the process/engine and therefore the delegate going away though.

If we want to call this while parts of the app are still alive: Should we clear the map?

If we want to call this when the app process is getting killed: Okay, we do not need to do anything here. But there's no event we could call this from.

pocmo · 2018-11-02T09:59:05Z

.../browser/storage-sync-places/src/main/java/mozilla/components/browser/storage/places/Conn.kt

+const val DB_NAME = "places.sqlite"
+
+/**
+ * A singleton which holds a shared reference to [PlacesConnection].


Does PlacesConnection need to be a singleton or could we have multiple of those concurrently and this is just a design decision here? Another option would be to let the app pass in a "Connection" object that it creates and that only has internal methods for our component.

pocmo · 2018-11-02T10:00:10Z

.../browser/storage-sync-places/src/main/java/mozilla/components/browser/storage/places/Conn.kt

+            placesConnection!!.close()
+            placesConnection = null
+        }
+    }


Slightly unclean: We have a close() here and the user of PlacesConnection could call close too; creating a situation where our connection holder will return a closed instance.

pocmo · 2018-11-02T10:03:47Z

...-sync-places/src/main/java/mozilla/components/browser/storage/places/PlacesHistoryStorage.kt

+import org.mozilla.places.VisitObservation
+
+class PlacesHistoryStorage(context: Context)
+    : TestablePlacesHistoryStorage(Conn.getInstance(context.getDatabasePath(DB_NAME).canonicalPath))


What exactly is the reason for TestablePlacesHistoryStorage? I assume that you do not want to make PlacesHistoryStorage open. If this is just the case for mocking then creating this file in the component will allow you to just mock the non-open class: https://github.com/mozilla-mobile/android-components/blob/master/components/browser/engine-gecko/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker

pocmo · 2018-11-02T10:04:47Z

...-sync-places/src/main/java/mozilla/components/browser/storage/places/PlacesHistoryStorage.kt

+    }
+
+    override fun cleanup() {
+        Conn.close()


nit: if we close the connection here: Do we want to cancel all jobs of this coroutine scope too? You may close a connection that is in use here.

pocmo · 2018-11-02T10:27:02Z

settings.gradle

 setupProject(':browser-search', 'components/browser/search', 'Search plugins and companion code to load, parse and use them.')
 setupProject(':browser-session', 'components/browser/session', 'An abstract layer hiding the actual browser engine implementation.')
 setupProject(':browser-storage-memory', 'components/browser/storage-memory', 'A memory-backed implementation of core data storage.')
+setupProject(':browser-storage-sync-places', 'components/browser/storage-sync-places', 'A syncable, Rust Places-backed implementation of core data storage.')


This needs to be added to artifacts.yml.

And a changelog entry.. :)

csadilek

Great progress! Let's land/release this and file follow-ups as discussed?

Just some conflicts that need fixing.

grigoryk · 2018-11-06T00:53:05Z

I've changed how the underlying places lib is wrapped (it's much clearer now than when @csadilek and @pocmo took a look), added tests and did some other cleanup work. Will land this once TC is green.

This is necessary because places lib was only included in 0.7.2, and various necessary changes to the places lib landed in 0.7.3, 0.8.0 and 0.8.1 to make API more ergonomic and testable.

Underlying HistoryStorage implementations might be blocking on IO (Rust Places, room), or migth be non-blocking (in-memory). A Deferred return type for get* methods makes it convenient to wrap both regular and `async` operations. Use of 'suspend' functions serves the same role for write* methods. This change makes it obvious at the engine call-site that the underlying implementation might suspend, at which point we can wrap calls in `runBlocking` or use another coroutine builder.

A storage implementation might need to cleanup its allocated resources, and this provides an entry point for that to happen. An example might be closing a native database connection that's maintained by the Rust Places library.

grigoryk requested a review from a team as a code owner October 25, 2018 00:59

grigoryk changed the title ~~WIP first pass on the HistoryTrackingDelegate implementation backed by Rust Places~~ WIP Closes #1126: HistoryTrackingDelegate implementation backed by Rust Places library Oct 25, 2018

grigoryk changed the title ~~WIP Closes #1126: HistoryTrackingDelegate implementation backed by Rust Places library~~ Closes #1126: WIP HistoryTrackingDelegate implementation backed by Rust Places library Oct 25, 2018

grigoryk mentioned this pull request Oct 25, 2018

Rust component- backed implementation of the HistoryTrackingDelegate #1126

Closed

pocmo reviewed Oct 26, 2018

View reviewed changes

grigoryk force-pushed the historyDelegatePlaces branch from ddd4dd8 to 77c4782 Compare October 27, 2018 01:35

grigoryk changed the title ~~Closes #1126: WIP HistoryTrackingDelegate implementation backed by Rust Places library~~ Closes #1126: WIP Rust Places backed implementation of concept-storage Oct 27, 2018

grigoryk mentioned this pull request Oct 30, 2018

Generic history capture hooked up to our component mozilla/application-services#283

Closed

grigoryk force-pushed the historyDelegatePlaces branch 6 times, most recently from 9948212 to b0f397a Compare October 31, 2018 05:33

grigoryk changed the title ~~Closes #1126: WIP Rust Places backed implementation of concept-storage~~ Closes #1126: Rust Places backed implementation of concept-storage Oct 31, 2018

grigoryk requested review from csadilek and pocmo October 31, 2018 05:42

grigoryk mentioned this pull request Oct 31, 2018

Allow using LibPlacesFFI in environments that don't have places native lib mozilla/application-services#342

Closed

pocmo added the 🕵️‍♀️ needs review PRs that need to be reviewed label Oct 31, 2018

grigoryk mentioned this pull request Oct 31, 2018

WIP towards a places component #1240

Closed

csadilek reviewed Nov 1, 2018

View reviewed changes

grigoryk force-pushed the historyDelegatePlaces branch 2 times, most recently from b0f397a to 8a268e7 Compare November 1, 2018 21:11

grigoryk force-pushed the historyDelegatePlaces branch 6 times, most recently from 6612687 to 4d591b0 Compare November 2, 2018 07:55

pocmo self-assigned this Nov 2, 2018

pocmo approved these changes Nov 2, 2018

View reviewed changes

pocmo added 🛬 needs landing PRs that are ready to land ✏️ needs changes PRs that need some changes/fixes before they can land and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels Nov 2, 2018

pocmo removed their assignment Nov 2, 2018

csadilek approved these changes Nov 5, 2018

View reviewed changes

grigoryk force-pushed the historyDelegatePlaces branch 2 times, most recently from 71f9a8f to 4b845b8 Compare November 6, 2018 00:49

Grisha Kruglov added 6 commits November 5, 2018 16:53

Bump app-services dependency to 0.8.1

c2e704d

This is necessary because places lib was only included in 0.7.2, and various necessary changes to the places lib landed in 0.7.3, 0.8.0 and 0.8.1 to make API more ergonomic and testable.

Add 'cleanup' method to HistoryStorage concept

dcaf924

A storage implementation might need to cleanup its allocated resources, and this provides an entry point for that to happen. An example might be closing a native database connection that's maintained by the Rust Places library.

Add 'limit' to HistoryStorage's getSuggestion concept method

61c4724

A Rust Places backed implementation of concept-storage

bd3204d

Updated changelog to reflect concept-* and browser-* changes

9a28351

grigoryk force-pushed the historyDelegatePlaces branch from 4b845b8 to 9a28351 Compare November 6, 2018 00:53

grigoryk merged commit f3e8295 into mozilla-mobile:master Nov 6, 2018

grigoryk deleted the historyDelegatePlaces branch November 6, 2018 01:27

grigoryk mentioned this pull request Nov 6, 2018

Integrate the history tracking and storage components. mozilla-mobile/reference-browser#40

Closed

grigoryk mentioned this pull request Nov 14, 2018

Improve lifecycle management of RustPlacesConnection (fix SQLiteBusy/PlacesConnectionBusy errors) #1369

Closed

Uh oh!

Closes #1126: Rust Places backed implementation of concept-storage #1164

Closes #1126: Rust Places backed implementation of concept-storage #1164

Uh oh!

Conversation

grigoryk commented Oct 25, 2018

Uh oh!

grigoryk commented Oct 25, 2018

Uh oh!

grigoryk commented Oct 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grigoryk commented Oct 31, 2018

Uh oh!

grigoryk commented Oct 31, 2018

Uh oh!

grigoryk commented Oct 31, 2018

Uh oh!

grigoryk commented Oct 31, 2018

Uh oh!

csadilek commented Oct 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pocmo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csadilek left a comment

Choose a reason for hiding this comment

Uh oh!

grigoryk commented Nov 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Nov 2, 2018 •

edited

Loading