526688 - remove user-set settings and other data in simple storage when user purges jetpack

Status: RESOLVED FIXED

(Mozilla Labs :: Jetpack Prototype, defect, P2)

Description

[Myk Melez [:myk] [@mykmelez]]

When a user purges a jetpack, Jetpack should remove its user-set settings.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Drew suggested to me today that it'd be worth removing other data in simple storage at the same time, and I agree, so updating the bug summary accordingly.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: deletes data when purging jetpack

Here's a patch that deletes data from simple storage when purging a jetpack.

In the process, I refactored the hash generation code into a hash utils module.  I could further factor out the process of generating an ID for a jetpack, so it all happens in the feed manager.  Would that make sense?

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: further factors out code

(In reply to comment #2)
> In the process, I refactored the hash generation code into a hash utils module.
>  I could further factor out the process of generating an ID for a jetpack, so
> it all happens in the feed manager.  Would that make sense?

Here's the version that does that.  It's not clear to me whether it makes sense to generate the ID in the feed, but it's probably a bit safer, since then we only generate it in one place.

Comment 4

[Ed Lee :Mardak]

Comment on attachment 415794 [details] [diff] [review]
patch v2: further factors out code

>+++ b/extension/ubiquity-modules/feedmanager.js
>@@ -487,16 +489,24 @@ FMgrProto.__makeFeed = function FMgr___m
>   feedInfo.purge = function feedInfo_purge() {
>     FEED_ANNOS.forEach(
>       function(ann) {
>         if (annSvc.pageHasAnnotation(uri, ann))
>           annSvc.removePageAnnotation(uri, ann);
>       });
>+
>+    // Delete the data in the simple store.
>+    let s = {};
>+    Components.utils.import("resource://jetpack/modules/simple-storage.js", s);
>+    let id = feedInfo.id;
>+    new s.simpleStorage.SimpleStorage(id).deleteBackingFile();
>+    new s.simpleStorage.SimpleStorage(id, "settings").deleteBackingFile();
As we get more things using the simple storage, it might be easier to just have a jetpack-runtime facing function such as.. s.simpleStore.purge(id); which would just recursively remove jetpack/<featureid>/storage

Comment 5

[Ed Lee :Mardak]

(We don't need to do the s.simpleStore.purge thing now. The patch is fine as is. And if we get more functionality that want purge hooks.. we can better standardize/refactor on a purge interface. implements Purgable? ;))

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Fixed by changeset http://hg.mozilla.org/labs/jetpack/rev/7285e21db49d.