IndexedDB: Replace IDL operation overloading with ScriptValue handling

IndexedDB: Replace IDL operation overloading with ScriptValue handling

The IDB spec has many IDL methods that take |any key| and in prose
have "if key is not a valid key or IDBKeyRange, throw; otherwise ...";
these ended up being implemented incrementally in Blink using
overloads in the IDL and .h/.cpp, and counting on nonstandard overload
handling in the code generator.

Update the Blink IDLs to mostly match the spec IDLs, and do the
ScriptValue to IDBKey or IDBKeyRange conversion via explicit calls to
a binding helper. This simplifies the IDL and implementations, and
will make future additions easier.

No behavior changes.

R=haraken@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156716

[jsbell, 2013-08-23 21:48:25]

Motivation: I was going to add IDBObjectStore.openKeyCursor() that we've
discussed w/ Moz, and the overloading made me cry.

haraken@ - please take a look?

[haraken, 2013-08-25 23:51:06]

Sorry for being late. LGTM with comments.

https://codereview.chromium.org/22893058/diff/10001/Source/bindings/v8/IDBBin...
File Source/bindings/v8/IDBBindingUtilities.cpp (right):

https://codereview.chromium.org/22893058/diff/10001/Source/bindings/v8/IDBBin...
Source/bindings/v8/IDBBindingUtilities.cpp:338:
ASSERT(v8::Context::InContext());

Why do you want to have this check? Given that the KeyRange conversion should be
done in the state->context(), Context::InContext() looks irrelevant.

https://codereview.chromium.org/22893058/diff/10001/Source/modules/indexeddb/...
File Source/modules/indexeddb/IDBIndex.cpp (right):

https://codereview.chromium.org/22893058/diff/10001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBIndex.cpp:80: return 0;

Don't you need to check if(!keyRange)?

https://codereview.chromium.org/22893058/diff/10001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBIndex.cpp:111: return 0;

Ditto.

https://codereview.chromium.org/22893058/diff/10001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBIndex.cpp:139: return 0;

Ditto.

https://codereview.chromium.org/22893058/diff/10001/Source/modules/indexeddb/...
File Source/modules/indexeddb/IDBObjectStore.cpp (left):

https://codereview.chromium.org/22893058/diff/10001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBObjectStore.cpp:434: return 0;

Why can you remove these checks?

https://codereview.chromium.org/22893058/diff/10001/Source/modules/indexeddb/...
File Source/modules/indexeddb/IDBObjectStore.cpp (right):

https://codereview.chromium.org/22893058/diff/10001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBObjectStore.cpp:529: 

Don't you need to check if(!keyRange) ?

https://codereview.chromium.org/22893058/diff/10001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBObjectStore.cpp:561: 

Shall we add ASSERT(keyRange) ?

[jsbell, 2013-08-26 16:06:17]

https://codereview.chromium.org/22893058/diff/10001/Source/bindings/v8/IDBBin...
File Source/bindings/v8/IDBBindingUtilities.cpp (right):

https://codereview.chromium.org/22893058/diff/10001/Source/bindings/v8/IDBBin...
Source/bindings/v8/IDBBindingUtilities.cpp:338:
ASSERT(v8::Context::InContext());
On 2013/08/25 23:51:06, haraken wrote:
> 
> Why do you want to have this check? Given that the KeyRange conversion should
be
> done in the state->context(), Context::InContext() looks irrelevant.

Purely copy/paste from the above methods. I'll remove it.

https://codereview.chromium.org/22893058/diff/10001/Source/modules/indexeddb/...
File Source/modules/indexeddb/IDBIndex.cpp (right):

https://codereview.chromium.org/22893058/diff/10001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBIndex.cpp:80: return 0;
On 2013/08/25 23:51:06, haraken wrote:
> 
> Don't you need to check if(!keyRange)?

No - null keyrange is acceptable, and this is not a change from previous
behavior. It means "range over all possible keys" which is not expressible with
a range itself, and is what happens if you call from script leaving off the
optional argument i.e. index.openCursor()

It should be checked in places where the argument is non-optional, however.

https://codereview.chromium.org/22893058/diff/10001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBIndex.cpp:111: return 0;
On 2013/08/25 23:51:06, haraken wrote:
> 
> Ditto.

Ditto. (range is optional for count())

https://codereview.chromium.org/22893058/diff/10001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBIndex.cpp:139: return 0;
On 2013/08/25 23:51:06, haraken wrote:
> 
> Ditto.

Ditto. (range is optional for openKeyCursor)

https://codereview.chromium.org/22893058/diff/10001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBIndex.cpp:166: if (!keyRange) {
A key or range is NOT optional for get(), hence the check here.

https://codereview.chromium.org/22893058/diff/10001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBIndex.cpp:195: if (!keyRange) {
Ditto for getKey()

https://codereview.chromium.org/22893058/diff/10001/Source/modules/indexeddb/...
File Source/modules/indexeddb/IDBObjectStore.cpp (left):

https://codereview.chromium.org/22893058/diff/10001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBObjectStore.cpp:434: return 0;
On 2013/08/25 23:51:06, haraken wrote:
> 
> Why can you remove these checks?

This change switches from calling a script-exposed method to an "internal"
method. The new "internal" openCursor() method is used from all call sites after
the checks, and doesn't take an ExceptionState.

The script-exposed openCursor() checks that the index was in a valid state for
operations (duplicating the checks at the top of this method) and that the
direction string is valid (but now the enum value is passed).

https://codereview.chromium.org/22893058/diff/10001/Source/modules/indexeddb/...
File Source/modules/indexeddb/IDBObjectStore.cpp (right):

https://codereview.chromium.org/22893058/diff/10001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBObjectStore.cpp:419: ASSERT(!es.hadException());
Not relevant to this patch, but I have no idea what this ASSERT is doing here. I
don't see a path leading to this line where an exception may be thrown without
returning.

https://codereview.chromium.org/22893058/diff/10001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBObjectStore.cpp:529: 
On 2013/08/25 23:51:06, haraken wrote:
> 
> Don't you need to check if(!keyRange) ?

openCursor() can take a null range.

https://codereview.chromium.org/22893058/diff/10001/Source/modules/indexeddb/...
Source/modules/indexeddb/IDBObjectStore.cpp:561: 
On 2013/08/25 23:51:06, haraken wrote:
> 
> Shall we add ASSERT(keyRange) ?

count() can take a null range

[commit-bot: I haz the power, 2013-08-26 16:51:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jsbell@chromium.org/22893058/17001

[commit-bot: I haz the power, 2013-08-26 18:40:26]

Change committed as 156716