Skip to content

Conversation

@linabutler
Copy link

Cargo-culting @grigoryk's changes for the sample browser in mozilla-mobile/android-components#1155 to use in-memory history storage. 😸

@linabutler linabutler requested a review from a team as a code owner October 31, 2018 02:55
Copy link
Contributor

@pocmo pocmo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I love how this is only a couple of lines of code and you get history tracking in your browser. :)

I think we want to create HistoryTrackingFeature earlier independent from the Fragment. But that's not blocking -> ✅


historyTrackingFeature = HistoryTrackingFeature(
requireComponents.engine,
requireComponents.historyStorage)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is more like an "application level" feature and we would want to create this in BrowserApplication.onCreate() so that we track history even when there's no fragment/UI around. (cc @grigoryk)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what "tracking history without a browser fragment/ui around" means, but generally I agree, history tracking isn't conceptually tied to the UI, so moving this over to BrowserApplication makes sense.

Separately, we'd want to make that change in the sample-browser as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what "tracking history without a browser fragment/ui around" means

We are able to load URLs while the app is in the background and while there may not be any UI (but we still want to record history). Fragments come and go. There's no reason we want to re-register a new delegate whenever there's a new BrowserFragment getting created.

@linabutler
Copy link
Author

This will fix #89. ✨

Copy link
Contributor

@grigoryk grigoryk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty cool! One more line change, and this is will be backed by places ;)


historyTrackingFeature = HistoryTrackingFeature(
requireComponents.engine,
requireComponents.historyStorage)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what "tracking history without a browser fragment/ui around" means, but generally I agree, history tracking isn't conceptually tied to the UI, so moving this over to BrowserApplication makes sense.

Separately, we'd want to make that change in the sample-browser as well.

@grigoryk
Copy link
Contributor

grigoryk commented Nov 6, 2018

Now that mozilla-mobile/android-components#1164 landed and released in 0.30, this can be backed by Rust Places instead of an in-memory storage.

@grigoryk
Copy link
Contributor

grigoryk commented Nov 8, 2018

To avoid creating a new issue, @linacambridge could you update this to use browser-storage-sync?

@Amejia481 Amejia481 changed the title Integrate the history tracking and storage components Integrate the history tracking and storage components. Nov 9, 2018
@pocmo
Copy link
Contributor

pocmo commented Nov 15, 2018

Closing in favor of #184.

@pocmo pocmo closed this 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

Development

Successfully merging this pull request may close these issues.

3 participants