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
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.
(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
(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?
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.
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
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
*** Bug 62334 has been marked as a duplicate of this bug. ***
(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?
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
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
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.
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.
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
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.
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 :-/
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
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
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
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
Comment on attachment 112969 [details] Patch Clearing flags on attachment: 112969 Committed r98806: <http://trac.webkit.org/changeset/98806>
All reviewed patches have been landed. Closing bug.
(In reply to comment #51) > All reviewed patches have been landed. Closing bug. Woot! Thanks Jochen!