1392554 - Port (Async)StatementParams to WebIDL

Status: RESOLVED FIXED

(Toolkit :: Storage, enhancement)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

This is a lot like bug 1390489 and the last blocker for bug 1389510.

What makes this one a bit more complicated:

(1) WebIDL requires a length property if we have an indexed getter. Unfortunately in the async case we don't know the number of parameters so I made this return UINT16_MAX.

(2) Property enumeration now includes the indexed properties and "length". I did an audit and we don't have any code in the browser relying on the property enumeration (other than test_js_helpers.js I fixed).

(3) *Getting* named and indexed properties is hard to implement because the underlying (Async)StatementParams class does not expose this functionality and it doesn't seem trivial to add. All of our code only *sets* properties on these objects and doesn't read them back (I verified this with a code audit + MOZ_CRASH on try). Unfortunately we can't throw an exception because setting these properties will also call the getter first.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8899752 [details] [diff] [review]
Patch

Review of attachment 8899752 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for this patch and the others!  This comprehensible magic is much preferable to the previous black magic, and I look forward to the JS engine wins that it sounds like will come from getting rid of this cruft!

::: storage/mozStorageAsyncStatementParams.cpp
@@ +72,4 @@
>  
> +  NS_ConvertUTF16toUTF8 name(aName);
> +
> +  // Check to see if there's a parameter with this name.

nit: This comment is not relevant here.  (It was relevant in StatementParams::NamedSetter where it was copied from though ;)

::: storage/mozStorageAsyncStatementParams.h
@@ +40,5 @@
> +  uint32_t Length() const {
> +    // WebIDL requires a .length property when there's an indexed getter.
> +    // Unfortunately we don't know how many params there are in the async case,
> +    // so we have to lie.
> +    return UINT16_MAX;

aside: I think this is fine[1], but in the event this ever caused things to explode, we could surface AsyncBindingParams::mParameters.Length().

1: As you diligently investigated, you can't get the parameters out once you put them in, nor should anyone want them back out.  It would make more sense to put the effort into being able to consume JS Arrays for binding-by-index and JS Objects/Maps for binding-by-name.

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8899752 [details] [diff] [review]
Patch

Review of attachment 8899752 [details] [diff] [review]:
-----------------------------------------------------------------

:asuth's comments pretty much mirror my own thoughts. There's some weirdness here with our getters and the length hardwiring, but since we're chrome only and the weirdness is commented, that's the best I think we can do. :bz may have followups when he gets back though.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df5b4f5fe775
Port (Async)StatementParams to WebIDL bindings. r=asuth,qdot

Comment 4

[Jan de Mooij [:jandem]]

Thanks for the reviews! Yeah, WebIDL is not a great fit for these objects IMO.

(In reply to Andrew Sutherland [:asuth] from comment #1)
> It would make more sense
> to put the effort into being able to consume JS Arrays for binding-by-index
> and JS Objects/Maps for binding-by-name.

Yeah, I know this code goes back decades, but if we ever redesign these APIs it would be nice to accept a plain JS object when executing the statement. Something like stmt.bindParams({x: foo, y: bar}). Then we don't need any of the proxy weirdness.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/df5b4f5fe775