[webkit-reviews] review denied: [Bug 51110] IDBCursor::delete is not implemented. : [Attachment 76651] Implements IDBCursor::delete

Thu Dec 16 03:57:42 PST 2010

Jeremy Orlow <jorlow at chromium.org> has denied Andrei Popescu
<andreip at google.com>'s request for review:
Bug 51110: IDBCursor::delete is not implemented.
https://bugs.webkit.org/show_bug.cgi?id=51110

Attachment 76651: Implements IDBCursor::delete
https://bugs.webkit.org/attachment.cgi?id=76651&action=review

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
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.  :-/