Summary: | IndexedDB: Need to distinguish key paths that don't yield value vs. yield invalid key | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joshua Bell <jsbell> | ||||||||
Component: | WebCore Misc. | Assignee: | Joshua Bell <jsbell> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, dglazkov, dgrogan, fishd, tony, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 76198 | ||||||||||
Attachments: |
|
Description
Joshua Bell
2012-01-17 15:47:01 PST
Landing this will require disabling some tests and extending some enumeration-sensitive code in the Chromium port first. See http://code.google.com/p/chromium/issues/detail?id=110956 for the "final state" of the Chromium changes. (In reply to comment #1) > Landing this will require disabling some tests and extending some enumeration-sensitive code in the Chromium port first. See http://code.google.com/p/chromium/issues/detail?id=110956 for the "final state" of the Chromium changes. Er, https://chromiumcodereview.appspot.com/9212038 Created attachment 123664 [details]
Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API. Comment on attachment 123664 [details] Patch Attachment 123664 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11336244 New failing tests: storage/indexeddb/objectstore-basics.html Created attachment 123767 [details]
Patch
Once everything has passed try-bots and reviews, the plan is: 1. Land https://chromiumcodereview.appspot.com/9117040 in Chromium 2. Land https://bugs.webkit.org/show_bug.cgi?id=76487 (this) 3. Land https://chromiumcodereview.appspot.com/9212038 in Chromium The Chromium port will have some broken functionality between steps #2 and #3, but the affected build and tests are addressed in #1. tony@, fishd@ - can I get an r? The corresponding Chromium changes have been lgtm'd, and obviously I'll land "part 1" before landing this. Comment on attachment 123767 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123767&action=review LGTM > Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:164 > + for (IndexMap::iterator it = m_indexes.begin(); it != m_indexes.end(); ++it) { So if an index's keypath yields no value, the index doesn't get an entry, but if the index's keypath yields an invalid value, DATA_ERR is thrown? Is the check at line 262-264 of this file now redundant or is there a difference between indexKey->valid() and indexKey->type() == IDBKey::InvalidType ? > LayoutTests/storage/indexeddb/keypath-edges.html:100 > + debug("Key path doesn't resolve to a value - internally, key should be null, put request should succeed"); I would have thought the key would be autoincremented to 1. Is the comment wrong or is my understanding wrong? Comment on attachment 123767 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123767&action=review > Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:158 > + } else { > + if (keyPathKey && !keyPathKey->valid()) { Nit: Merge lines into an "else if"? (In reply to comment #9) > (From update of attachment 123767 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=123767&action=review > > LGTM > > > Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:164 > > + for (IndexMap::iterator it = m_indexes.begin(); it != m_indexes.end(); ++it) { > > So if an index's keypath yields no value, the index doesn't get an entry, but if the index's keypath yields an invalid value, DATA_ERR is thrown? That's correct. In the spec: * For the add/put call(): "If any of the following conditions are true, this method throws a DOMException of type DataError: ... If there are any indexes referencing this object store whose key path is a string, evaluating their key path on the value parameter yields a value, and that value is not a valid key." * During the async store operation: "If index's key path is a string, then evaluate it on value. If this does not yield a value don't take any further actions for this index." > Is the check at line 262-264 of this file now redundant or is there a difference between indexKey->valid() and indexKey->type() == IDBKey::InvalidType ? Those expressions are the same, and the checks in putInternal (and selectKeyForPut) should now indeed be redundant. I'll see if I can safely take them out now (was going to do cleanup on them later once I'd come up for air). > > LayoutTests/storage/indexeddb/keypath-edges.html:100 > > + debug("Key path doesn't resolve to a value - internally, key should be null, put request should succeed"); > > I would have thought the key would be autoincremented to 1. Is the comment wrong or is my understanding wrong? The comment is extremely unclear. I meant the result of evaluating the key path would yield a "null" key; you're right, the actual key being applied would end up being 1. I'll fix the comment. FYI, I've filed https://bugs.webkit.org/show_bug.cgi?id=76952 for further cleanup that can be done following some spec discussion on the list, and added a FIXME comment with the URL. New patch coming shortly. Created attachment 123833 [details]
Patch
Comment on attachment 123833 [details] Patch Rejecting attachment 123833 [details] from commit-queue. New failing tests: media/audio-garbage-collect.html Full output: http://queues.webkit.org/results/11264148 Comment on attachment 123833 [details]
Patch
I'm re-marking cq+. That audio test had to be a flake.
Comment on attachment 123833 [details]
Patch
On second look, marking cq- because we didn't hear from Darin.
(In reply to comment #16) > (From update of attachment 123833 [details]) > On second look, marking cq- because we didn't hear from Darin. Oops, yeah, thanks. Comment on attachment 123833 [details]
Patch
API changes LGTM ... sorry for the latency. CQ+
Comment on attachment 123833 [details] Patch Clearing flags on attachment: 123833 Committed r105891: <http://trac.webkit.org/changeset/105891> All reviewed patches have been landed. Closing bug. |