Add ability to bind multiple sets of parameters and execute asynchronously

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Toolkit
Storage
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Tracking

({dev-doc-complete})

Trunk
mozilla1.9.2a1
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [doc-waiting-feedback])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

8 years ago
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.
(Assignee)

Updated

8 years ago
Depends on: 490833
(Assignee)

Comment 1

8 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...
(Assignee)

Updated

8 years ago
Summary: Add ability to bind multiple rows of parameters and execute asynchronously → Add ability to bind multiple sets of parameters and execute asynchronously
(Assignee)

Comment 2

8 years ago
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.
Attachment #374555 - Attachment is obsolete: true
(Assignee)

Comment 3

8 years ago
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.
Attachment #375201 - Attachment is obsolete: true
Attachment #375240 - Flags: review?(bugmail)
(Assignee)

Updated

8 years ago
Whiteboard: [needs review asuth]
(Assignee)

Comment 4

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

This is a new API, so lets get sr on this as well.
Attachment #375240 - Flags: superreview?(vladimir)
(Assignee)

Updated

8 years ago
Whiteboard: [needs review asuth] → [needs review asuth][needs sr vlad]
Target Milestone: --- → mozilla1.9.2a1
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.)
Attachment #375240 - Flags: review?(bugmail) → review+
(Assignee)

Updated

8 years ago
Whiteboard: [needs review asuth][needs sr vlad] → [needs sr vlad]
Attachment #375240 - Flags: superreview?(vladimir) → superreview+
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.
(Assignee)

Updated

8 years ago
Whiteboard: [needs sr vlad] → [needs new patch]
(Assignee)

Comment 7

8 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.
Whiteboard: [needs new patch]
(Assignee)

Comment 8

8 years ago
Created attachment 379777 [details] [diff] [review]
v1.1

For checkin.
Attachment #375240 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Whiteboard: [can land]
(Assignee)

Comment 9

8 years ago
http://hg.mozilla.org/mozilla-central/rev/d546bd2c46a4
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Keywords: dev-doc-needed
Resolution: --- → FIXED
Whiteboard: [can land]
(Assignee)

Comment 10

8 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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [needs new patch]
(Assignee)

Comment 11

8 years ago
For my own reference, we a do-while on hasResults in executeAndProcessStatement.
Depends on: 499271
(Assignee)

Comment 12

8 years ago
bug 488148 was actually the cause of the failure.
Status: REOPENED → ASSIGNED
(Assignee)

Comment 13

8 years ago
http://hg.mozilla.org/mozilla-central/rev/e6b53112a17c
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
Whiteboard: [needs new patch]
(Assignee)

Updated

8 years ago
Blocks: 506022
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
Whiteboard: [doc-waiting-feedback]
(Assignee)

Comment 15

8 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?
Looks snazzy - thank you!
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.