Bug 53690 – IndexedDB: Cursors should skip deleted entries

• Component: New Bugs
• Status: RESOLVED FIXED

[Hans Wennborg]

IndexedDB: Cursors should skip deleted entries

[Hans Wennborg]

Created attachment 81073 [details]
Patch

[Jeremy Orlow]

Comment on attachment 81073 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81073&action=review

Maybe you should make your test function a bit more generic and then you can specifically test more corner cases: the first element, the last, the current, the previous, a couple previous, the next, and a couple later.  (The last is all you test now.)  I don't think it'll take too much time and it'll definitely make this more robust in the future.

Looks great though.

> Source/WebCore/storage/IDBCursorBackendImpl.cpp:104
> +

newline seems kinda useless....you could even use a ternary operator if you wanted...don't care much either way tho

[Hans Wennborg]

(In reply to comment #2)
> (From update of attachment 81073 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81073&action=review
> 
> Maybe you should make your test function a bit more generic and then you can specifically test more corner cases: the first element, the last, the current, the previous, a couple previous, the next, and a couple later.  (The last is all you test now.)  I don't think it'll take too much time and it'll definitely make this more robust in the future.

You're right. Trying to make the test a bit more generic and less spaghetti.

> 
> Looks great though.
> 
> > Source/WebCore/storage/IDBCursorBackendImpl.cpp:104
> > +
> 
> newline seems kinda useless....you could even use a ternary operator if you wanted...don't care much either way tho

Done.

[Hans Wennborg]

Created attachment 81226 [details]
Patch

[Jeremy Orlow]

Comment on attachment 81226 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81226&action=review

This test should also verify that anything not deleted is seen while iterating.  r=me assuming you do that.

> LayoutTests/storage/indexeddb/cursor-skip-deleted.html:61
> +    resetObjectStore(); // No callback; let the setVersion transaction complete.

Seems cleaner to pass in an empty function here...you probably don't even need a comment then.

> LayoutTests/storage/indexeddb/cursor-skip-deleted.html:140
> +                   {id: 15, targets: [13,14]}

while you're at it, might as well throw a few more interesting combinations in.  :-)

> LayoutTests/storage/indexeddb/cursor-skip-deleted.html:169
> +    testCursor(deletes, "trans.objectStore('store').index('nameIndex').openCursor(webkitIDBKeyRange.lowerBound('Alpha'))", function() { resetObjectStore(); });

why do a final reset?

[Hans Wennborg]

Committed r78035: <http://trac.webkit.org/changeset/78035>