Bug 83638

Summary: IndexedDB: Support get/getKey(keyRange)
Product: WebKit Reporter: Alec Flett <alecflett>
Component: WebKit Misc.Assignee: Alec Flett <alecflett>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, jsbell, levin+threading, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 83637    
Bug Blocks: 84285    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ec2-cr-linux-02
none
Patch none

Description Alec Flett 2012-04-10 16:51:43 PDT
Need to implement this in WebCore
Comment 1 Alec Flett 2012-04-12 12:40:09 PDT
Created attachment 136954 [details]
Patch
Comment 2 Alec Flett 2012-04-13 11:12:53 PDT
jsbell@ - quick review of webkit side of this?
Comment 3 Joshua Bell 2012-04-13 13:49:27 PDT
Comment on attachment 136954 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        IndexedDB: Support get/getKey(keyRange)

Is there a separate bug for IDBIndex.get/getKey? If so, change the summary of this bug to IDBObjectStore.get(keyRange)

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:81
> +PassRefPtr<IDBRequest> IDBObjectStore::get(ScriptExecutionContext* context, PassRefPtr<IDBKeyRange> keyRange, ExceptionCode& ec)

Need to check for !keyRange (see IDBObjectStore::deleteFunction), I believe.

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:105
> +    return get(context, keyRange.release(), ec);

If the Chromium port is not updated, this will fail there because the IPC plumbing for the resulting call to IDBObjectStoreBackendInterface::get(IDBKeyRange) is not in place.

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:89
> +    RefPtr<IDBKeyRange> keyRange = IDBKeyRange::only(prpKey, ec);

Add a FIXME comment indicating that this can be removed once all ports are updated to call the IDBKeyRange overload.

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:108
> +    RefPtr<IDBBackingStore::ObjectStoreRecordIdentifier> recordIdentifier;

This is not used, and can be removed.

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:117
> +            if (!wireData.isNull())

Could be combined into while() clause, i.e. while (wireData.isNull() && backingStoreCursor->continueFunction(0));

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendInterface.h:55
>      virtual void get(PassRefPtr<IDBKey>, PassRefPtr<IDBCallbacks>, IDBTransactionBackendInterface*, ExceptionCode&) = 0;

Maybe add FIXME here noting that the get(IDBKey) overload can be removed once all ports are updated

> LayoutTests/storage/indexeddb/get-keyrange-expected.txt:27
> +objectStore.get(IDBKeyRange.only(3))

Please add a test with get(null)

> LayoutTests/storage/indexeddb/objectStore-required-arguments-expected.txt:16
> +PASS objectStore.get(); threw exception TypeError: Type error.

FYI, known issue when adding method overloads - I just ran into this as well. (I should file a bug against the code generator.)
Comment 4 Alec Flett 2012-04-16 12:24:23 PDT
Created attachment 137376 [details]
Patch
Comment 5 Alec Flett 2012-04-16 12:35:54 PDT
hold off. I'm just going to implement IDBIndex.get/getKey in this too.
Comment 6 Alec Flett 2012-04-18 14:12:03 PDT
Created attachment 137765 [details]
Patch
Comment 7 Alec Flett 2012-04-18 14:18:37 PDT
jsbell@ - ok this one is ready to go - the fully unified post-chromium patch.

Notes:
- after this I will do code cleanup - removing IDBIndexBackendInterface::get(IDBKey...) and IDBObjectStoreBackendInterface::get(IDBKey...)
- since we're now using the IDBKeyRange-based get() in the backend, this may seem like we're going to take a minor perf hit, but I'd argue that if it does end up being a perf hit, that we should be optimizing IDB*BackendImpl::get(IDBKeyRange...) to recognize IDBKeyRange.only as a special case, and optimize that, which would give consumers a similar optimization if they were also using IDBKeyRange.only (and we could probably implement that optimization all the way down in the cursor, so everyone would benefit)
Comment 8 Alec Flett 2012-04-18 14:20:10 PDT
*** Bug 84256 has been marked as a duplicate of this bug. ***
Comment 9 Joshua Bell 2012-04-18 14:50:55 PDT
Comment on attachment 137765 [details]
Patch

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

Try and keep the overloads in consistent ordering (i.e. always key/keyrange, or always keyrange/key) in IDLs, headers and implementations.

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:87
> +// the IDBKeyRange version.

Although we don't do it consistently, it's becoming common practice to file a bug and reference it from the FIXME comment.

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:132
> +

Delete this blank line.

> LayoutTests/storage/indexeddb/get-keyrange-expected.txt:15
> +objectStore.createIndex('someIndex', 'x')

This only tests the IDBObjectStore.get() not IDBIndex.get() - don't create the index, or add tests for it.

> LayoutTests/storage/indexeddb/get-keyrange-expected.txt:31
> +objectStore.get(IDBKeyRange.lowerBound(5, true))

Should have an open upper bound test as well.

> LayoutTests/storage/indexeddb/resources/get-keyrange.js:54
> +    request = evalAndLog("objectStore.get(IDBKeyRange.only(3))");

Given how similar the bodies of these tests are, consider factoring out an async test runner function - see resources/index-count.js for an example.
Comment 10 Alec Flett 2012-04-18 17:05:54 PDT
Created attachment 137806 [details]
Patch
Comment 11 Alec Flett 2012-04-18 17:07:06 PDT
jsbell@ - all comments addressed, but I didn't do the generic async tester, mainly because the addition of support for the index (with both get() and getKey()) made things "meta" enough that I didn't want to add another layer of abstraction
Comment 12 Joshua Bell 2012-04-18 17:12:39 PDT
Comment on attachment 137806 [details]
Patch

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

> LayoutTests/storage/indexeddb/resources/get-keyrange.js:53
> +function runObjStoreTests() {

Inconsistent { placement (put it on its own line for a top-level function)

> LayoutTests/storage/indexeddb/resources/get-keyrange.js:54
> +  getRangeOnlyTest("objectStore", "get", ".x", "runIndexStoreTests()");

Should be indented 4 spaces.

Ditto for next few methods.

> LayoutTests/storage/indexeddb/resources/get-keyrange.js:173
> +  evalAndExpectException(store + "." + method + "(null)", "IDBDatabaseException.DATA_ERR");

Inconsistent indentation. Should be 4 spaces total.
Comment 13 Alec Flett 2012-04-18 17:26:23 PDT
Created attachment 137808 [details]
Patch
Comment 14 WebKit Review Bot 2012-04-18 19:30:33 PDT
Comment on attachment 137808 [details]
Patch

Attachment 137808 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12297173

New failing tests:
storage/indexeddb/get-keyrange.html
Comment 15 WebKit Review Bot 2012-04-18 19:30:42 PDT
Created attachment 137819 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 16 Alec Flett 2012-04-19 10:10:32 PDT
Created attachment 137914 [details]
Patch
Comment 17 Alec Flett 2012-04-19 10:12:53 PDT
Comment on attachment 137914 [details]
Patch

ok, this is ready to go, finally. dglazkov@ - r? cq?
Comment 18 WebKit Review Bot 2012-04-20 17:17:04 PDT
Comment on attachment 137914 [details]
Patch

Clearing flags on attachment: 137914

Committed r114805: <http://trac.webkit.org/changeset/114805>
Comment 19 WebKit Review Bot 2012-04-20 17:17:18 PDT
All reviewed patches have been landed.  Closing bug.