IndexedDB: IDBCursor.continue(key) does not throw for out of range key
Created attachment 155090 [details] Patch
I found this while looking at our exception coverage for http://webkit.org/b/85513 dgrogan@, alecflett@ - please take a look.
Comment on attachment 155090 [details] Patch Whoops, forgot expectations.
Created attachment 155092 [details] Patch
Updated patch has expectations and removes some LOG_ERROR lines I left in.
(In reply to comment #2) > I found this while looking at our exception coverage for http://webkit.org/b/85513 > > dgrogan@, alecflett@ - please take a look. Ping?
Comment on attachment 155092 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155092&action=review LGTM > Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:45 > +IDBCursorBackendImpl::IDBCursorBackendImpl(PassRefPtr<IDBBackingStore::Cursor> cursor, IDBCursor::Direction, CursorType cursorType, IDBTransactionBackendImpl* transaction, IDBObjectStoreBackendImpl* objectStore) I'm guessing IDBCursor::Direction is still used by IDBCursorBackendProxy? > LayoutTests/storage/indexeddb/resources/cursor-continue-dir.js:55 > + request.onsuccess = null; What's going on with the request.onsuccess = null lines?
Comment on attachment 155092 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155092&action=review >> Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:45 >> +IDBCursorBackendImpl::IDBCursorBackendImpl(PassRefPtr<IDBBackingStore::Cursor> cursor, IDBCursor::Direction, CursorType cursorType, IDBTransactionBackendImpl* transaction, IDBObjectStoreBackendImpl* objectStore) > > I'm guessing IDBCursor::Direction is still used by IDBCursorBackendProxy? No, it could be removed. (I rushed this patch a bit since it was a regression and I was headed out.) The direction is passed through to the IDBBackingStore::Cursor creation during the openCursor call, and the backend cursor is passed in here. Turns out the IDBCursorBackend* no longer needs to know about the direction at all. I can remove it. (I'll post an updated patch on Wednesday.) >> LayoutTests/storage/indexeddb/resources/cursor-continue-dir.js:55 >> + request.onsuccess = null; > > What's going on with the request.onsuccess = null lines? Debugging residue, I'll remove them. (Before fixing the bug the cursor.continue() call was not throwing, so the test would infinite-loop if the success handler was still around.)
in another patch, I'm making progress towards replacing the 'unsigned short' directions with IDBCursor::Direction, so definitely don't remove it! My intention is that for direction, either you're using the string ("next") or the enum, (and the string quickly gets transformed into the enum) but not the unsigned short. But this LGTM to me as well - makes me wonder what else we can rip out of the cursor backend, since it sounds like the backing store is doing most of the work.
(In reply to comment #9) > in another patch, I'm making progress towards replacing the 'unsigned short' directions with IDBCursor::Direction, so definitely don't remove it! We're just talking about removing it from the IDBCursorBackendImpl constructor - that's not a problem, is it? > But this LGTM to me as well - makes me wonder what else we can rip out of the cursor backend, since it sounds like the backing store is doing most of the work. It's pretty slim at this point. Most of the code is boilerplate or prefetching. I think some of the deleteFunction() checks can turn into ASSERTs but I'll leave that for later.
(In reply to comment #10) > (In reply to comment #9) > > in another patch, I'm making progress towards replacing the 'unsigned short' directions with IDBCursor::Direction, so definitely don't remove it! > > We're just talking about removing it from the IDBCursorBackendImpl constructor - that's not a problem, is it? Nope...I just mean don't remove the enum itself in the process of cleaning this up..
Created attachment 155832 [details] Patch
Latest patch addresses dgrogan@'s comments - the unused direction argument to the IDBCursorBackendImpl is removed, and the unnecessary event handler clearing is removed. tony@ (or others) - r? cq?
Oops, tony@ is unavailable. ojan@, abarth@, eric@ - r? cq?
Comment on attachment 155832 [details] Patch rs=me
Comment on attachment 155832 [details] Patch Clearing flags on attachment: 155832 Committed r124361: <http://trac.webkit.org/changeset/124361>
All reviewed patches have been landed. Closing bug.