51110 – IDBCursor::delete is not implemented.

RESOLVED FIXED 51110

IDBCursor::delete is not implemented.

https://bugs.webkit.org/show_bug.cgi?id=51110

Summary IDBCursor::delete is not implemented.

Andrei Popescu

Reported 2010-12-15 08:12:17 PST

We should implemement IDBCursor::delete.

Attachments
Implements IDBCursor::delete (29.32 KB, patch) 2010-12-15 08:55 PST, Andrei Popescu	jorlow: review-	Details Formatted Diff Diff
jorlow's comments (38.58 KB, patch) 2010-12-17 07:25 PST, Andrei Popescu	no flags	Details Formatted Diff Diff
jorlow's comments 2 (38.57 KB, patch) 2010-12-17 10:44 PST, Andrei Popescu	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Andrei Popescu

Comment 1 2010-12-15 08:55:26 PST

Created attachment 76651 [details] Implements IDBCursor::delete

Jeremy Orlow

Comment 2 2010-12-16 03:57:43 PST

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. :-/

Andrei Popescu

Comment 3 2010-12-17 07:25:47 PST

Created attachment 76877 [details] jorlow's comments

Andrei Popescu

Comment 4 2010-12-17 07:29:33 PST

(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.

Jeremy Orlow

Comment 5 2010-12-17 10:00:20 PST

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?

Andrei Popescu

Comment 6 2010-12-17 10:44:21 PST

Created attachment 76891 [details] jorlow's comments 2

Andrei Popescu

Comment 7 2010-12-17 10:44:41 PST

(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.

Jeremy Orlow

Comment 8 2010-12-17 10:45:03 PST

Comment on attachment 76891 [details] jorlow's comments 2 rubber stamp

Eric Seidel (no email)

Comment 9 2010-12-17 16:38:28 PST

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.

Andrei Popescu

Comment 10 2010-12-20 06:27:15 PST

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.

WebKit Commit Bot

Comment 11 2010-12-20 06:31:32 PST

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

WebKit Commit Bot

Comment 12 2010-12-20 06:31:41 PST

Comment on attachment 76891 [details] jorlow's comments 2 Clearing flags on attachment: 76891 Committed r74342: <http://trac.webkit.org/changeset/74342>

WebKit Commit Bot

Comment 13 2010-12-20 06:31:48 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware All

OS All

Product WebKit

Component WebCore Misc.

Assignee

Nobody

Reported

2010-12-15 08:12 PST

Modified

2010-12-20 06:31 PST History

CC List

3 users Show

URL

Keywords

Depends on

Blocks