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

History deletion #591

Merged
merged 2 commits into from Feb 1, 2019

Conversation

Projects
None yet
4 participants
@mhammond
Copy link
Member

commented Jan 31, 2019

  • This implements delete_visits_since to delete all visits since the specified
    timestamp. It also exposes this to the FFI, although that's untested.

  • This exposes the existing storage::history::delete_place_by_guid() as
    places_delete_place, taking a URL as a param. There's no way to choose to
    not sync this deletion - we can worry about that if it turns out to be a
    real requirement.

  • Note that origins are not deleted with this patch - this should happen
    automagically once #429 lands.

  • This patch doesn't change the "reset" path so that reset is optionally
    synced or not - I think that scenario needs more thought so can be done
    later once requirements are clear.

  • If 'bookmarks' lands before this, I'll probably add a way to expire history
    to places-utils.rs - I've really no idea how efficient it is yet. One way
    this is worse than desktop is that it manages to recalculate new frecencies
    in a single sql statement whereas this has one sql statement per deleted page.

@mhammond mhammond added the review label Jan 31, 2019

@mhammond mhammond self-assigned this Jan 31, 2019

@mhammond mhammond requested review from thomcc and linacambridge Jan 31, 2019

// probably is possible. We don't currently do that, but might later, so
// we do it anyway.
let remove_ids: Vec<RowId> = to_remove.iter().map(|p| p.id).collect();
sql_support::each_chunk(&remove_ids, |chunk, _| -> Result<()> {

This comment has been minimized.

Copy link
@mhammond

mhammond Jan 31, 2019

Author Member

This function doesn't actually need to use chunking as it is called by a chunking parent. I should make that change, maybe with an assertion that the number of pages is less than the max chunk size.

This comment has been minimized.

Copy link
@thomcc

thomcc Jan 31, 2019

Contributor

In that case, it's collect() is also not necessary, as you can pass an IntoIter directly into execute

@thomcc

thomcc approved these changes Jan 31, 2019

Copy link
Contributor

left a comment

Looks good, modulo several nits.

Also: Please add a changelog entry to CHANGELOG.md.

// affected, then delete all visits, then delete all place ids in the set
// which are orphans after the delete.
let sql = "SELECT id, place_id FROM moz_historyvisits WHERE visit_date >= :since";
let visits =

This comment has been minimized.

Copy link
@thomcc

thomcc Jan 31, 2019

Contributor

It worries me that something like delete_visits_since(db, 0) will load every visit and place rowid into memory... Since desktop does it this way we're probably fine, but it's still pretty concerning.

This comment has been minimized.

Copy link
@linacambridge

linacambridge Feb 1, 2019

Member

Hmm...we could use an AFTER UPDATE OF local_visit_count, remote_visit_count ON moz_places WHEN local_visit_count + remote_visit_count = 0 trigger to clean up orphaned pages, but we still need to handle frecency recalculation.

Show resolved Hide resolved components/places/src/storage/history.rs
Show resolved Hide resolved components/places/src/storage/history.rs Outdated
Show resolved Hide resolved components/places/src/storage/history.rs Outdated
@rfk

This comment has been minimized.

Copy link
Member

commented Jan 31, 2019

This exposes the existing storage::history::delete_place_by_guid() as
places_delete_place, taking a URL as a param.

Asking because this has been a problem in the past...where and how do we track the work of exposing this into a-c and up into the higher-level history features of that platform.

@thomcc

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2019

This exposes the existing storage::history::delete_place_by_guid() as
places_delete_place, taking a URL as a param.

Asking because this has been a problem in the past...where and how do we track the work of exposing this into a-c and up into the higher-level history features of that platform.

For places, we don't have a wrapper in A-C the way we do for logins or fxa (I'm unsure of the reason behind this, but I don't mind the situation so it's fine), so most everything happens in this repo.

@thomcc thomcc force-pushed the history-deletion branch from 2cd4555 to 4c2c227 Feb 1, 2019

@thomcc

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2019

(@mhammond Sorry in advance for this, I misread the PTO calendar and thought you were on PTO for longer than you actually are, so i took up this and bookmarks. If it doesn't land before you get back, I did force push, so you'll have to deal with that, sorry 🙁)

@thomcc thomcc force-pushed the history-deletion branch 2 times, most recently from 83acecd to 687fc47 Feb 1, 2019

@linacambridge
Copy link
Member

left a comment

This looks good to me, let me fix up my own nit, and then we can land.

Show resolved Hide resolved ...ents/places/android/src/main/java/org/mozilla/places/PlacesConnection.kt Outdated
Show resolved Hide resolved ...ents/places/android/src/main/java/org/mozilla/places/PlacesConnection.kt
// affected, then delete all visits, then delete all place ids in the set
// which are orphans after the delete.
let sql = "SELECT id, place_id FROM moz_historyvisits WHERE visit_date >= :since";
let visits =

This comment has been minimized.

Copy link
@linacambridge

linacambridge Feb 1, 2019

Member

Hmm...we could use an AFTER UPDATE OF local_visit_count, remote_visit_count ON moz_places WHEN local_visit_count + remote_visit_count = 0 trigger to clean up orphaned pages, but we still need to handle frecency recalculation.

@thomcc thomcc force-pushed the history-deletion branch from 687fc47 to 1ad9499 Feb 1, 2019

mhammond and others added some commits Jan 31, 2019

History deletion
* This implements `delete_visits_since` to delete all visits since the specified
  timestamp. It also exposes this to the FFI, although that's untested.

* This exposes the existing `storage::history::delete_place_by_guid()` as
  `places_delete_place`, taking a URL as a param. There's no way to choose to
  not sync this deletion - we can worry about that if it turns out to be a
  real requirement.

* Note that origins are not deleted with this patch - this should happen
  automagically once #429 lands.

* This patch doesn't change the "reset" path so that reset is optionally
  synced or not - I think that scenario needs more thought so can be done
  later once requirements are clear.

* If 'bookmarks' lands before this, I'll probably add a way to expire history
  to `places-utils.rs` - I've really no idea how efficient it is yet. One way
  this is worse than desktop is that it manages to recalculate new frecencies
  in a single sql statement whereas this has one sql statement per deleted page.

@thomcc thomcc force-pushed the history-deletion branch from 1ad9499 to 04f3cb8 Feb 1, 2019

@thomcc thomcc merged commit a1be526 into master Feb 1, 2019

1 of 5 checks passed

Taskcluster (pull_request) TaskGroup: Pending (for pull_request.synchronize)
Details
ci/circleci: Rust tests - beta CircleCI is running your tests
Details
ci/circleci: Rust tests - stable CircleCI is running your tests
Details
ci/circleci: Sync integration tests CircleCI is running your tests
Details
ci/circleci: Check Rust formatting Your tests passed on CircleCI!
Details

@thomcc thomcc deleted the history-deletion branch Feb 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.