Bug 79422 – IndexedDB: IDBObjectStore.count() and IDBIndex.count() should accept key argument

• Component: New Bugs
• Status: RESOLVED FIXED

[Joshua Bell]

IndexedDB: IDBObjectStore.count() and IDBIndex.count() should accept key argument

[Joshua Bell]

Created attachment 128607 [details]
Patch

[Joshua Bell]

tony@ - review at your leisure?

[Tony Chang]

Comment on attachment 128607 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128607&action=review

> Source/WebCore/storage/IDBIndex.idl:48
>          [CallWith=ScriptExecutionContext] IDBRequest count(in [Optional] IDBKeyRange range)
>              raises (IDBDatabaseException);
> +        [CallWith=ScriptExecutionContext] IDBRequest count(in IDBKey key)
> +            raises (IDBDatabaseException);

The spec defines overloaded methods?  That seems unfortunate if both can take null.

> LayoutTests/storage/indexeddb/objectstore-count.html:128
> +    evalAndExpectException("store.count(NaN)", "IDBDatabaseException.DATA_ERR");
> +    evalAndExpectException("store.count({})", "IDBDatabaseException.DATA_ERR");
> +    evalAndExpectException("store.count(/regex/)", "IDBDatabaseException.DATA_ERR");

Can you add a test that passes in null?  Maybe that's elsewhere?

[Joshua Bell]

(In reply to comment #3)
> (From update of attachment 128607 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128607&action=review
> 
> > Source/WebCore/storage/IDBIndex.idl:48
> >          [CallWith=ScriptExecutionContext] IDBRequest count(in [Optional] IDBKeyRange range)
> >              raises (IDBDatabaseException);
> > +        [CallWith=ScriptExecutionContext] IDBRequest count(in IDBKey key)
> > +            raises (IDBDatabaseException);
> 
> The spec defines overloaded methods?  That seems unfortunate if both can take null.

The spec actually defines it (and several other methods) as taking an "optional any", and then in the prose defines behavior for no argument, an argument of type IDBKeyRange, an argument of another type which is a valid key, and otherwise.

WebKit's IDB implementation pushes this into the binding layer, and introduces an IDBKey type which is like a "restricted any" (it's either valid key data of type string/number/date/array, or "invalid"). For added fun, WebKit also defines an IDBAny which is more like a variant type, but it is only used for return values.

It may not be too difficult to make IDBAny work as an argument type, with just slightly more custom binding logic in WebCore/binding/v8/IDBBindingUtilities.cpp. The IDL folks (both in WebKit and spec writers at Mozilla) have been pushing for more expressive IDL/less custom binding, so I'm not sure that's a win, though.

Thoughts?

> > LayoutTests/storage/indexeddb/objectstore-count.html:128
> > +    evalAndExpectException("store.count(NaN)", "IDBDatabaseException.DATA_ERR");
> > +    evalAndExpectException("store.count({})", "IDBDatabaseException.DATA_ERR");
> > +    evalAndExpectException("store.count(/regex/)", "IDBDatabaseException.DATA_ERR");
> 
> Can you add a test that passes in null?  Maybe that's elsewhere?

I'm pretty sure the generated binding code treats a null argument as matching [optional] as though there was no argument, but I'll add tests to verify. I'm unsure what the WebIDL spec says re: calling with no argument vs. calling with an argument of null or undefined. (Browsers tend to be pretty inconsistent here.)

[Tony Chang]

(In reply to comment #4)
> It may not be too difficult to make IDBAny work as an argument type, with just slightly more custom binding logic in WebCore/binding/v8/IDBBindingUtilities.cpp. The IDL folks (both in WebKit and spec writers at Mozilla) have been pushing for more expressive IDL/less custom binding, so I'm not sure that's a win, though.
> 
> Thoughts?

The current implementation and spec seem fine.  It seems pretty clear from the code what is happening.

> > Can you add a test that passes in null?  Maybe that's elsewhere?
> 
> I'm pretty sure the generated binding code treats a null argument as matching [optional] as though there was no argument, but I'll add tests to verify. I'm unsure what the WebIDL spec says re: calling with no argument vs. calling with an argument of null or undefined. (Browsers tend to be pretty inconsistent here.)

Adding tests sound great, if for no other reason than it provides regression testing.

Thanks!

[Joshua Bell]

Created attachment 128756 [details]
Patch

[Joshua Bell]

tony@ - I uploaded again instead of using land-safely. Can you re: r+ ?

[WebKit Review Bot]

Comment on attachment 128756 [details]
Patch

Clearing flags on attachment: 128756

Committed r108852: <http://trac.webkit.org/changeset/108852>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.