IndexedDB: Fix reverse cursor with non-existent upper bound
Created attachment 116759 [details] Patch
Please take a look when you have time.
Comment on attachment 116759 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116759&action=review > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:1255 > cursorOptions.highOpen = true; // Not included. Would it be more readable as: if (cursorOptions.forward) { ... } else { ... } > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:1264 > cursorOptions.highOpen = range->upperOpen(); Would it be more readable as: if (cursorOptions.forward) { ... } else { ... } > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:1308 > if (!cursorOptions.forward) { // We need a key that exists. ditto re: if (cursorOptions.forward) { ... } else { ... } > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:1369 > + // If the target key should not be included, but we end up with a small key, we should include that. Typo: "small" -> "smaller" > LayoutTests/storage/indexeddb/cursor-reverse-bug.html:21 > + evalAndLog("IDBTransaction = window.IDBTransaction || window.webkitIDBTransaction"); Include IDBKeyRange = webkitIDBKeyRange here > LayoutTests/storage/indexeddb/cursor-reverse-bug.html:103 > + storeReq = evalAndLog("storeReq = store.openCursor(webkitIDBKeyRange.upperBound(test.upperBound, test.open), IDBCursor.PREV)"); ... and just use IDBKeyRange here
Thanks for the review! (In reply to comment #3) > (From update of attachment 116759 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=116759&action=review > > > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:1255 > > cursorOptions.highOpen = true; // Not included. > > Would it be more readable as: if (cursorOptions.forward) { ... } else { ... } Done. > > > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:1264 > > cursorOptions.highOpen = range->upperOpen(); > > Would it be more readable as: if (cursorOptions.forward) { ... } else { ... } > > > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:1308 > > if (!cursorOptions.forward) { // We need a key that exists. > > ditto re: if (cursorOptions.forward) { ... } else { ... } Hmm, I'm not sure what I'd put in the if-branch in this case? > > > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:1369 > > + // If the target key should not be included, but we end up with a small key, we should include that. > > Typo: "small" -> "smaller" Done. > > > LayoutTests/storage/indexeddb/cursor-reverse-bug.html:21 > > + evalAndLog("IDBTransaction = window.IDBTransaction || window.webkitIDBTransaction"); > > Include IDBKeyRange = webkitIDBKeyRange here Done. > > > LayoutTests/storage/indexeddb/cursor-reverse-bug.html:103 > > + storeReq = evalAndLog("storeReq = store.openCursor(webkitIDBKeyRange.upperBound(test.upperBound, test.open), IDBCursor.PREV)"); > > ... and just use IDBKeyRange here Done.
Created attachment 116978 [details] Patch
Josh: do you have any more comments? Tony: could you take a look?
Comment on attachment 116978 [details] Patch LGTM
Committed r101648: <http://trac.webkit.org/changeset/101648>