[webkit-reviews] review denied: [Bug 47245] IDBDatabase and IDBObjectStore metadata is not recovered correctly when the setVersion transactions aborts. : [Attachment 69883] Patch

[bugzilla-daemon at webkit.org]

Jeremy Orlow <jorlow at chromium.org> has denied Andrei Popescu
<andreip at google.com>'s request for review:
Bug 47245: IDBDatabase and IDBObjectStore metadata is not recovered correctly
when the setVersion transactions aborts.
https://bugs.webkit.org/show_bug.cgi?id=47245

Attachment 69883: Patch
https://bugs.webkit.org/attachment.cgi?id=69883&action=review

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=69883&action=review

Looks pretty good!

> LayoutTests/storage/indexeddb/objectstore-basics.html:73
> +function createIndex() {

{ on next line

> LayoutTests/storage/indexeddb/resources/shared.js:86
> +    verifyAbortEvent(event);

Verify complete event.

> WebCore/storage/IDBDatabaseBackendImpl.cpp:193
> +				     
createCallbackTask(&IDBDatabaseBackendImpl::removeObjectStoreFromMap, database,
objectStore))) {

This should add it, right?  Can you add testing for that?

> WebCore/storage/IDBDatabaseBackendImpl.cpp:256
> +    database->m_objectStores.remove(objectStore->name());

What does remove do if it doesn't exist?  If it doesn't assert, maybe add one
manually?

> WebCore/storage/IDBDatabaseBackendImpl.cpp:261
> +    database->m_objectStores.set(objectStore->name(), objectStore);

similarly here

> WebCore/storage/IDBObjectStoreBackendImpl.cpp:414
> +    objectStore->m_indexes.remove(index->name());

and here

> WebCore/storage/IDBObjectStoreBackendImpl.cpp:419
> +    objectStore->m_indexes.set(index->name(), index);

and here

> WebCore/storage/IDBTransactionBackendImpl.cpp:70
> +	   m_abortTaskQueue.prepend(abortTask);

Is this expensive?  If so, maybe you should process the abort task queue in
reverse?

And maybe have a layout test that adds removes adds removes and then aborts?

Also maybe add a layout test that double adds then aborts...and double removes
then aborts?

> WebCore/storage/IDBTransactionBackendImpl.h:50
> +    virtual bool scheduleTask(PassOwnPtr<ScriptExecutionContext::Task> task,
PassOwnPtr<ScriptExecutionContext::Task> abortTaskn);

abortTask minus the n