We should implemement IDBCursor::delete.
Created attachment 76651 [details] Implements IDBCursor::delete
Comment on attachment 76651 [details] Implements IDBCursor::delete View in context: https://bugs.webkit.org/attachment.cgi?id=76651&action=review Overall, looks great. > LayoutTests/storage/indexeddb/cursor-delete.html:1 > +<html> Whats this? > LayoutTests/storage/indexeddb/cursor-delete.html:48 > + commitAndContinue(); Don't do this. I've been removing it as fast as I can, but apparnetly it's not fast enough. This is not a valid way of doing this. Set oncomplete > LayoutTests/storage/indexeddb/cursor-delete.html:163 > + deleteResult.onsuccess = iterateCursor; You could even just call continue here since it's supposed to run after the delete happens. > WebCore/storage/IDBCursorBackendImpl.cpp:170 > + // if (m_transaction->mode() == IDBTransaction::READ_ONLY) { We should be able to re-enable now. > WebCore/storage/IDBIndexBackendImpl.cpp:107 > + RefPtr<IDBCursorBackendInterface> cursor = IDBCursorBackendImpl::create(index->m_database.get(), range, direction, query.release(), objectCursor, transaction.get(), 0); You don't test this case. And if you did, you'd see a crash I'm pretty sure. I'm not sure how we can supply this without making cycles though. :-/
Created attachment 76877 [details] jorlow's comments
(In reply to comment #2) > (From update of attachment 76651 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=76651&action=review > > Overall, looks great. > > > LayoutTests/storage/indexeddb/cursor-delete.html:1 > > +<html> > > Whats this? > No idea, Visual Studio generated that :( > > LayoutTests/storage/indexeddb/cursor-delete.html:48 > > + commitAndContinue(); > > Don't do this. I've been removing it as fast as I can, but apparnetly it's not fast enough. This is not a valid way of doing this. Set oncomplete > Fixed. > > LayoutTests/storage/indexeddb/cursor-delete.html:163 > > + deleteResult.onsuccess = iterateCursor; > > You could even just call continue here since it's supposed to run after the delete happens. > Removed this since we have a pre-existing bug here. Will file a separate bug for the situation where we delete an item using objectStore.delete() while we're in the middle of iterating a cursor over the same objectStore. > > WebCore/storage/IDBCursorBackendImpl.cpp:170 > > + // if (m_transaction->mode() == IDBTransaction::READ_ONLY) { > > We should be able to re-enable now. > Doesn't work, I tried. Perhaps another bug? > > WebCore/storage/IDBIndexBackendImpl.cpp:107 > > + RefPtr<IDBCursorBackendInterface> cursor = IDBCursorBackendImpl::create(index->m_database.get(), range, direction, query.release(), objectCursor, transaction.get(), 0); > > You don't test this case. And if you did, you'd see a crash I'm pretty sure. I'm not sure how we can supply this without making cycles though. :-/ Oops, fixed. Note that when we were deleting from an object store, we weren't deleting the data from the associated index. Fixed that too.
Comment on attachment 76877 [details] jorlow's comments View in context: https://bugs.webkit.org/attachment.cgi?id=76877&action=review r=me > WebCore/storage/IDBIndexBackendImpl.cpp:109 > + ASSERT(objectStore.get()); don't need .get()....u can probably combine both asserts since it's likely that they'll both be true or not > WebCore/storage/IDBObjectStoreBackendImpl.cpp:309 > + SQLiteStatement indexQuery(objectStore->sqliteDatabase(), "DELETE FROM IndexData WHERE objectStoreDataId = ?"); We have an index on this, right?
Created attachment 76891 [details] jorlow's comments 2
(In reply to comment #5) > (From update of attachment 76877 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=76877&action=review > > r=me > > > WebCore/storage/IDBIndexBackendImpl.cpp:109 > > + ASSERT(objectStore.get()); > > don't need .get()....u can probably combine both asserts since it's likely that they'll both be true or not > Done. > > WebCore/storage/IDBObjectStoreBackendImpl.cpp:309 > > + SQLiteStatement indexQuery(objectStore->sqliteDatabase(), "DELETE FROM IndexData WHERE objectStoreDataId = ?"); > > We have an index on this, right? We do.
Comment on attachment 76891 [details] jorlow's comments 2 rubber stamp
Comment on attachment 76877 [details] jorlow's comments Cleared Jeremy Orlow's review+ from obsolete attachment 76877 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment on attachment 76891 [details] jorlow's comments 2 Looks like the commit queue is in a weird state and can't land this patch. It keeps saying: "Patch 76891 from bug 51110: 0 minutes ago (cr-jail-4) Tests passed, but commit failed (checkout out of date). Updating, then landing without building or re-running tests." even though I see no other changes landing at this time.
Comment on attachment 76891 [details] jorlow's comments 2 Rejecting attachment 76891 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'land-attachment', '--force-clean', '--ignore-builders', '--non-interactive', '--parent-command=commit-queue', 76891]" exit_code: 2 Last 500 characters of output: DBKey.cpp M WebCore/storage/IDBObjectStoreBackendImpl.cpp M WebKit/chromium/ChangeLog M WebKit/chromium/public/WebIDBCursor.h M WebKit/chromium/src/IDBCursorBackendProxy.cpp M WebKit/chromium/src/IDBCursorBackendProxy.h M WebKit/chromium/src/WebIDBCursorImpl.cpp M WebKit/chromium/src/WebIDBCursorImpl.h Merge conflict during commit: Conflict at '/trunk/WebKit/chromium/ChangeLog' at /usr/local/git/libexec/git-core/git-svn line 573 Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1 Full output: http://queues.webkit.org/results/7293078
Comment on attachment 76891 [details] jorlow's comments 2 Clearing flags on attachment: 76891 Committed r74342: <http://trac.webkit.org/changeset/74342>
All reviewed patches have been landed. Closing bug.