Bug 84652 – IndexedDB: support openCursor(IDBKey)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Alec Flett]

pretty trivial - per the spec, this is equivalent to openCursor(IDBKeyRange.only(key))

[Alec Flett]

Created attachment 138834 [details]
Patch

[Alec Flett]

Created attachment 138837 [details]
Patch

[Alec Flett]

jsbell@ - mind taking a look?

Note that this patch removes support for dictionary-style parameters to openCursor() but as best as I can tell, they're currently broken for all overloaded key/keyrange APIs, (i.e. count(), get(), etc) and I can't figure out a way to get the IDL compiler generating the right code to recognize them. 

Basically the generated code from the IDL only allows/detects dictionary iteration , so I while I know that openCursor({range: keyRange}) is now broken with this, I *believe*  that get({key:...}) might work. 

Further, I see no reference to this kind of support in the spec, nor can I find any documentation that we *should* support this. 

But in either case we're inconsistent enough for me to believe that it's ok to break this right now and file a new bug if necessary.

[Joshua Bell]

Comment on attachment 138837 [details]
Patch

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

Overall lgtm, with nits.

> Source/WebCore/ChangeLog:10
> +        Add signatures for openCursor/openKeyCursor(IDBKey).

This should be above the Test: line

> LayoutTests/storage/indexeddb/resources/opencursor-key.js:64
> +    request.onsuccess = testAll;

Change this to transaction.oncomplete = testAll, (and add earlier transaction = request.result, before request is overridden). It will soon be an error to create a transaction within the setVersion's callback.

> LayoutTests/storage/indexeddb/resources/opencursor-key.js:98
> +      testIndex();

Another option is to register this with trans.oncomplete = testIndex; e.g. at the very end of the testObjectStore() function so the chaining logic is clearer, and replace this line with e.g. // Allow transaction to complete. I find that clearer, YMMV.

> LayoutTests/storage/indexeddb/resources/opencursor-key.js:105
> +

Delete empty line.

> LayoutTests/storage/indexeddb/resources/opencursor-key.js:146
> +

Delete empty line.

> LayoutTests/storage/indexeddb/resources/opencursor-key.js:181
> +

Delete empty line,

> LayoutTests/storage/indexeddb/resources/opencursor-key.js:183
> +

Delete empty line.

[Alec Flett]

Created attachment 138855 [details]
Patch

[Alec Flett]

Comment on attachment 138837 [details]
Patch

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

>> LayoutTests/storage/indexeddb/resources/opencursor-key.js:64
>> +    request.onsuccess = testAll;
> 
> Change this to transaction.oncomplete = testAll, (and add earlier transaction = request.result, before request is overridden). It will soon be an error to create a transaction within the setVersion's callback.

agreed, and cleaned up the other tests below to use the oncomplete stuff. Much cleaner.

[Alec Flett]

ojan@ - r? cq?

[WebKit Review Bot]

Comment on attachment 138855 [details]
Patch

Clearing flags on attachment: 138855

Committed r115255: <http://trac.webkit.org/changeset/115255>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.