Bug 73220 – IndexedDB: Fix reverse cursor with non-existent upper bound

• Component: New Bugs
• Status: RESOLVED FIXED

[Hans Wennborg]

IndexedDB: Fix reverse cursor with non-existent upper bound

[Hans Wennborg]

Created attachment 116759 [details]
Patch

[Hans Wennborg]

Please take a look when you have time.

[Joshua Bell]

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

[Hans Wennborg]

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.

[Hans Wennborg]

Created attachment 116978 [details]
Patch

[Hans Wennborg]

Josh: do you have any more comments?

Tony: could you take a look?

[Joshua Bell]

Comment on attachment 116978 [details]
Patch

LGTM

[Tony Chang]

Comment on attachment 116978 [details]
Patch

LGTM

[Hans Wennborg]

Committed r101648: <http://trac.webkit.org/changeset/101648>