I'm looking for feedback on the proposed API (with sample code) for an
asynchronous API for mozStorage.  What I've got so far can be found here:
http://developer.mozilla.org/en/docs/User_talk:Comrade693#Async_Stora...

This is something we are looking at for Firefox.next, so the sooner you
give feedback, the better. :)

Cheers,

Shawn

Thanks for taking this on, Shawn!

I think we should make async API as similar to the synchronous one as
possible. What's the reason behind basing it on HTML5 SQL? Developers
are already accustomed to existing mozStorage APIs, so we shouldn't
deviate from them without a good reason. Learning curve aside, it would
create a big inconsistency. Let's take a look at the differences:

* AsyncTransaction is a completely different animal than a traditional
transaction.
* mozStorage APIs revolve around statement objects. There's no explicit
statement here.
* Parameters are bound in a different way
* Results are read in a different way

By the way, AsyncTransaction.execute() reads as if the transaction is
executed, not the statement. C.f. mozIStorageStatement.execute().

Another concern is the callback. Let's make it a single function instead
of an object implementing given methods. In the latter case you've got
to implement QI and the code that handles the statement is more
scattered. If you use many async statements, you usually make a
<code>switch</code> and forward the callbacks to separate dedicated
functions, anyway.

Here is the Shawn's example, adapted to use my idea of the API. Please
note that I have no knowledge of the internals of sqlite and mozStorage,
so I don't know how feasible this is.

// conn is a mozIStorageConnection
let sql = "SELECT * FROM moz_downloads WHERE state = ?1";
let stmt = conn.createStatement(sql);
stmt.bindInt32Parameter(0, Ci.nsIDownloadManager.DOWNLOAD_FINISHED);
stmt.executeAsync(callback);

