Gregor Wagner [:gwagner] Assignee
	Description • 13 years ago

Contacts, Settings, Activities are copying the same code over and over again.

	Gregor Wagner [:gwagner] Assignee
	Comment 3 • 13 years ago

Fabrice, I will update the patch once bug 748852 lands.

	Philipp von Weitershausen [:philikon]
	Comment 4 • 13 years ago

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

	Gregor Wagner [:gwagner] Assignee
	Comment 5 • 13 years ago

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.

	Gregor Wagner [:gwagner] Assignee
	Comment 6 • 13 years ago

Updated patch. Philipp feel free to steal the review :)

	[:fabrice] Fabrice Desré
	Comment 7 • 13 years ago

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

	Gregor Wagner [:gwagner] Assignee
	Comment 8 • 13 years ago

(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

	Gregor Wagner [:gwagner] Assignee
	Comment 9 • 13 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/730e41d306f0

	Ed Morley [:emorley]
	Comment 10 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/730e41d306f0