496694 - JEP 11 - Simple storage

Status: RESOLVED FIXED

(Mozilla Labs :: Jetpack Prototype, enhancement)

Description

[Drew Willcoxon :adw]

Attachment: patch v1

I've assigned this to myself since I'm submitting this bug and patch, but David and I have both been working on it.

We talked with Shawn about approaches in regard to mozStorage.  He recommended going with one database per Jetpack feature instead of a big one that all features would share.  That's what we've done.  We create the DB in the user's profdir at jetpack/featureId/storage/simple.sqlite, where featureId is the hashed URL of a given feature.  I figure at some point features will need sandboxed disk space for storing whatever... Maybe you guys already had thoughts on this?

A few minutes later Shawn also insisted on barring new sync I/O APIs altogether and got Shaver to agree with him, if indeed the aim is to uplift Jetpack.  So this API is completely async.

Comes with tests.  I have not yet tested, however, writing a feature that uses this, so the meta-stuff in jetpack-environment.js is untested.

Todo: Log mozStorage errors.  Remove DB when feature is purged.  These shouldn't take long to do but I'm sleepy now.

Requesting first-pass review to make sure our approach is OK.

Comment 1

[Drew Willcoxon :adw]

Oh, one more thing.  Originally we had the conventional SimpleStorage.prototype = { ... } construct, setting private members (like the DB connection) in the constructor directly on each SimpleStorage instance.  But I had concerns about a feature mucking with its DB connection and more importantly the potential for its mucking with another's.  I don't have a firm grasp yet on how everything in Jetpack fits together, so maybe that won't be an issue, but this patch effectively makes the connection private.

Comment 2

[Aza Raskin [:aza]]

Having a folder which contains resources of the jetpack by featureId makes sense. I, at least, haven't thought deeply about this part yet.

From a programmer ergonomics stand-point, I'm not sure that async is always very nice. That is, having to litter your code with callbacks to simply store a value (although, JS Promises might be a nice alternative).

Anyway, can you update the JEP appropriately with the new async stuff?

Comment 3

[David Dahl :ddahl]

After consulting with the MozStorage module owner, sdwilsh, sychronous mozStorage is not only deprecated, but will have trouble in the uplift process. 

I think we are going to make simple storage "easy enough". The flipside is UI locking:(

Comment 4

[David Dahl :ddahl]

The other thing that can be done is see how this api works against a JSON file on disk. Perhaps the jetpack can be configured like: simpleStorageBackend: filesystem or simpleStorageBackend: mozStorage

The "best" of both worlds?

Comment 5

[Dietrich Ayala (:dietrich)]

(In reply to comment #4)
> The other thing that can be done is see how this api works against a JSON file
> on disk. Perhaps the jetpack can be configured like: simpleStorageBackend:
> filesystem or simpleStorageBackend: mozStorage
> 
> The "best" of both worlds?

simpleStorageBackend: browsercouch

Comment 6

[Atul Varma [:atul]]

This is great! I especially like the documentation and all the tests.

Right now all the JS files in extension/content/js aren't actually JS modules, though--Jetpack's codebase is designed to be loaded entirely in a page so that it's easy to make changes to the code and simply reload about:jetpack instead of restarting the entire browser, which is what one needs to do with jsm's (though we'll probably be introducing our own module system for Jetpack that combines the best of both worlds).  In any case, as a result, your js module is effectively being loaded as a script in a page, so all its non-exported symbols are being put into the global scope of the page too.

The easiest way to fix this would be to move the simple-storage.js file into the "extension/modules" directory (where we keep all our js modules) and then in the importer for jetpack.storage.simple just load the jsm and return an instance of simple storage. I don't want to make you turn it into a page script now since we'll probably be moving to a reloadable module system later anyways.

(Also, jetpack-environment.js has changed a lot since you submitted your patch, so you'll have to modify that part of your patch... Sorry!)

Comment 7

[Drew Willcoxon :adw]

Updated the JEP for asynchronicity.  Also, with the current patch there's no way for the caller to be notified on error, so the JEP talks about that too.  I'll wait for your approval before I update the patch.  (I feel like we've rained on your parade with this asynchronicity talk... :\ )

Comment 8

[Drew Willcoxon :adw]

Attachment: patch v2

Addresses comment 6 and implements Aza's suggestion of callbacks as functions or objects.

Comment 9

[Aza Raskin [:aza]]

Awesome! Thanks for the quick turn around, Drew!

Comment 10

[Aza Raskin [:aza]]

I've committed the patch! I tried to submit as Drew's user name, but unfortunately I failed. Apparently just changing the user name in the commit log doesn't work. For future reference, it appears that when committing someone else's work you have to go through the full hg commit -u [USER NAME] dance.

Comment 11

[Vatsil]

Attachment: implements TODO for SimpleStorage.deleteDatabaseFile