Closed Bug 435994 Opened 12 years ago Closed 11 years ago

Implement ResultSet and Tuple for mozIStorageStatement::executeAsync

Categories

(Toolkit :: Storage, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch v1.0 (obsolete) — Splinter Review
Spun out of Bug 429986 to make it easier to review.  This implements storageIResultSet and storageITuple.
Depends on: 435995
Attached patch v1.1 (obsolete) — Splinter Review
Adds some error checking, and brought some code into this patch that was in the next patch in my queue.  I think this is ready for review.
Attachment #322709 - Attachment is obsolete: true
Attachment #322793 - Flags: superreview?(shaver)
Attachment #322793 - Flags: review?(vladimir)
This is for.. allowing better access to storage results from C++?

Also, while in general I like using more normal-looking C++, you shouldn't split the module that way; either rename everything (e.g. mozStorageValueArray -> Storage::ValueArray) or keep following the current template.  "storageIDataSet" also looks weird; maybe just IStorageDataSet, implemented by Storage::DataSet, if we're going to drop the moz.
(In reply to comment #2)
> This is for.. allowing better access to storage results from C++?
No, this is for accessing the results of a statement executed in the background (see http://developer.mozilla.org/en/docs/User_talk:Comrade693#Async_Storage_Statements)

> Also, while in general I like using more normal-looking C++, you shouldn't
> split the module that way; either rename everything (e.g. mozStorageValueArray
> -> Storage::ValueArray) or keep following the current template. 
> "storageIDataSet" also looks weird; maybe just IStorageDataSet, implemented by
> Storage::DataSet, if we're going to drop the moz.
I'd be happy to rename everything - just didn't think folks would like it if I did that.  Figured it'd be best to rename and namespace things as we came across them.  Let me know what you want me to do here.

As for storageI vs IStorage, I went with the former for a few reasons:
1) going for a [module] I [interface name] approach
2) I thought it would be more useful to have the interface name after the I.
(1) was a result of a conversation with some folks on #developers one night (who, I cannot recall however), and (2) was just intuition.  I'm not particularly tied to either one, so let me know here as well.
(In reply to comment #3)
> (In reply to comment #2)
> > This is for.. allowing better access to storage results from C++?
> No, this is for accessing the results of a statement executed in the background

Hrm, why is a tuple any different than just a valuearray?  One thing that's missing in that case is a scriptable helper, so that you can write tuple[0] or tuple['column'] in JS, right?

> I'd be happy to rename everything - just didn't think folks would like it if I
> did that.  Figured it'd be best to rename and namespace things as we came
> across them.  Let me know what you want me to do here.
> 
> As for storageI vs IStorage, I went with the former for a few reasons:
> 1) going for a [module] I [interface name] approach
> 2) I thought it would be more useful to have the interface name after the I.
> (1) was a result of a conversation with some folks on #developers one night
> (who, I cannot recall however), and (2) was just intuition.  I'm not
> particularly tied to either one, so let me know here as well.

We should probably figure this out with a bit wider audience; "storageI" is kind of a mouthful, and doesn't read all that cleanly ("Storage I Tuple"; not a problem with "N S I" or "MOZ I ...").  I don't have any good suggestions though, but I think we should decide what to do in general instead of doing different things per module.  XPCOM interface names are probably the biggest problems here.. I'd almost rather just keep them with a nsI or mozI prefix and avoid the problem entirely.

The other comment that I'd have here is that the namespace should probably be mozStorage, or moz::Storage; if we're going to drop the 'moz' prefix somehow, we shouldn't be polluting the global namespace.
(In reply to comment #4)
> Hrm, why is a tuple any different than just a valuearray?  One thing that's
> missing in that case is a scriptable helper, so that you can write tuple[0] or
> tuple['column'] in JS, right?
tuple is just meant to extend valuearray to provide name based access to columns at the minimum.  This doesn't make sense to add to valuearray because we use valuearray's for function arguments as well as results, and name-based function arguments don't make sense.  I found it to be nonsensical to add this support when I was looking into bug 387945.  The tuple interface is what I'd like to see our normal statement execution path to return as well, but that's too big of a change for 1.9.1 (I think).

As for the JS wrapper - I agree that we should be able to access properties like that.  With that said, I don't think we should block our 1.9.1 work on that since it's not terrible now because we return an nsIVariant which gets converted to the right js object automagically.  Short answer here is that I'd like to do this in a follow-up once this async stuff is done.  I also don't think we should make it an explicit wrapper - it should just work without the wrapper in my opinion.

> We should probably figure this out with a bit wider audience; "storageI" is
> kind of a mouthful, and doesn't read all that cleanly ("Storage I Tuple"; not a
> problem with "N S I" or "MOZ I ...").  I don't have any good suggestions
> though, but I think we should decide what to do in general instead of doing
> different things per module.  XPCOM interface names are probably the biggest
> problems here.. I'd almost rather just keep them with a nsI or mozI prefix and
> avoid the problem entirely.
I'm not particularly attached to anything,  but I've never cared for ns because we aren't netscape anymore, and moz just hasn't seemed right.  Where do you think is a good place to bring this up at - .planning or .platform, or elsewhere?

> The other comment that I'd have here is that the namespace should probably be
> mozStorage, or moz::Storage; if we're going to drop the 'moz' prefix somehow,
> we shouldn't be polluting the global namespace.
Does this matter?  Would we export that information even?
With that said, I did some exploring with google's code search and it seems other projects do one of the following:
1) just use generic names for their code
2) have a namespace for their own code, then a generic namespace
3) have a generic name prefixed.
There wasn't an overwhelming trend though.  This should probably be brought up in the newsgroups too.
(In reply to comment #4)
Short of the naming concerns, is there anything else you need addressed before doing your review?
Comment on attachment 322793 [details] [diff] [review]
v1.1

Clearing review until the naming issue is resolved; I don't really like the piecemeal naming change, it's just another thing on which new developers are likely to stub their toe.  Consistency trumps purity here, especially when the purity is unclear itself.
Attachment #322793 - Flags: superreview?(shaver)
Attachment #322793 - Flags: review?(vladimir)
Attached patch v1.2 (obsolete) — Splinter Review
Addresses naming concerns and any review comments
Attachment #322793 - Attachment is obsolete: true
Attachment #326806 - Flags: superreview?(shaver)
Attachment #326806 - Flags: review?(vladimir)
Attachment #326806 - Attachment is patch: true
Attachment #326806 - Attachment mime type: application/octet-stream → text/plain
Whiteboard: [has patch][needs review vlad][needs sr shaver]
Comment on attachment 326806 [details] [diff] [review]
v1.2

Looks fine, though this begs for some scriptable helpers instead of getResultByIndex and getResultByName.  row[0] and row['name']... should file a separate bug to do this.
Attachment #326806 - Flags: review?(vladimir) → review+
I think we'll want to mimic the HTML 5 spec a bit more too here - they provide a length and a randomaccess getter.  I don't presently see why we wouldn't want to do that - although I'm also not sold on the use case.  I don't think named rows make sense though ;)
Whiteboard: [has patch][needs review vlad][needs sr shaver] → [has patch][has review][needs sr shaver]
Comment on attachment 326806 [details] [diff] [review]
v1.2

sr=shaver
Attachment #326806 - Flags: superreview?(shaver) → superreview+
Whiteboard: [has patch][has review][needs sr shaver] → [has patch][has review][has sr]
Attached patch v1.3Splinter Review
Updated from changes to bug 435995.  Very minimal, so not asking for review.
Attachment #326806 - Attachment is obsolete: true
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/b8df7471a2e2
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [has patch][has review][has sr]
You need to log in before you can comment on or make changes to this bug.