function callback(aStatement, aResult, aError) {
     if (aError)
         // update UI accordingly

     for (let row in aResult) {
         // process data
         // should be able to access data in a row by name or argument
number
     }

Alternatively, the callback could be defined in the style of XMLHttpRequest.

stmt.onResultsReady = onResultsReady;
stmt.onError = onError;
stmt.executeAsync();

function onResultsRead(aStatement, aResults) {

function onError(aStatement, aError) {

- Adam

I am extensively using mozIStorage interfaces in an accounting xul
application, that is approaching production readiness. Shawn, thanks for
this opportunity to present my point.

Async calls are a great idea!

Comments on the proposed interface:

1. Is there any technical reasons that require AsyncTransaction?

It seems to be redundant otherwise. The best practice is use sql
parameters, which are to be bound after the statement is already
prepared and its syntax is verified. The counter-proposal is to add
'executeAsync()' to 'mozIStorageStatement'.

2. Separate method to obtain ResultSet is a right thing. We can probably
also change 'mozIStorageStatement' synchronous calls API in the same
way. Currently, the first call of 'executeStep()' has a special EXECUTE
meaning, while all subsequent ones actually mean FETCH ROW. Separating
this two meanings may be a good idea.

Instead of passing ResultSet proxy to a callback, let's:
    drop 'boolean executeStep();',
    drop 'void reset();',
    drop 'readonly attribute long state;'
    add  'readonly attribute mozIStorageValueArray resultSet;'
to 'mozIStorageStatement', and stop subclassing 'mozIStorageValueArray'
there. In turn 'mozIStorageValueArray' will receive
   'readonly attribute long state;',
   'void fetchNext();', and
   'void fetchNextAsync([optional] in Callback aCallback);'

So 'mozIStorageValueArray' (aka cursor) is separated from
'mozIStorageStatement'. The former can later be extended to provide
support for random access cursors. 'reset()' goes away entirely. It can
be invoked internally in 'execute()', 'executeAsync()' and 'finalize()'.

As a result, mozIStorage* will become more SQL-ish and friendlier to
other databases like MySQL or PostgreSQL. There is a legitimate question
"why do we care about other databases?". Here is one hypothetical
example: since we already keep a hefty part of user profile (history and
bookmarks) in a relational database, we can move the rest of valuable
data -- mainly passwords and extensions (yes! extensions) -- there as
well. So the profile will contain a database (or databases) and cached
data. This creates an opportunity for big organizations to use a central
database to store and manage extensions, web settings and browsing data
of their users. And if such a user logins simultaneously on several
workstations, no browsing data will be lost.

3. Finally, can we directly use nsIWebProgressListener2 as a callback
class in both 'executeAsync()' and 'fetchNextAsync()'?

This can be the first step of many to provide full database progress
feedback using progress bar. Long places operations (the main client of
storage) will clearly benefit from this feature.

4. And woooooohooooooo, as 3.0 RC1 has finally been tagged for building!

--
Sergey Yanovich

I based it off the HTML SQL spec for two reasons:
(1) A lot of mozilla stuff is based off of web APIs.  This lowers the
barrier to entry for new developers (who have done web work at least)
since they are generally somewhat familiar with existing APIs.
(2) I don't think we should create new APIs solely based on the existing
API.  It's my opinion that the current API is rather clunky in areas,
and the proposed API was meant to fix some of that.

Yes, and no.  Each statement has an implicit transaction wrapped around
it - the whole statement either succeeds or fails.  With that said, I'm
open to a better name on this.

I'm not sure this is a bad thing, but it is different.

It's my opinion that our current approach to binding parameters is
clunky and error-prone.  I'd love to fix that, and that's what I'm
trying to do here.

Same as above.  Note that since this is targeting Firefox.next, which is
supposed to come out some time in the fall, we don't want to change
existing APIs.  So we can add to our current API, but changing it should
be avoided if possible.

Implicit transaction again.  Although, I do believe this could be done
without the AsyncTransaction object which would probably ease this
concern of yours (I had a reason for doing it with AsyncTransaction
because I shared your line of thinking initially.  Something made me
change my mind, but I can't recall what that was currently.  When I
figure it out again, I'll post).

Letting one method handle both errors and handling of an actual result
is a form of Logical Cohesion which is generally frowned upon (yeah, we
do it in the mozilla codebase in various places).  Functional Cohesion
is preferred.  I recommend reading Code Complete 2nd Ed., Chapter 7 for
more details (I imagine wikipedia has something on it too).

Additionally, JS consumers don't have to implement the QI code, and they
don't even need to provide an error or result handler if they don't want
to.  XPConnect, as I understand it, will just throw
NS_ERROR_NOT_IMPLEMENTED if the back-end code tries to call it.  The
back-end should be able to handle that gracefully.

The problem with this approach is that preparing the statement (the
createStatement call) is often the most expensive operation for SELECT
statements.  With the current API we can do that work on the background
thread[pool].

The only reason why I don't like this approach is because it seems like
a lot more lines of code to accomplish the same thing.  With that said,
I'm indifferent about it.

Thanks for the great feedback.  Let me know if this addresses your concerns.

Cheers,

Shawn

At this point, I don't think so.  I had a reason at some point, but I
cannot recall what it was.  The only thing that might be useful is
leaving it in for the callback to handle, but then it should have some
properties on it, otherwise it seems useless (checking as to why the
HTML 5 spec did this would probably be useful too - I'll get on that).

I think that this might be the way to go, but I don't think we should
have them prepare it first (see previous post about that in reply to Adam).

I'd love to, but we don't want to have that much API churn for Firefox
3.1.  For Firefox 4 we can look at that.

I'm not sure I understand how that is useful, so you'd have to elaborate
on that.

Cheers,

Shawn

I've gone and incorporated some of the feedback from Adam and Sergey.
All changes are up on the wiki [1], but here's a summary:

(1) Dropped the AsyncTransaction interface, and moved its executeAsync
method onto mozIStorageConnection.  There was no reason for the
interface to exist, and it's one less thing consumers have to worry about.

(2) Added a SQLError interface that will be passed back to the callback
for errors.  There is, however, no definition as I'm not 100% sure what
we want to include when we notify that there was an error.

(3) Updated the sample code to go along with the new interface proposal.

Cheers,

Shawn

[1]
http://developer.mozilla.org/en/docs/User_talk:Comrade693#Async_Stora...

(I'm looking at the wiki, r71407)

Overall: It would probably be useful to explicitly state, near the top
of the wiki page, exactly which Mozilla version this is targeting -
sounds like Firefox.next / 1.9.nn?

On the other hand, this also means it's not possible to re-use statements.

I'm not quite sure how that second argument (the one with
the things to bind) can be implemented, given that XPIDL doesn't, as far
as I know, have method overloading.  It seems to take either a JS Array,
Object, or a C++ Parameters thing that is to be determined.  Would we
still get type-checking on the C++ side?  (Hey, if you somehow manage to
get that, all the better.)

Of course, I know about nil about API design ;)

(Off-topic note-to-self: trigger + mozIStorageFunction = win)

--
Mook

While strictly speaking true, there are ways to get around this, esp. if all
your calls are from JS into C++.  You can use nsIVariant, or declare no args in
the IDL and read them directly using XPConnect APIs.

-Boris

Sure. I mistook Firefox.next as Firefox 4.

I had in mind whole-sale database access like when History -> Show All
History is invoked. Our current Library window doesn't contain progress
bar, because it would be useless, since the routine that populates the
main tree is synchronous. But things will change, once async call
becomes possible. We can then provide substantially better user
experience by doing async loading and showing  progress in a progress bar.

However, in the context of a dot release, introducing this new
functionality to the mozIStorage* no longer seems a good idea.

A) I started to argue with the necessity of changing connection
interface, but then came around this:

This is a great argument in favor of creating a statement
asynchronously, but it is also an argument in favor of storing the
produced statement for re-use.

This may have been Shawn's motivation to introduce AsyncTransaction.
Nevertheless, it must be an async operation. A possible name could be
createStatementAsync(). Another important method of the statement is
finalize(), which is the only reliable way to free the database from JS.

B) In the view of createStatementAsync(), Parameters callback receives
additional task of storing the statement.

C) Finally, ResultSet behavior needs some clarification. From how it is
used in the example, it may be inferred, that getNextResult() is
synchronous. That means, all data needs to be fetched and saved locally
before handleResult() is invoked. A number of questions arise:

* When working the large amounts of data (like browsing history), this
may result is excessive memory consumption.

* Although the data is fetched row by row, nothing will be displayed,
until the last row is fetched.

* If there are errors in the end of a large list, nothing will be
displayed at all.

Proposed solution is to have API for both paths of execution:
1. mozIStorageConnection.executeSimpleAsync(in AUTF8String aSQL,
[optional] in Parameters aParameters, [optional] in Callback aCallback);

2. mozIStorageConnection.createStatementAsync(in in AUTF8String aSQL,
[optional] in Parameters aParameters), then
    mozIStorageStatement.executeAsync(in Callback aCallback), then
    mozIStorageValueArray.fetchNextAsync(in Callback aCallback) for each
row.

D) Callback needs to have separate methods handleAllResults/handleResult
respectively to handle 1. and 2.

--
Sergey Yanovich

This is true for very light-weight SELECT statements, to be clear, and
it only matters if you're running them many times.

No -- it can't really block, and the additional complexity of using it
will be a huge developer cost for no gain.

Yes.

:)

Mike