Shawn Wilsher :sdwilsh Assignee
	Description • 15 years ago

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.

	Shawn Wilsher :sdwilsh Assignee
	Comment 1 • 15 years ago

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...

	Shawn Wilsher :sdwilsh Assignee
	Comment 2 • 15 years ago

This one actually passes the tests that I mentioned in the pass.  I added one more test since the last iteration as well.

	Shawn Wilsher :sdwilsh Assignee
	Comment 3 • 15 years ago

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.

	Shawn Wilsher :sdwilsh Assignee
	Comment 4 • 15 years ago

Comment on attachment 375240 [details] [diff] [review]
v1.0

This is a new API, so lets get sr on this as well.

	Andrew Sutherland [:asuth] (he/him)
	Comment 5 • 15 years ago

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.)

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 6 • 15 years ago

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.

	Shawn Wilsher :sdwilsh Assignee
	Comment 7 • 15 years ago

(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.

	Shawn Wilsher :sdwilsh Assignee
	Comment 8 • 15 years ago

For checkin.

	Shawn Wilsher :sdwilsh Assignee
	Comment 9 • 15 years ago

http://hg.mozilla.org/mozilla-central/rev/d546bd2c46a4

	Shawn Wilsher :sdwilsh Assignee
	Comment 10 • 15 years ago

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.

	Shawn Wilsher :sdwilsh Assignee
	Comment 11 • 15 years ago

For my own reference, we a do-while on hasResults in executeAndProcessStatement.

	Shawn Wilsher :sdwilsh Assignee
	Comment 12 • 15 years ago

bug 488148 was actually the cause of the failure.

	Shawn Wilsher :sdwilsh Assignee
	Comment 13 • 15 years ago

http://hg.mozilla.org/mozilla-central/rev/e6b53112a17c

	Eric Shepherd [:sheppy]
	Comment 14 • 15 years ago

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

	Shawn Wilsher :sdwilsh Assignee
	Comment 15 • 15 years ago

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?

	Eric Shepherd [:sheppy]
	Comment 16 • 15 years ago

Looks snazzy - thank you!