| Summary: | IndexedDB: Null key path gets stored as empty string key path | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Joshua Bell <jsbell> | ||||||||||
| Component: | New Bugs | Assignee: | Joshua Bell <jsbell> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | dglazkov, dgrogan, hans, tony, webkit.review.bot | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Joshua Bell
2011-09-23 13:49:22 PDT
Created attachment 108530 [details]
Patch
Chromium issue is http://code.google.com/p/chromium/issues/detail?id=90635 Tests to detect regressions across browser sessions will be added on the chromium side. Comment on attachment 108530 [details] Patch Attachment 108530 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9831049 Thanks! It will be very nice to get this landed. The storage/indexeddb/migrate-basics.html layout test failed when I tried this locally. Created attachment 108558 [details]
Patch
Hooray for tests. Ensure we don't try and decode a stop key. Passes the storage layout tests now Comment on attachment 108558 [details] Patch Attachment 108558 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9831125 From that bot: Source/WebCore/storage/IDBLevelDBBackingStore.cpp:225: error: unused variable 'p' Comment on attachment 108558 [details] Patch Looks good, just a couple of nits. View in context: https://bugs.webkit.org/attachment.cgi?id=108558&action=review > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:226 > + ASSERT(p); you want ASSERT_UNUSED(p, p); > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:291 > + hasKeyPath = *it->value().begin(); This works, but I would feel safer if we had specific encode/decode functions for bools. Would you like to add a FIXME about that? > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:409 > + if (!deleteRange(m_currentTransaction.get(), ObjectStoreMetaDataKey::encode(databaseId, objectStoreId, 0), ObjectStoreMetaDataKey::encode(databaseId, objectStoreId, INT64_MAX))) We try to avoid using INT64_MAX in this file by calling various *encodeMaxKey functions instead. You can add a ObjectStoreMetaDataKey::encodeMaxKey(int databaseId, int objectStoreId) function. Created attachment 108681 [details]
Patch
(In reply to comment #10) > Created an attachment (id=108681) [details] > Patch LGTM Comment on attachment 108681 [details]
Patch
Tony, can you please review?
Comment on attachment 108681 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108681&action=review > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:215 > +bool checkObjectStoreAndMetaDataType(const LevelDBIterator* it, const Vector<char>& stopKey, int64_t objectStoreId, int64_t metaDataType) Nit: Should this function be static? Created attachment 108687 [details]
Patch
Comment on attachment 108687 [details]
Patch
Good call on static function
Comment on attachment 108687 [details] Patch Clearing flags on attachment: 108687 Committed r96007: <http://trac.webkit.org/changeset/96007> All reviewed patches have been landed. Closing bug. |