IndexedDB: backingStoreMap is per backing store, not per database
Created attachment 96597 [details] Patch
This fixes a bug introduced in r88358 which could cause the same LevelDB database to opened multiple times, and makes sure that it's possible to use a SQLite backing store even though a LevelDB one has been previously created.
Comment on attachment 96597 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96597&action=review > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:99 > + if (hasSQLBackingStore) { It looks like the old code checked to see if the migration already occurred. We don't need that anymore?
(In reply to comment #3) > (From update of attachment 96597 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96597&action=review > > > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:99 > > + if (hasSQLBackingStore) { > > It looks like the old code checked to see if the migration already occurred. We don't need that anymore? I think it makes more sense to always try to migrate if there is a SQLite backing store. When we flip the switch and make LevelDB the default, the migration test will have problems otherwise: - first some tests run with leveldb, - then the migration test writes some data to sqlite, - then it opens a leveldb backing store.. and since there already exists a leveldb backing store, it doesn't do migration.
(In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 96597 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=96597&action=review > > > > > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:99 > > > + if (hasSQLBackingStore) { > > > > It looks like the old code checked to see if the migration already occurred. We don't need that anymore? > > I think it makes more sense to always try to migrate if there is a SQLite backing store. Maybe I just don't know how the migration works. Is the sqlite database removed after migration runs? The case I'm thinking of is where a user: 1) visits an indexeddb-enabled site that stores stuff in sqlite 2) upgrades chrome to a version that uses leveldb 3) revisits that site: migration code runs and the site stores some more stuff in indexeddb/leveldb 4) user later revisits that site: original sqlite database is migrated, blowing away the new stuff stored in (3). > When we flip the switch and make LevelDB the default, the migration test will have problems otherwise: > > - first some tests run with leveldb, > - then the migration test writes some data to sqlite, > - then it opens a leveldb backing store.. and since there already exists a leveldb backing store, it doesn't do migration. Because DumpRenderTree would give all the tests the same origin?
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > (From update of attachment 96597 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=96597&action=review > > > > > > > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:99 > > > > + if (hasSQLBackingStore) { > > > > > > It looks like the old code checked to see if the migration already occurred. We don't need that anymore? > > > > I think it makes more sense to always try to migrate if there is a SQLite backing store. > > Maybe I just don't know how the migration works. Is the sqlite database removed after migration runs? The case I'm thinking of is where a user: Since there's no migration code landed yet, I think we're free to define the best way for it to work :) I'm thinking that removing the sqlite database after migration would make sense. Or have the migration code be smart enough to only migrate the databases/objectstores/data from sqlite which are not already present in leveldb. > > > When we flip the switch and make LevelDB the default, the migration test will have problems otherwise: > > > > - first some tests run with leveldb, > > - then the migration test writes some data to sqlite, > > > - then it opens a leveldb backing store.. and since there already exists a leveldb backing store, it doesn't do migration. > > Because DumpRenderTree would give all the tests the same origin? Exactly.
(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > (In reply to comment #3) > > > > (From update of attachment 96597 [details] [details] [details] [details]) > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=96597&action=review > > > > > > > > > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:99 > > > > > + if (hasSQLBackingStore) { > > > > > > > > It looks like the old code checked to see if the migration already occurred. We don't need that anymore? > > > > > > I think it makes more sense to always try to migrate if there is a SQLite backing store. > > > > Maybe I just don't know how the migration works. Is the sqlite database removed after migration runs? The case I'm thinking of is where a user: > > Since there's no migration code landed yet, I think we're free to define the best way for it to work :) > > I'm thinking that removing the sqlite database after migration would make sense. Or have the migration code be smart enough to only migrate the databases/objectstores/data from sqlite which are not already present in leveldb. It would not be difficult to migrate only object stores that do not exist. > > > > > When we flip the switch and make LevelDB the default, the migration test will have problems otherwise: > > > > > > - first some tests run with leveldb, > > > - then the migration test writes some data to sqlite, > > > > > - then it opens a leveldb backing store.. and since there already exists a leveldb backing store, it doesn't do migration. > > > > Because DumpRenderTree would give all the tests the same origin? > > Exactly.
(In reply to comment #7) > > I'm thinking that removing the sqlite database after migration would make sense. Or have the migration code be smart enough to only migrate the databases/objectstores/data from sqlite which are not already present in leveldb. > > > It would not be difficult to migrate only object stores that do not exist. That would be great
LGTM (In reply to comment #8) > (In reply to comment #7) > > > I'm thinking that removing the sqlite database after migration would make sense. Or have the migration code be smart enough to only migrate the databases/objectstores/data from sqlite which are not already present in leveldb. > > > > > > It would not be difficult to migrate only object stores that do not exist. > > That would be great Either of smart migration or delete-after-migrate sounds good to me.
Comment on attachment 96597 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96597&action=review > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:125 > + return false; // FIXME: To be implemented. If you want, could replace this with a notImplemented() call which will just log.
Committed r89130: <http://trac.webkit.org/changeset/89130>