Bug 84652 - IndexedDB: support openCursor(IDBKey)
Summary: IndexedDB: support openCursor(IDBKey)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alec Flett
URL:
Keywords: WebExposed
Depends on:
Blocks:
 
Reported: 2012-04-23 16:56 PDT by Alec Flett
Modified: 2012-06-08 14:58 PDT (History)
4 users (show)

See Also:


Attachments
Patch (29.57 KB, patch)
2012-04-25 10:16 PDT, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (29.65 KB, patch)
2012-04-25 10:26 PDT, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (30.69 KB, patch)
2012-04-25 12:30 PDT, Alec Flett
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alec Flett 2012-04-23 16:56:20 PDT
pretty trivial - per the spec, this is equivalent to openCursor(IDBKeyRange.only(key))
Comment 1 Alec Flett 2012-04-25 10:16:34 PDT
Created attachment 138834 [details]
Patch
Comment 2 Alec Flett 2012-04-25 10:26:10 PDT
Created attachment 138837 [details]
Patch
Comment 3 Alec Flett 2012-04-25 10:57:05 PDT
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.
Comment 4 Joshua Bell 2012-04-25 11:48:54 PDT
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.
Comment 5 Alec Flett 2012-04-25 12:30:25 PDT
Created attachment 138855 [details]
Patch
Comment 6 Alec Flett 2012-04-25 12:31:08 PDT
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.
Comment 7 Alec Flett 2012-04-25 12:31:46 PDT
ojan@ - r? cq?
Comment 8 WebKit Review Bot 2012-04-25 16:04:47 PDT
Comment on attachment 138855 [details]
Patch

Clearing flags on attachment: 138855

Committed r115255: <http://trac.webkit.org/changeset/115255>
Comment 9 WebKit Review Bot 2012-04-25 16:04:54 PDT
All reviewed patches have been landed.  Closing bug.