Last Comment Bug 490085 - Add ability to bind multiple sets of parameters and execute asynchronously
: Add ability to bind multiple sets of parameters and execute asynchronously
Status: RESOLVED FIXED
[doc-waiting-feedback]
: dev-doc-complete
Product: Toolkit
Classification: Components
Component: Storage (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9.2a1
Assigned To: Shawn Wilsher :sdwilsh
:
:
Mentors:
Depends on: 490083 490084 490833 499271
Blocks: 506022
  Show dependency treegraph
 
Reported: 2009-04-24 17:17 PDT by Shawn Wilsher :sdwilsh
Modified: 2009-10-29 07:29 PDT (History)
3 users (show)
sdwilsh: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v0.1 (54.43 KB, patch)
2009-04-24 17:17 PDT, Shawn Wilsher :sdwilsh
no flags Details | Diff | Splinter Review
v0.2 (66.54 KB, patch)
2009-04-30 10:59 PDT, Shawn Wilsher :sdwilsh
no flags Details | Diff | Splinter Review
v1.0 (75.40 KB, patch)
2009-04-30 15:33 PDT, Shawn Wilsher :sdwilsh
bugmail: review+
vladimir: superreview+
Details | Diff | Splinter Review
v1.1 (75.79 KB, patch)
2009-05-26 16:42 PDT, Shawn Wilsher :sdwilsh
no flags Details | Diff | Splinter Review

Description Shawn Wilsher :sdwilsh 2009-04-24 17:17:22 PDT
Created attachment 374555 [details] [diff] [review]
v0.1

As discussed in the newsgroups, we need a way to bind multiple parameters to one statement, and execute that statement asynchronously.  The attached patch does this, but I need to write a bunch more tests before I'm confident with this implementation.
Comment 1 Shawn Wilsher :sdwilsh 2009-04-30 10:51:33 PDT
Trying to debug test_statement_executeAsync.js has led to a major refactoring of it.  It's for the better, but it's a bit of code churn in the test...
Comment 2 Shawn Wilsher :sdwilsh 2009-04-30 10:59:55 PDT
Created attachment 375201 [details] [diff] [review]
v0.2

This one actually passes the tests that I mentioned in the pass.  I added one more test since the last iteration as well.
Comment 3 Shawn Wilsher :sdwilsh 2009-04-30 15:33:11 PDT
Created attachment 375240 [details] [diff] [review]
v1.0

I think the only thing I'm not testing here is the noscript methods.  I'm not sure I care, given that the code is very straightforward, and calls into the generic methods anyway.

My public patch queue has this patch updated as well.
Comment 4 Shawn Wilsher :sdwilsh 2009-04-30 15:41:00 PDT
Comment on attachment 375240 [details] [diff] [review]
v1.0

This is a new API, so lets get sr on this as well.
Comment 5 Andrew Sutherland [:asuth] 2009-05-11 08:14:05 PDT
Comment on attachment 375240 [details] [diff] [review]
v1.0

The code is beautiful and the extensive unit tests are awesome.  Although the choice of "itr" as an iterator name is quite the juxtaposition to the beauty :)

(Pulled the current patch from your patch repo along with new-async-api and the needs-revision patch on bug 490833.  Tests ran fine.)
Comment 6 Vladimir Vukicevic [:vlad] [:vladv] 2009-05-18 11:13:11 PDT
Comment on attachment 375240 [details] [diff] [review]
v1.0

