IndexedDB: IDBObjectStore.count() and IDBIndex.count() should accept key argument
Created attachment 128607 [details] Patch
tony@ - review at your leisure?
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?
(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.)
(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!
Created attachment 128756 [details] Patch
tony@ - I uploaded again instead of using land-safely. Can you re: r+ ?
Comment on attachment 128756 [details] Patch Clearing flags on attachment: 128756 Committed r108852: <http://trac.webkit.org/changeset/108852>
All reviewed patches have been landed. Closing bug.