IndexedDB: Throw DATA_ERR on invalid keys, remove null key support
Created attachment 110917 [details] Patch
Note: this requires 3 commits: (1) WebKit Part 1 (this patch) - renames IDBKey::NullKey to IDBKey::InvalidKey and has the validation logic in the methods that take keys (2) Chromium patch which drops dependency on WebIDBKey::NullType, treating it the same as the previously existing WebIDBKey::InvalidType (3) WebKit Part 2 - removes WebIDBKey::NullType
Any feedback on how to handle the optional parameters in IDBKeyRange.h in a less grotesque way would be appreciated.
The Chromium side is at: http://codereview.chromium.org/8276023
Comment on attachment 110917 [details] Patch Attachment 110917 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10066316
Comment on attachment 110917 [details] Patch LGTM with nits; thanks for fixing. View in context: https://bugs.webkit.org/attachment.cgi?id=110917&action=review > Source/WebCore/storage/IDBKeyRange.cpp:44 > +PassRefPtr<IDBKeyRange> IDBKeyRange::only(PassRefPtr<IDBKey> prpValue, ExceptionCode& ec) maybe rename prpValue to prpKey while you're at it? > Source/WebCore/storage/IDBKeyRange.h:51 > + static PassRefPtr<IDBKeyRange> only(PassRefPtr<IDBKey> value, ExceptionCode&); i think you need to forward-declare ExceptionCode > Source/WebCore/storage/IDBLevelDBCoding.cpp:355 > + return Vector<char>(); being paranoid, if we ever end up in this code path, in a Release built (so we don't hit the assert), it might be bad if we end up writing an empty string for a key, since we could have problems parsing that later. So definitely keep the assert, but maybe return encodeByte(kIDBKeyNullTypeByte)? > Source/WebCore/storage/IDBSQLiteBackingStore.cpp:345 > + case IDBKey::InvalidType: I believe this file is now actually gone :) > Source/WebKit/chromium/src/AssertMatchingEnums.cpp:395 > +COMPILE_ASSERT_MATCHING_ENUM(WebIDBKey::NullType, IDBKey::InvalidType); not that it makes much difference, but a FIXME that this is temporary would be nice. > Source/WebKit/chromium/src/WebIDBKey.cpp:98 > + m_private = 0; why not IDBKey::createInvalid() ?
Comment on attachment 110917 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110917&action=review Thanks, all the nits are good. >> Source/WebKit/chromium/src/WebIDBKey.cpp:98 >> + m_private = 0; > > why not IDBKey::createInvalid() ? WebIDBKey already has a notion of invalid which it stores with a null pointer. Since we're collapsing the the Null and Invalid types together, I made Null act like Invalid rather than vice versa. I could go the other way and have invalid WebIDBKeys store an Invalid IDBKey; I don't have a strong preference.
Created attachment 111087 [details] Patch
(In reply to comment #7) > >> Source/WebKit/chromium/src/WebIDBKey.cpp:98 > >> + m_private = 0; > > > > why not IDBKey::createInvalid() ? > > WebIDBKey already has a notion of invalid which it stores with a null pointer. Since we're collapsing the the Null and Invalid types together, I made Null act like Invalid rather than vice versa. I could go the other way and have invalid WebIDBKeys store an Invalid IDBKey; I don't have a strong preference. Ok, that makes sense. LGTM
Tony, can you give this a look-see?
Comment on attachment 111087 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111087&action=review > Source/WebCore/storage/IDBKeyRange.h:56 > + return lowerBound(bound, false, ec); Nit: WebKit style prefers using enums rather than bools for arguments. It makes it more clear at the caller what the second param is. Since the code was already using bools, this is just a suggestion for a future cleanup. This would only apply to the private methods, not the idl methods.
Comment on attachment 111087 [details] Patch Filed https://bugs.webkit.org/show_bug.cgi?id=70743 as a TODO for the bool->enum nit
Comment on attachment 111087 [details] Patch Clearing flags on attachment: 111087 Committed r98278: <http://trac.webkit.org/changeset/98278>
All reviewed patches have been landed. Closing bug.