>    // Bind the data to our statement.
>    nsCOMPtr<mozIStorageError> error((*itr)->bind(stmt));
>    if (error) {

Just as a point of style, I really dislike when important things are done as function arguments; the important action here is the binding, not the creating of the error object.  So I'd split these up (probably with the extra blank lines so that the Important Bit doesn't get lost, especially given that it doesn't stand out:

    nsCOMPtr<mozIStorageError> error;

    error = (*itr)->bind(stmt);

    if (error) {


In BindByIndex/BindByName:
>   NS_ENSURE_FALSE(mLocked, NS_ERROR_UNEXPECTED);

Given that lock() is an explicit method here, this probably shouldn't be a NS_ENSURE_FALSE -- that'll actually spit out an error in debug builds, and I don't think that should happen since it's not really an assertion type thing.  It should be an error, but probably something explicit like NS_ERROR_STORAGE_BINDING_PARAMS_LOCKED instead of just unexpected (though NS_ERROR_UNEXPECTED is probably ok).

>   * Locks the array and prevents further modification to it (such as adding
>   * more elements to it).
>   */
>
>   void lock();
>
>   BindingParamsArray(Statement *aOwningStatement);

Another style nit (and I'm only being this nitpicky about style because Storage overall has pretty good coding style already) -- this constructor decl is in a really odd place in the header file -- at the very least put it above lock() (and lock's docs), or maybe right after the NS_DECL_* blocks at the top.  Maybe even move the iterator class so that it's defined just before begin(), and move getOwner() to be after lock(), to keep the iterator "block" all together.

Other than that, everything looks great.
Comment 7 Shawn Wilsher :sdwilsh 2009-05-26 16:37:55 PDT
(In reply to comment #6)
> Given that lock() is an explicit method here, this probably shouldn't be a
> NS_ENSURE_FALSE -- that'll actually spit out an error in debug builds, and I
> don't think that should happen since it's not really an assertion type thing. 
> It should be an error, but probably something explicit like
> NS_ERROR_STORAGE_BINDING_PARAMS_LOCKED instead of just unexpected (though
> NS_ERROR_UNEXPECTED is probably ok).
I'm not sure I agree with you actually.  They are misusing the API by calling this at this point, and I'd want to know it.  In general, we are pretty bad about giving specifics of the mistake in storage, so throwing NS_ERROR_UNEXPECTED is nothing new.  I think that if someone is using this API wrong, we should make it obvious in a debug build (there is no use case when it'd be legal to call this, so they'd have a real bug in their code).

Everything else has been fixed.
Comment 8 Shawn Wilsher :sdwilsh 2009-05-26 16:42:00 PDT
Created attachment 379777 [details] [diff] [review]
v1.1

For checkin.
Comment 9 Shawn Wilsher :sdwilsh 2009-06-17 12:18:25 PDT
http://hg.mozilla.org/mozilla-central/rev/d546bd2c46a4
Comment 10 Shawn Wilsher :sdwilsh 2009-06-17 13:47:16 PDT
backed this out because we only return one row in the old case even if you have more than one row.  Oops.

I'll attach a patch that has a test for this, and the fix.
Comment 11 Shawn Wilsher :sdwilsh 2009-06-17 13:54:14 PDT
For my own reference, we a do-while on hasResults in executeAndProcessStatement.
Comment 12 Shawn Wilsher :sdwilsh 2009-06-23 14:10:42 PDT
bug 488148 was actually the cause of the failure.
Comment 13 Shawn Wilsher :sdwilsh 2009-06-24 10:20:08 PDT
http://hg.mozilla.org/mozilla-central/rev/e6b53112a17c
Comment 14 Eric Shepherd [:sheppy] 2009-10-28 12:32:21 PDT
I've added initial documentation to MDC, and would appreciate a review. Also, do we have a good, readable code sample somewhere? The tests are a little convoluted and contrived (which is usually, by necessity, the case).

The documentation is here:

https://developer.mozilla.org/en/Storage#Binding_multiple_parameters
https://developer.mozilla.org/en/mozIStorageBindingParams
https://developer.mozilla.org/en/mozIStorageBindingParamsArray

And the first two methods here:

https://developer.mozilla.org/en/mozIStorageStatement
Comment 15 Shawn Wilsher :sdwilsh 2009-10-28 15:43:16 PDT
I've modified https://developer.mozilla.org/en/Storage#Binding_Parameters making it a bit more structured and beefed up the example code.  Also clarified some points that were wrong.

Updated https://developer.mozilla.org/en/mozIStorageBindingParams to use the storage templates so all the wording is consistent throughout all the documents (feel free to tweak those templates).

Look good?
Comment 16 Eric Shepherd [:sheppy] 2009-10-29 07:29:17 PDT
Looks snazzy - thank you!

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