Last Comment Bug 746933 - Create a JS helper module for common IndexedDB functionality
: Create a JS helper module for common IndexedDB functionality
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: IndexedDB (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: Gregor Wagner [:gwagner]
:
Mentors:
Depends on:
Blocks: 749811 822406
  Show dependency treegraph
 
Reported: 2012-04-19 04:14 PDT by Gregor Wagner [:gwagner]
Modified: 2012-12-17 11:49 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WiP (12.01 KB, patch)
2012-04-19 04:17 PDT, Gregor Wagner [:gwagner]
no flags Details | Diff | Review
Patch (18.57 KB, patch)
2012-04-24 16:11 PDT, Gregor Wagner [:gwagner]
no flags Details | Diff | Review
patch (18.16 KB, patch)
2012-04-25 16:28 PDT, Gregor Wagner [:gwagner]
fabrice: review+
Details | Diff | Review

Description Gregor Wagner [:gwagner] 2012-04-19 04:14:00 PDT
Contacts, Settings, Activities are copying the same code over and over again.
Comment 1 Gregor Wagner [:gwagner] 2012-04-19 04:17:21 PDT
Created attachment 616514 [details] [diff] [review]
WiP
Comment 2 Gregor Wagner [:gwagner] 2012-04-24 16:11:44 PDT
Created attachment 618091 [details] [diff] [review]
Patch
Comment 3 Gregor Wagner [:gwagner] 2012-04-25 14:32:37 PDT
Fabrice, I will update the patch once bug 748852 lands.
Comment 4 Philipp von Weitershausen [:philikon] 2012-04-25 15:13:22 PDT
Comment on attachment 618091 [details] [diff] [review]
Patch

Review of attachment 618091 [details] [diff] [review]:
-----------------------------------------------------------------

I like this. A quick drive-by:

::: dom/base/IndexedDBHelper.jsm
@@ +8,5 @@
> +let DEBUG = 0;
> +if (DEBUG)
> +  debug = function (s) { dump("-*- IndexedDBHelper: " + s + "\n"); }
> +else
> +  debug = function (s) {}

nit: braces

@@ +29,5 @@
> +
> +  // Close the database
> +  close: function close() {
> +    if (this._db)
> +      this._db.close();

nit: braces

@@ +68,5 @@
> +
> +        default:
> +          debug("No idea what to do with old database version:" + aEvent.oldVersion);
> +          aFailureCb(aEvent.target.errorMessage);
> +          break;

Err, that's not going to work. We need to be able to handle schema upgrades. I suggest calling "self.upgradeSchema" here.

@@ +106,5 @@
> +   * @param callback
> +   *        Function to call when the transaction is available. It will
> +   *        be invoked with the transaction and the 'aDBStoreName' object store.
> +   * @param successCb
> +   *        Success callback to call on a successful transaction commit.

You also want to document that successCb will be called with txn.result. Basically (ab)using the transaction object as a message passing method between the various callbacks. This is a *convention* that I invented, not something that's a given, so I think it should be documented.

@@ +149,5 @@
> +      callback(txn, store);
> +    }.bind(this), failureCb);
> +  },
> +
> +  getTransaction: function getTransaction(aType) {

Some docs would be nice on what this method does and how it's different from newTxn(). Also, the naming isn't optimal: It doesn't really just *get* a transaction, it creates one. (And consistency with transaction/txn would be nice.)

::: dom/base/Makefile.in
@@ +75,1 @@
>  		$(NULL)

Nit: 2 spaces for indentation
Comment 5 Gregor Wagner [:gwagner] 2012-04-25 16:21:19 PDT
Thanks Philipp!

> 
> Err, that's not going to work. We need to be able to handle schema upgrades.
> I suggest calling "self.upgradeSchema" here.

Yeah I needed some inspiration there :) I added now createSchema for version 0 and upgradeSchema that has the old and new version as argument.


> > +  getTransaction: function getTransaction(aType) {
> 
> Some docs would be nice on what this method does and how it's different from
> newTxn(). Also, the naming isn't optimal: It doesn't really just *get* a
> transaction, it creates one. (And consistency with transaction/txn would be
> nice.)

I wanted to get rid of using the DB_NAME  and similar from outside but it's need to get the objectStore as well. I remove this function again.
Comment 6 Gregor Wagner [:gwagner] 2012-04-25 16:28:55 PDT
Created attachment 618480 [details] [diff] [review]
patch

Updated patch. Philipp feel free to steal the review :)
Comment 7 [:fabrice] Fabrice Desré 2012-04-26 14:10:54 PDT
Comment on attachment 618480 [details] [diff] [review]
patch

Review of attachment 618480 [details] [diff] [review]:
-----------------------------------------------------------------

Look good to me! hooray for less code!

::: dom/base/IndexedDBHelper.jsm
@@ +138,5 @@
> +   * 
> +   * @param aDBName
> +   *        DB name for the open call.
> +   * @param aDBVersion
> +   *        Current DB version. Make sure the proper handling of createSchema.

I don't understand the "Make sure the proper handling of createSchema" part

::: dom/base/Makefile.in
@@ +60,1 @@
>  

nit: indentation
Comment 8 Gregor Wagner [:gwagner] 2012-04-26 14:56:48 PDT
(In reply to Fabrice Desré [:fabrice] from comment #7)
> Comment on attachment 618480 [details] [diff] [review]
> patch
> 
> Review of attachment 618480 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Look good to me! hooray for less code!
> 
> ::: dom/base/IndexedDBHelper.jsm
> @@ +138,5 @@
> > +   * 
> > +   * @param aDBName
> > +   *        DB name for the open call.
> > +   * @param aDBVersion
> > +   *        Current DB version. Make sure the proper handling of createSchema.
> 
> I don't understand the "Make sure the proper handling of createSchema" part

I changed it. It should mean that the user has to implement createSchema and upgradeSchema
Comment 10 Ed Morley [:emorley] 2012-04-27 07:09:27 PDT
https://hg.mozilla.org/mozilla-central/rev/730e41d306f0

Note You need to log in before you can comment on or make changes to this bug.