Bug 73220 - IndexedDB: Fix reverse cursor with non-existent upper bound
Summary: IndexedDB: Fix reverse cursor with non-existent upper bound
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hans Wennborg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-28 07:47 PST by Hans Wennborg
Modified: 2011-12-01 03:21 PST (History)
3 users (show)

See Also:


Attachments
Patch (17.46 KB, patch)
2011-11-28 07:50 PST, Hans Wennborg
no flags Details | Formatted Diff | Diff
Patch (17.85 KB, patch)
2011-11-29 08:56 PST, Hans Wennborg
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hans Wennborg 2011-11-28 07:47:10 PST
IndexedDB: Fix reverse cursor with non-existent upper bound
Comment 1 Hans Wennborg 2011-11-28 07:50:19 PST
Created attachment 116759 [details]
Patch
Comment 2 Hans Wennborg 2011-11-28 07:51:32 PST
Please take a look when you have time.
Comment 3 Joshua Bell 2011-11-28 10:24:16 PST
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
Comment 4 Hans Wennborg 2011-11-29 08:56:02 PST
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.
Comment 5 Hans Wennborg 2011-11-29 08:56:30 PST
Created attachment 116978 [details]
Patch
Comment 6 Hans Wennborg 2011-11-30 13:10:40 PST
Josh: do you have any more comments?

Tony: could you take a look?
Comment 7 Joshua Bell 2011-11-30 13:28:29 PST
Comment on attachment 116978 [details]
Patch

LGTM
Comment 8 Tony Chang 2011-11-30 13:47:46 PST
Comment on attachment 116978 [details]
Patch

LGTM
Comment 9 Hans Wennborg 2011-12-01 03:21:22 PST
Committed r101648: <http://trac.webkit.org/changeset/101648>