Bug 83638 – IndexedDB: Support get/getKey(keyRange)

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Alec Flett]

Need to implement this in WebCore

[Alec Flett]

Created attachment 136954 [details]
Patch

[Alec Flett]

jsbell@ - quick review of webkit side of this?

[Joshua Bell]

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.)

[Alec Flett]

Created attachment 137376 [details]
Patch

[Alec Flett]

hold off. I'm just going to implement IDBIndex.get/getKey in this too.

[Alec Flett]

Created attachment 137765 [details]
Patch

[Alec Flett]

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)

[Alec Flett]

*** Bug 84256 has been marked as a duplicate of this bug. ***

[Joshua Bell]

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.

[Alec Flett]

Created attachment 137806 [details]
Patch

[Alec Flett]

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

[Joshua Bell]

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.

[Alec Flett]

Created attachment 137808 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Alec Flett]

Created attachment 137914 [details]
Patch

[Alec Flett]

Comment on attachment 137914 [details]
Patch

ok, this is ready to go, finally. dglazkov@ - r? cq?

[WebKit Review Bot]

Comment on attachment 137914 [details]
Patch

Clearing flags on attachment: 137914

Committed r114805: <http://trac.webkit.org/changeset/114805>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.