Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add placesHistoryStorage as a history provider #184

Merged
merged 3 commits into from Nov 15, 2018

Conversation

@mhammond
Copy link
Contributor

@mhammond mhammond commented Nov 14, 2018

This patch adds places as a history provider to the reference browser. As things stand, this will conflict with #40 - that PR hooks up the "generic" provider while this hooks up the "places" provider - I'm assuming that we want the places one, although I'm not sure if there was some intention to have that be configurable?

Because the geckoview changes necessary to collect history haven't landed yet, (a) I haven't tested that part of this patch actually works, and (b) for the awesomebar to work you will need to have copied in a places.sqlite generated by rust code in the app-services repo - but this seems clean enough to think about landing even before we have that set up.

Other feedback:

  • It's also very nice how easy this was to put together - such a small patch for some powerful functionality! Also, the improved awesomebar looks great when there's some history - nice work @pocmo (and whoever helped!)

  • @grigoryk landed some awesome code which took our places needs into account even when the work he was directly doing didn't need it - thanks!

  • @linacambridge 's #40 made this very easy and IIUC, hopes to shepherd geckoview changes fairly soon, which will make this all come together perfectly!

@mhammond mhammond requested a review from mozilla-mobile/act as a code owner Nov 14, 2018
@pocmo
pocmo approved these changes Nov 14, 2018
Copy link
Contributor

@pocmo pocmo left a comment

Nice :)

It looks like this PR is missing the additions to Dependencies.kt:

No such property: mozilla_concept_storage for class: Deps

@pocmo pocmo requested a review from grigoryk Nov 14, 2018
@pocmo
pocmo approved these changes Nov 14, 2018
val historyTrackingFeature by lazy {
HistoryTrackingFeature(
this.components.engine,
this.components.placesHistoryStorage)

This comment has been minimized.

@pocmo

pocmo Nov 14, 2018
Contributor

nit: this is redundant here.

Copy link
Contributor

@grigoryk grigoryk left a comment

Easy-peasy! Thanks for the feedback, Mark!

@grigoryk
Copy link
Contributor

@grigoryk grigoryk commented Nov 14, 2018

I'm assuming that we want the places one, although I'm not sure if there was some intention to have that be configurable?

Yeah, let's just land places for the reference-browser. We use the simpler in-memory store in the android-components sample-browser. Lina's PR used the in-memory store because that's all we had at the time.

Because the geckoview changes necessary to collect history haven't landed yet, (a) I haven't tested that part of this patch actually works

You should be able to test this entirely if you build the reference browser against the SystemEngine, which fully supports history tracking.

(b) for the awesomebar to work you will need to have copied in a places.sqlite generated by rust code in the app-services repo

I'm not sure why you'd need to manually copy stuff if you're using GV? Do you mean to actually see some results, right? Because places.sqlite would have been created regardless.

Also, GV right now will emit title change events, which will be passed on to places via noteObservation. I'm guessing 'places' is just dropping these events on the floor (since there are no associated visits, and the places table needs to have "last visited" timestamps and whatnot).

@grigoryk
Copy link
Contributor

@grigoryk grigoryk commented Nov 14, 2018

I think this is the change to Dependencies.kt you'll need: https://github.com/mozilla-mobile/reference-browser/pull/40/files#diff-f6dcd12435c476b2cedd79bcd292b929 (except use the different storage).

@mhammond
Copy link
Contributor Author

@mhammond mhammond commented Nov 14, 2018

It looks like this PR is missing the additions to Dependencies.kt:

doh! That file also had a change so mozilla_android_components pointed at a snapshot - but I reverted the entire file instead of just that -snapshot change.

I'm not sure why you'd need to manually copy stuff if you're using GV? Do you mean to actually see some results, right? Because places.sqlite would have been created regardless.

Yes, that's correct - places.sqlite will be created but will not have visit data, so isn't going to find matches.

Also, GV right now will emit title change events, which will be passed on to places via noteObservation. I'm guessing 'places' is just dropping these events on the floor (since there are no associated visits, and the places table needs to have "last visited" timestamps and whatnot).

Hopefully we will do the right thing there now (it's ok to have a place without visits), but we can tackle issues there on the application-services side.

Thanks!

@pocmo pocmo merged commit 4649713 into mozilla-mobile:master Nov 15, 2018
1 check passed
1 check passed
Taskcluster (pull_request) TaskGroup: success
Details
@mhammond mhammond deleted the mhammond:add-places-history-storage branch Nov 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants