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
Alec Flett
2012-04-10 16:51:43 PDT
Created attachment 136954 [details]
Patch
jsbell@ - quick review of webkit side of this? 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.) Created attachment 137376 [details]
Patch
hold off. I'm just going to implement IDBIndex.get/getKey in this too. Created attachment 137765 [details]
Patch
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) *** Bug 84256 has been marked as a duplicate of this bug. *** 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. Created attachment 137806 [details]
Patch
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 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. Created attachment 137808 [details]
Patch
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 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
Created attachment 137914 [details]
Patch
Comment on attachment 137914 [details]
Patch
ok, this is ready to go, finally. dglazkov@ - r? cq?
Comment on attachment 137914 [details] Patch Clearing flags on attachment: 137914 Committed r114805: <http://trac.webkit.org/changeset/114805> All reviewed patches have been landed. Closing bug. |