Bug 51110

Summary:

IDBCursor::delete is not implemented.

Product:

WebKit

Reporter:

Andrei Popescu <andreip>

Component:

WebCore Misc.

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, hans, jorlow

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Implements IDBCursor::delete	jorlow: review-
jorlow's comments	none
jorlow's comments 2	none

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.