Bug 62622 – Implement IDBFactory.deleteDatabase

• Component: New Bugs
• Status: RESOLVED FIXED

Implement IDBFactory.deleteDatabase

Created attachment 97079 [details]
Patch

This is what I have currently.

The remaining issue is that setVersion doesn't block opening new database connections. Consequently, I can't use a setVersion transaction to reliably close all existing connections (and thus the layout test doesn't work)

I guess I'll first fix that before I continue to work on this patch

Feedback on this one would be welcome nevertheless

[Hans Wennborg]

Comment on attachment 97079 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97079&action=review

> LayoutTests/storage/indexeddb/database-delete.html:46
> +    commitAndContinue();

I don't think commitAndContinue() is safe. It's better to use the .oncomplete handler on the transaction.

> Source/WebCore/storage/IDBLevelDBBackingStore.cpp:181
> +    return deleteFile(path);

Whoa, that'll delete the LevelDB for the whole origin.

Feel free to not worry about the LevelDB back-end for now, and just stick a FIXME in here.

> Source/WebCore/storage/IDBSQLiteBackingStore.cpp:209
> +    return deleteFile(path);

This also deletes the whole origin.

I think deleteDatabase() needs to take the database name as a parameter, and then only delete the right stuff from the backing store.

[David Grogan]

(In reply to comment #2)

> The remaining issue is that setVersion doesn't block opening new database connections. Consequently, I can't use a setVersion transaction to reliably close all existing connections (and thus the layout test doesn't work)

Doesn't the layout test open just one connection and wait for it to close before deleting the database?  Where does the layout test fail?

(In reply to comment #4)
> (In reply to comment #2)
> 
> > The remaining issue is that setVersion doesn't block opening new database connections. Consequently, I can't use a setVersion transaction to reliably close all existing connections (and thus the layout test doesn't work)
> 
> Doesn't the layout test open just one connection and wait for it to close before deleting the database?  Where does the layout test fail?

the internal error in IDBFactoryBackendImpl.cpp:115 is thrown

[David Grogan]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #2)
> > 
> > > The remaining issue is that setVersion doesn't block opening new database connections. Consequently, I can't use a setVersion transaction to reliably close all existing connections (and thus the layout test doesn't work)
> > 
> > Doesn't the layout test open just one connection and wait for it to close before deleting the database?  Where does the layout test fail?
> 
> the internal error in IDBFactoryBackendImpl.cpp:115 is thrown

That error is thrown as the layout test is in your patch now?  Not only if the window.setTimeout(deleteDatabase, 0) temporary hack is removed?

[Hans Wennborg]

FYI, the spec seems to be getting an update for deleteDatabase: http://lists.w3.org/Archives/Public/public-webapps/2011AprJun/1064.html
Not sure if that affects this patch though.

Created attachment 104344 [details]
Patch

Created attachment 104346 [details]
Patch

I've implemented this returning an IDBVersionChangeRequest.

Note that as long as the version change stuff isn't done asynchronously, i.e. we can't close all connections and do something when this is done, it's hardly possible to use deleteDatabase: as soon as you've opened the database, the connection is cached and we cannot safely delete the database without first ensuring that it is closed.

[Hans Wennborg]

Comment on attachment 104346 [details]
Patch

Thanks for sticking with this!


View in context: https://bugs.webkit.org/attachment.cgi?id=104346&action=review

> LayoutTests/storage/indexeddb/database-delete.html:18
> +{

It seems the test deletes a database which doesn't exist in the first place. Shouldn't it be creating a database, then close it, delete it, and verify it's gone?
Oh, I suppose that this is the part that doesn't work?

> LayoutTests/storage/indexeddb/database-delete.html:45
> +function reopenSuccess()

nit: I think this function should come after deleteSuccess(), so they're lined up in the order they run basically.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:270
> +        // FIXME: Close the database first. For now, we just chicken out, so we can do the delete further down synchronously.

hmm, I'm not entirely sure how that stuff works, but shouldn't the transaction machinery be guaranteeing that all other connections to this database is closed when the setVersion transaction runs? Or is that the part that doesn't work yet?

It would be nice if we didn't need to replicate all the logic of IDBFactoryBackendImpl::open(), but could just call it, delete the database, and remove it from the m_databaseBackendMap.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:289
> +        }

hmm, this is kind of weird...
If there is a SQLite database, we first migrate the database to LevelDB, and then delete it.
Only at the moment, we don't actually delete from SQLite after migration, so this ends up being a no-op.. we'll need to fix this.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:323
> +        backingStore->deleteObjectStore(databaseId, *objectStoreId);

It would be better if IDBBackingStore had a deleteDatabase() function that deletes both the database meta-data, and all object stores, but this is fine for now. Maybe add a FIXME?

Comment on attachment 104346 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104346&action=review

>> LayoutTests/storage/indexeddb/database-delete.html:18
>> +{
> 
> It seems the test deletes a database which doesn't exist in the first place. Shouldn't it be creating a database, then close it, delete it, and verify it's gone?
> Oh, I suppose that this is the part that doesn't work?

Right. 

It would work if there was a way to delete the IDBDatabase object from javascript. Just invoking db.close(); db=null; is not enough. I guess we'd at least need to trigger a garbage collection or something

>> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:270
>> +        // FIXME: Close the database first. For now, we just chicken out, so we can do the delete further down synchronously.
> 
> hmm, I'm not entirely sure how that stuff works, but shouldn't the transaction machinery be guaranteeing that all other connections to this database is closed when the setVersion transaction runs? Or is that the part that doesn't work yet?
> 
> It would be nice if we didn't need to replicate all the logic of IDBFactoryBackendImpl::open(), but could just call it, delete the database, and remove it from the m_databaseBackendMap.

IDBFactoryBackendImpl::open doesn't check whether there's a transaction in flight, so you can create a new connection while the transaction runs

Also, there's no way to register a callback with the backing store for when the setversion transaction completes.

Created attachment 104487 [details]
Patch

Comment on attachment 104487 [details]
Patch

Addressed all comments

[David Grogan]

Sorry for the delay, I'm working on making pending setVersion calls block an open call.  Though I'm a bit confused as to whether that will help you or not.

(In reply to comment #15)
> Sorry for the delay, I'm working on making pending setVersion calls block an open call.  Though I'm a bit confused as to whether that will help you or not.

My hope is that you will have a way to schedule a task to be executed when the setVersion transaction has finished.

For deleting a database, the steps start by firing a setVersion transaction at the database. I'd then register a task that actually deletes the database for when the transaction is finished, using whatever means you used to schedule that task

[Joshua Bell]

*** Bug 62334 has been marked as a duplicate of this bug. ***

[Joshua Bell]

(In reply to comment #16)
> 
> My hope is that you will have a way to schedule a task to be executed when the setVersion transaction has finished.
> 
> For deleting a database, the steps start by firing a setVersion transaction at the database. I'd then register a task that actually deletes the database for when the transaction is finished, using whatever means you used to schedule that task

I've picked up dgrogan's work on blocking actions until after setVersion. Patch in review at:

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

@jochen: can you see if that approach would be sufficient? It doesn't allow for arbitrary tasks to be scheduled for running after setVersions are processed (just other setVersion calls and other open calls). If deletes vs. opens need to have a specific order then a separate queue for each makes sense. Otherwise we can coalesce those into a queue of generic tasks, similar to what transactions have internally.

The setVersion mechanism is being replaced "soon", so we don't want to get too clever right now.

(In reply to comment #18)
> (In reply to comment #16)
> > 
> > My hope is that you will have a way to schedule a task to be executed when the setVersion transaction has finished.
> > 
> > For deleting a database, the steps start by firing a setVersion transaction at the database. I'd then register a task that actually deletes the database for when the transaction is finished, using whatever means you used to schedule that task
> 
> I've picked up dgrogan's work on blocking actions until after setVersion. Patch in review at:
> 
> https://bugs.webkit.org/show_bug.cgi?id=69307
> 
> @jochen: can you see if that approach would be sufficient? It doesn't allow for arbitrary tasks to be scheduled for running after setVersions are processed (just other setVersion calls and other open calls). If deletes vs. opens need to have a specific order then a separate queue for each makes sense. Otherwise we can coalesce those into a queue of generic tasks, similar to what transactions have internally.

The delete callbacks should come before the open calls, so two queues will be fine.

I think I can work on top of your patch, thanks!

> 
> The setVersion mechanism is being replaced "soon", so we don't want to get too clever right now.

Created attachment 111352 [details]
Patch

After the setVersion/open changes are landed, here's the updated patch

Note that deleting the database doesn't actually work :(

the factory-deletedatabase.html test is demonstrating this

I guess IDBLevelDBBackingStore::deleteDatabase does something wrong, I verified it is invoked, and doesn't exit early

any idea what's going on, leveldb-knowledgable-ppl?

[WebKit Review Bot]

Comment on attachment 111352 [details]
Patch

Attachment 111352 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10122113

New failing tests:
storage/indexeddb/factory-basics.html
storage/indexeddb/factory-cmp.html

Created attachment 111477 [details]
Patch

[Joshua Bell]

Comment on attachment 111477 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111477&action=review

Not a complete review, I'll spend more time on it. Just some initial observations.

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:281
> +    while (!m_runningVersionChangeTransaction && m_pendingSetVersionCalls.isEmpty() && !m_deletePending && !pendingDeleteCalls.isEmpty()) {

It shouldn't occur, but if m_runningVersionChangeTransaction or m_pendingSetVersionCalls became true during this loop, the pendingDeleteCalls that hadn't been requeued would be lost. Can you pull those clauses out to a surrounding if block and/or early exit?

What happens if several calls are in the queue and m_deletePending becomes true? Will the rest of the calls in the queue get lost and hence the callbacks never fire?

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:284
> +        if (!m_pendingDeleteCalls.isEmpty()) {

Would it be cleaner to just let deleteBackingStore() requeue the pending calls? (If not, we should adjust the pendingOpenCall logic below)

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:327
> +        m_pendingDeleteCalls.append(PendingDeleteCall::create(prpCallbacks));

Nit: Return early here so the rest of the method doesn't need to be in the else block.

> Source/WebCore/storage/IDBRequest.cpp:299
> +    // If the result was of type IDBCursor, or a onBlocked event, then we'll fire again.

I think we determined this shouldn't be necessary (onblocked shouldn't have a result), but haven't finished investigating why this occurs. Can you add a FIXME unless you made further discoveries?

Created attachment 111524 [details]
Patch

Comment on attachment 111477 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111477&action=review

>> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:281
>> +    while (!m_runningVersionChangeTransaction && m_pendingSetVersionCalls.isEmpty() && !m_deletePending && !pendingDeleteCalls.isEmpty()) {
> 
> It shouldn't occur, but if m_runningVersionChangeTransaction or m_pendingSetVersionCalls became true during this loop, the pendingDeleteCalls that hadn't been requeued would be lost. Can you pull those clauses out to a surrounding if block and/or early exit?
> 
> What happens if several calls are in the queue and m_deletePending becomes true? Will the rest of the calls in the queue get lost and hence the callbacks never fire?

I've remodeled the section to be hopefully clearer

>> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:327
>> +        m_pendingDeleteCalls.append(PendingDeleteCall::create(prpCallbacks));
> 
> Nit: Return early here so the rest of the method doesn't need to be in the else block.

done

>> Source/WebCore/storage/IDBRequest.cpp:299
>> +    // If the result was of type IDBCursor, or a onBlocked event, then we'll fire again.
> 
> I think we determined this shouldn't be necessary (onblocked shouldn't have a result), but haven't finished investigating why this occurs. Can you add a FIXME unless you made further discoveries?

If I fire an onBlocked event followed by an onSuccess event without the onBlocked event being processed in between, m_result is set when onBlocked is being dispatched

This happens in factory-deletedatabase-interactions test3

[Joshua Bell]

Comment on attachment 111524 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111524&action=review

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:324
> +void IDBDatabaseBackendImpl::deleteBackingStore(PassRefPtr<IDBCallbacks> prpCallbacks)

Nit: Suggest "deleteDatabase" as the method name, since the backing store is not what's being deleted

> Source/WebCore/storage/IDBFactory.idl:35
> +        [CallWith=ScriptExecutionContext] IDBVersionChangeRequest deleteDatabase(in DOMString anem)

Typo: "anem" -> "name"

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:132
> +        // then.

Nit: typo: "then" -> "them"

> Source/WebCore/storage/IDBLevelDBBackingStore.cpp:263
> +    m_db->remove(DatabaseMetaDataKey::encode(databaseId, DatabaseMetaDataKey::kUserVersion));

This doesn't remove the other database metadata (origin name, database name, maximum store ID, or add the databaseId to the free list. I'm not sure how crucial the former are, or if the latter is actually used.

> Source/WebKit/chromium/src/WebIDBFactoryImpl.cpp:78
> +    WebString path = dataDir;

Do you need this copy here?

> LayoutTests/storage/indexeddb/factory-deletedatabase.html:13
> +description("Test IndexedDB's webkitIndexedDB.deleteDatabase().");

Please add a test that deletes a database that doesn't exist. (Unless I missed it)

> LayoutTests/storage/indexeddb/factory-deletedatabase.html:38
> +    deleteAllObjectStores(db);

Don't need this in the test, since it will run fresh each time. Delete for clarity.

[Hans Wennborg]

Comment on attachment 111524 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111524&action=review

> Source/WebCore/storage/IDBLevelDBBackingStore.cpp:238
> +{

I would go for a more low-level approach here. We should be able to just nuke everything that's prefixed with the database id:

KeyPrefix start(databaseId, 0, 0);
KeyPrefix stop(databaseId + 1, 0, 0);
deleteRange(start::encode(), stop::encode());

This covers the meta-data, object stores, indexes and anything we might add in the future.

[Joshua Bell]

Comment on attachment 111524 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111524&action=review

> Source/WebCore/storage/IDBLevelDBBackingStore.cpp:259
> +        deleteObjectStore(databaseId, *objectStoreId);

+1 to Hans; also, deleteObjectStore asserts that m_currentTransaction is non-null whereas this method early-exits if m_currentTransaction is null, so that method isn't directly usable anyway

Created attachment 112356 [details]
Patch

(In reply to comment #27)
> (From update of attachment 111524 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111524&action=review
> 
> > Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:324
> > +void IDBDatabaseBackendImpl::deleteBackingStore(PassRefPtr<IDBCallbacks> prpCallbacks)
> 
> Nit: Suggest "deleteDatabase" as the method name, since the backing store is not what's being deleted

done

> 
> > Source/WebCore/storage/IDBFactory.idl:35
> > +        [CallWith=ScriptExecutionContext] IDBVersionChangeRequest deleteDatabase(in DOMString anem)
> 
> Typo: "anem" -> "name"

done

> 
> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:132
> > +        // then.
> 
> Nit: typo: "then" -> "them"

done

> 
> > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:263
> > +    m_db->remove(DatabaseMetaDataKey::encode(databaseId, DatabaseMetaDataKey::kUserVersion));
> 
> This doesn't remove the other database metadata (origin name, database name, maximum store ID, or add the databaseId to the free list. I'm not sure how crucial the former are, or if the latter is actually used.

done

> 
> > Source/WebKit/chromium/src/WebIDBFactoryImpl.cpp:78
> > +    WebString path = dataDir;
> 
> Do you need this copy here?

removed

> 
> > LayoutTests/storage/indexeddb/factory-deletedatabase.html:13
> > +description("Test IndexedDB's webkitIndexedDB.deleteDatabase().");
> 
> Please add a test that deletes a database that doesn't exist. (Unless I missed it)

done

> 
> > LayoutTests/storage/indexeddb/factory-deletedatabase.html:38
> > +    deleteAllObjectStores(db);
> 
> Don't need this in the test, since it will run fresh each time. Delete for clarity.

done

(In reply to comment #29)
> (From update of attachment 111524 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111524&action=review
> 
> > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:259
> > +        deleteObjectStore(databaseId, *objectStoreId);
> 
> +1 to Hans; also, deleteObjectStore asserts that m_currentTransaction is non-null whereas this method early-exits if m_currentTransaction is null, so that method isn't directly usable anyway

After chatting with Hans, I changed the implementation to what it is now.

Note that while I check that there's no current transaction, I create a new one in that function

Created attachment 112362 [details]
Patch

[Hans Wennborg]

Comment on attachment 112362 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112362&action=review

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:132
> +        // them.

is this comment right? isn't this just finding the database backend in the cache, calling delete on it, and then we can return?

> LayoutTests/storage/indexeddb/factory-deletedatabase-interactions.html:13
> +description("Test the deleteDatabase call and it's interaction with open/setVersion");

s/it's/its/


This looks good to me as far as I can tell (just the minor nits above), but there are some points I'm not entirely sure of how they work; it would be great if Josh would take a look too.

Created attachment 112687 [details]
Patch

(In reply to comment #34)
> (From update of attachment 112362 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112362&action=review
> 
> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:132
> > +        // them.
> 
> is this comment right? isn't this just finding the database backend in the cache, calling delete on it, and then we can return?

updated comment

> 
> > LayoutTests/storage/indexeddb/factory-deletedatabase-interactions.html:13
> > +description("Test the deleteDatabase call and it's interaction with open/setVersion");
> 
> s/it's/its/

done

> 
> 
> This looks good to me as far as I can tell (just the minor nits above), but there are some points I'm not entirely sure of how they work; it would be great if Josh would take a look too.

[WebKit Review Bot]

Comment on attachment 112687 [details]
Patch

Attachment 112687 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10229559

New failing tests:
storage/indexeddb/factory-deletedatabase.html

(In reply to comment #37)
> (From update of attachment 112687 [details])
> Attachment 112687 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/10229559
> 
> New failing tests:
> storage/indexeddb/factory-deletedatabase.html

passes locally :-/

[Joshua Bell]

Comment on attachment 112362 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112362&action=review

The flow of the tests make sense to me. (cr-linux failure probably a bot hiccup, log doesn't show that the new test actually failed)

> LayoutTests/storage/indexeddb/factory-deletedatabase-interactions.html:80
> +    window.steps.push(evalAndLog("'deleteDatabase'"));

For consistency, should include the database name in the "step" so we can distinguish ordering later on. Calls that go through Connection do this automagically e.g. "h.open" where "h" is the name. So "deleteDatabase(h)" or something.

> LayoutTests/storage/indexeddb/factory-deletedatabase-interactions.html:84
> +        if (opts && opts.onsuccess) { opts.onsuccess.call(self); }

"self" doesn't refer to anything here. (Within a Connection object above, it's an alias for "this" captured in the closure, but this function isn't a method on an object)
It's harmless because it's only used to provide a value for "this" when the onXXX is fired, and that's never used the deleteDatabase calls. (In fact, it isn't used anywhere, all of these onXXX are plain functions rather than referring back to the connection)

So... just change call(self) to call(null) within deleteDatabase(). (As this test is written, the behavior should be identical if you make that change within function Connection as well, but there it does have a purpose, even if it's unused.)

> LayoutTests/storage/indexeddb/factory-deletedatabase-interactions.html:120
> +         function(doNext) { deleteDatabase(window.dbname, {

indentation

> LayoutTests/storage/indexeddb/factory-deletedatabase-interactions.html:147
> +         function(doNext) { deleteDatabase(window.dbname, {

indentation

> LayoutTests/storage/indexeddb/factory-deletedatabase-interactions.html:206
> +         function(doNext) { deleteDatabase(window.dbname); doNext(); },

indentation

Created attachment 112823 [details]
Patch

(In reply to comment #39)
> (From update of attachment 112362 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112362&action=review
> 
> The flow of the tests make sense to me. (cr-linux failure probably a bot hiccup, log doesn't show that the new test actually failed)
> 
> > LayoutTests/storage/indexeddb/factory-deletedatabase-interactions.html:80
> > +    window.steps.push(evalAndLog("'deleteDatabase'"));
> 
> For consistency, should include the database name in the "step" so we can distinguish ordering later on. Calls that go through Connection do this automagically e.g. "h.open" where "h" is the name. So "deleteDatabase(h)" or something.

as the name of the database to be deleted isn't necessarily unique across invocations (e.g. I once try to delete the same database twice), I've added an extra id argument which is logged with all calls, so we can tell different invocations apart

> 
> > LayoutTests/storage/indexeddb/factory-deletedatabase-interactions.html:84
> > +        if (opts && opts.onsuccess) { opts.onsuccess.call(self); }
> 
> "self" doesn't refer to anything here. (Within a Connection object above, it's an alias for "this" captured in the closure, but this function isn't a method on an object)
> It's harmless because it's only used to provide a value for "this" when the onXXX is fired, and that's never used the deleteDatabase calls. (In fact, it isn't used anywhere, all of these onXXX are plain functions rather than referring back to the connection)
> 
> So... just change call(self) to call(null) within deleteDatabase(). (As this test is written, the behavior should be identical if you make that change within function Connection as well, but there it does have a purpose, even if it's unused.)

done

> 
> > LayoutTests/storage/indexeddb/factory-deletedatabase-interactions.html:120
> > +         function(doNext) { deleteDatabase(window.dbname, {
> 
> indentation

done

> 
> > LayoutTests/storage/indexeddb/factory-deletedatabase-interactions.html:147
> > +         function(doNext) { deleteDatabase(window.dbname, {
> 
> indentation

done

> 
> > LayoutTests/storage/indexeddb/factory-deletedatabase-interactions.html:206
> > +         function(doNext) { deleteDatabase(window.dbname); doNext(); },
> 
> indentation

done

[WebKit Review Bot]

Comment on attachment 112823 [details]
Patch

Attachment 112823 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10236200

New failing tests:
storage/indexeddb/factory-deletedatabase.html

Created attachment 112841 [details]
Patch

[Tony Chang]

Comment on attachment 112841 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112841&action=review

Just some style nits.

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:293
> +    if (m_runningVersionChangeTransaction || !m_pendingSetVersionCalls.isEmpty() || !m_pendingDeleteCalls.isEmpty())

Nit: Is the !m_pendingDeleteCalls.isEmpty() necessary at this point?

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:340
> +    // FIXME: Only fire onVersionChange if there the connection isn't in
> +    // the process of closing.
> +    // http://crbug.com/100645

Nit: Would be nice to have webkit bugs here instead of chromium bugs.

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:345
> +    // FIXME: Only fire onBlocked if there are open connections after the
> +    // VersionChangeEvents are received, not just set up to fire.
> +    // http://crbug.com/100123

ditto

> LayoutTests/storage/indexeddb/factory-deletedatabase-interactions.html:228
> +var successfullyParsed = true;

Nit: After https://bugs.webkit.org/show_bug.cgi?id=70784 , this is no longer required.

> LayoutTests/storage/indexeddb/factory-deletedatabase.html:101
> +var successfullyParsed = true;

Ditto.

Created attachment 112897 [details]
Patch

(In reply to comment #44)
> (From update of attachment 112841 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112841&action=review
> 
> Just some style nits.
> 
> > Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:293
> > +    if (m_runningVersionChangeTransaction || !m_pendingSetVersionCalls.isEmpty() || !m_pendingDeleteCalls.isEmpty())
> 
> Nit: Is the !m_pendingDeleteCalls.isEmpty() necessary at this point?

It can be non-empty. The loop above iterates over a copy, and some of the callbacks might get re-added.

> 
> > Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:340
> > +    // FIXME: Only fire onVersionChange if there the connection isn't in
> > +    // the process of closing.
> > +    // http://crbug.com/100645
> 
> Nit: Would be nice to have webkit bugs here instead of chromium bugs.

done

> 
> > Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:345
> > +    // FIXME: Only fire onBlocked if there are open connections after the
> > +    // VersionChangeEvents are received, not just set up to fire.
> > +    // http://crbug.com/100123
> 
> ditto

done

> 
> > LayoutTests/storage/indexeddb/factory-deletedatabase-interactions.html:228
> > +var successfullyParsed = true;
> 
> Nit: After https://bugs.webkit.org/show_bug.cgi?id=70784 , this is no longer required.

done

> 
> > LayoutTests/storage/indexeddb/factory-deletedatabase.html:101
> > +var successfullyParsed = true;
> 
> Ditto.

done

[WebKit Review Bot]

Comment on attachment 112897 [details]
Patch

Rejecting attachment 112897 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
/indexeddb/factory-deletedatabase-interactions.html
patching file LayoutTests/storage/indexeddb/factory-deletedatabase.html
patching file LayoutTests/storage/indexeddb/open-close-version-expected.txt
patching file LayoutTests/storage/indexeddb/open-close-version.html
Hunk #1 succeeded at 137 (offset -1 lines).
Hunk #2 succeeded at 210 (offset -1 lines).
Hunk #3 succeeded at 290 (offset -1 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/10242126

Created attachment 112969 [details]
Patch

Comment on attachment 112969 [details]
Patch

rebased

[WebKit Review Bot]

Comment on attachment 112969 [details]
Patch

Clearing flags on attachment: 112969

Committed r98806: <http://trac.webkit.org/changeset/98806>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Hans Wennborg]

(In reply to comment #51)
> All reviewed patches have been landed.  Closing bug.

Woot! Thanks Jochen!