Bug 62622 - Implement IDBFactory.deleteDatabase
: Implement IDBFactory.deleteDatabase
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-06-14 00:21 PST by
Modified: 2011-10-31 09:46 PST (History)


Attachments
Patch (21.39 KB, patch)
2011-06-14 00:23 PST, jochen@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff
Patch (28.42 KB, patch)
2011-08-18 08:17 PST, jochen@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff
Patch (28.43 KB, patch)
2011-08-18 08:28 PST, jochen@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff
Patch (29.86 KB, patch)
2011-08-19 04:12 PST, jochen@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff
Patch (42.38 KB, patch)
2011-10-17 17:08 PST, jochen@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff
Patch (44.33 KB, patch)
2011-10-18 12:06 PST, jochen@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff
Patch (44.02 KB, patch)
2011-10-18 16:28 PST, jochen@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff
Patch (47.73 KB, patch)
2011-10-25 10:29 PST, jochen@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff
Patch (47.74 KB, patch)
2011-10-25 11:16 PST, jochen@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff
Patch (47.75 KB, patch)
2011-10-27 08:22 PST, jochen@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff
Patch (47.94 KB, patch)
2011-10-28 00:33 PST, jochen@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff
Patch (47.88 KB, patch)
2011-10-28 03:05 PST, jochen@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff
Patch (51.89 KB, patch)
2011-10-28 12:55 PST, jochen@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff
Patch (51.98 KB, patch)
2011-10-29 08:11 PST, jochen@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2011-06-14 00:21:00 PST
Implement IDBFactory.deleteDatabase
------- Comment #1 From 2011-06-14 00:23:25 PST -------
Created an attachment (id=97079) [details]
Patch
------- Comment #2 From 2011-06-14 00:26:22 PST -------
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
------- Comment #3 From 2011-06-14 03:45:00 PST -------
(From update of attachment 97079 [details])
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.
------- Comment #4 From 2011-06-14 16:18:44 PST -------
(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?
------- Comment #5 From 2011-06-14 16:22:35 PST -------
(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
------- Comment #6 From 2011-06-14 16:27:38 PST -------
(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?
------- Comment #7 From 2011-06-16 06:50:49 PST -------
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.
------- Comment #8 From 2011-08-18 08:17:17 PST -------
Created an attachment (id=104344) [details]
Patch
------- Comment #9 From 2011-08-18 08:28:22 PST -------
Created an attachment (id=104346) [details]
Patch
------- Comment #10 From 2011-08-18 08:31:09 PST -------
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.
------- Comment #11 From 2011-08-19 02:46:00 PST -------
(From update of attachment 104346 [details])
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 #12 From 2011-08-19 02:55:35 PST -------
(From update of attachment 104346 [details])
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.
------- Comment #13 From 2011-08-19 04:12:03 PST -------
Created an attachment (id=104487) [details]
Patch
------- Comment #14 From 2011-08-19 04:12:42 PST -------
(From update of attachment 104487 [details])
Addressed all comments
------- Comment #15 From 2011-08-22 21:36:11 PST -------
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.
------- Comment #16 From 2011-09-06 12:53:00 PST -------
(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
------- Comment #17 From 2011-10-05 14:07:29 PST -------
*** Bug 62334 has been marked as a duplicate of this bug. ***
------- Comment #18 From 2011-10-05 14:13:57 PST -------
(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.
------- Comment #19 From 2011-10-05 14:29:48 PST -------
(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.
------- Comment #20 From 2011-10-17 17:08:42 PST -------
Created an attachment (id=111352) [details]
Patch
------- Comment #21 From 2011-10-17 17:11:31 PST -------
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?
------- Comment #22 From 2011-10-17 21:16:27 PST -------
(From update of attachment 111352 [details])
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
------- Comment #23 From 2011-10-18 12:06:50 PST -------
Created an attachment (id=111477) [details]
Patch
------- Comment #24 From 2011-10-18 15:04:59 PST -------
(From update of attachment 111477 [details])
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?
------- Comment #25 From 2011-10-18 16:28:03 PST -------
Created an attachment (id=111524) [details]
Patch
------- Comment #26 From 2011-10-18 16:30:08 PST -------
(From update of attachment 111477 [details])
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
------- Comment #27 From 2011-10-19 14:54:49 PST -------
(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

> 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.
------- Comment #28 From 2011-10-24 02:24:20 PST -------
(From update of attachment 111524 [details])
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.
------- Comment #29 From 2011-10-24 10:33:08 PST -------
(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
------- Comment #30 From 2011-10-25 10:29:57 PST -------
Created an attachment (id=112356) [details]
Patch
------- Comment #31 From 2011-10-25 10:31:34 PST -------
(In reply to comment #27)
> (From update of attachment 111524 [details] [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
------- Comment #32 From 2011-10-25 10:32:24 PST -------
(In reply to comment #29)
> (From update of attachment 111524 [details] [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
------- Comment #33 From 2011-10-25 11:16:07 PST -------
Created an attachment (id=112362) [details]
Patch
------- Comment #34 From 2011-10-27 07:57:32 PST -------
(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?

> 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.
------- Comment #35 From 2011-10-27 08:22:16 PST -------
Created an attachment (id=112687) [details]
Patch
------- Comment #36 From 2011-10-27 08:22:45 PST -------
(In reply to comment #34)
> (From update of attachment 112362 [details] [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.
------- Comment #37 From 2011-10-27 10:31:43 PST -------
(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
------- Comment #38 From 2011-10-27 10:49:23 PST -------
(In reply to comment #37)
> (From update of attachment 112687 [details] [details])
> Attachment 112687 [details] [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 :-/
------- Comment #39 From 2011-10-27 17:07:53 PST -------
(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.

> 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
------- Comment #40 From 2011-10-28 00:33:57 PST -------
Created an attachment (id=112823) [details]
Patch
------- Comment #41 From 2011-10-28 00:35:25 PST -------
(In reply to comment #39)
> (From update of attachment 112362 [details] [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
------- Comment #42 From 2011-10-28 01:12:54 PST -------
(From update of attachment 112823 [details])
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
------- Comment #43 From 2011-10-28 03:05:21 PST -------
Created an attachment (id=112841) [details]
Patch
------- Comment #44 From 2011-10-28 10:31:05 PST -------
(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?

> 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.
------- Comment #45 From 2011-10-28 12:55:24 PST -------
Created an attachment (id=112897) [details]
Patch
------- Comment #46 From 2011-10-28 12:56:41 PST -------
(In reply to comment #44)
> (From update of attachment 112841 [details] [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
------- Comment #47 From 2011-10-28 13:46:43 PST -------
(From update of attachment 112897 [details])
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
------- Comment #48 From 2011-10-29 08:11:02 PST -------
Created an attachment (id=112969) [details]
Patch
------- Comment #49 From 2011-10-29 08:12:02 PST -------
(From update of attachment 112969 [details])
rebased
------- Comment #50 From 2011-10-29 09:19:01 PST -------
(From update of attachment 112969 [details])
Clearing flags on attachment: 112969

Committed r98806: <http://trac.webkit.org/changeset/98806>
------- Comment #51 From 2011-10-29 09:19:09 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #52 From 2011-10-31 09:46:44 PST -------
(In reply to comment #51)
> All reviewed patches have been landed.  Closing bug.

Woot! Thanks Jochen!