501595 - Simple storage: Implement method deleteDatabaseFile

Status: RESOLVED FIXED

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

Description

[Drew Willcoxon :adw]

The method to delete a feature's backing database is not yet implemented.  Eventually this method will be called by the environment when a feature is purged.  I'm creating this bug to coordinate Arun's efforts, since he's already posted a patch in bug 496694.

Comment 1

[Vatsil]

Attachment: patch v1

Comment 2

[Drew Willcoxon :adw]

Comment on attachment 388031 [details] [diff] [review]
patch v1

Looks good, a few things:

>   this.deleteDatabaseFile = function SimpleStorage_deleteDatabaseFile() {
>-    throw new Error("NOT YET IMPLEMENTED");
>+    var file = getDatabaseFile(this.featureId);

this.featureId -> aFeatureId

>+    if(file.exists()) {
>+      file.remove();
>+    }

if (file.exists())

Since we're following Atul's style [1] more or less, please remove the braces, and you should pass false to file.remove().

We should also delete the directory structure if it becomes empty.  SimpleStorage_deleteBackingFile() already does that -- in fact could you break out that portion into a separate helper function and use it here?  You should also wrap the body of SimpleStorage_deleteDatabaseFile() in a try-catch, like SimpleStorage_deleteBackingFile() does.

>+// Returns the path to the given feature's dataBase file.

nit: dataBase -> database.  Also, this function returns an nsIFile, not a path.

[1] https://wiki.mozilla.org/User:Avarma/Coding_Style

Comment 3

[Vatsil]

Attachment: patch v2

Comment 4

[Drew Willcoxon :adw]

Comment on attachment 391361 [details] [diff] [review]
patch v2

>+function deleteBackingFileStructure(file) {
>+  try {
>+      if (!file.exists())
>+        return;
>+      file.remove(false);
>+
>+      // Remove the storage, feature, and jetpack directories (in that order)
>+      // if they are now empty.
>+      let dir = file.parent;
>+      for (let i = 0; i < 3; i++) {
>+        if (dir.directoryEntries.hasMoreElements())
>+          return;
>+        dir.remove(false);
>+        dir = dir.parent;
>+      }
>+    }
>+    catch (err) {
>+      // This method should only be called when the feature is purged, and
>+      // there's nothing the caller can really do if this fails.  So, just
>+      // report the error instead of throwing it.
>+      let e = new SimpleStorageError("Error deleting file: " + err);
>+      Components.utils.reportError(e);
>+    }
> }

3 nits: please use the aParam convention for declaring parameters, fix the indentation under the try, and insert this method in alphabetical order under Helper functions.

Hmm, I see a problem.  Since the async database API is deprecated, we don't create the database until a feature uses one of the deprecated methods.  However, getDatabaseFile() always creates the directory structure.  So if a feature never uses the database API, deleteDatabaseFile() will create the directory structure and then immediately delete it.  Not a big deal generally, but it shouldn't do that.  The "proper" way to fix it would be to split getDatabaseFile() into two functions that create the nsIFile (like getJsonFile()) and create the directory structure.  An easier way would be to:

* Rewrite getOrCreateDatabase() so that it takes an nsIFile.
* In SimpleStorageDeprecatedImpl__ensureDatabaseIsSetup(), call getDatabaseFile() and save the return value in a variable (say, databaseFile) whose scope is SimpleStorageDeprecatedImpl() (like dbConn and suppressDeprecationWarnings).  Pass that variable to getOrCreateDatabase().
* In SimpleStorage_deleteDatabaseFile(), first check that databaseFile is defined and if it is, pass it to deleteBackingFileStructure().

Since this code is deprecated, the easier way is fine.  Or if you want to try the "proper" way or a simpler way, go for it.

Comment 5

[Vatsil]

Attachment: patch v3

Comment 6

[Vatsil]

Attachment: patch v4

Comment 7

[Drew Willcoxon :adw]

Comment on attachment 391400 [details] [diff] [review]
patch v4

Fantastic.  This is much better than what I suggested.  I'll push it.  Thanks!

Comment 8

[Drew Willcoxon :adw]

http://hg.mozilla.org/labs/jetpack/rev/6dfcb2b860cd