IndexedDB: Numeric keys are floats.
Created attachment 75883 [details] Patch
Comment on attachment 75883 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=75883&action=review Looks great! > LayoutTests/storage/indexeddb/objectstore-cursor.html:19 > 1, Each one increases the size of the test exponentially. Let's just get rid of 1 and 2 maybe? Also add something to index-cursor. Replace 2 of the identical integers with identical doubles maybe? > WebCore/storage/IDBFactoryBackendImpl.cpp:127 > + if (!sqliteDatabase.returnsAtLeastOneResult("SELECT sql FROM SQLITE_MASTER WHERE type = 'table' AND tbl_name = 'ObjectStoreData' AND sql = 'CREATE TABLE ObjectStoreData (id INTEGER PRIMARY KEY, objectStoreId INTEGER NOT NULL REFERENCES ObjectStore(id), keyString TEXT, keyDate INTEGER, keyNumber INTEGER, value TEXT NOT NULL)'")) I wonder if this is too fragile. If some SQLite somewhere will not use this exact string or something like that. Maybe it is worth looking for a version table and having the migration create one and add a version number of 1? > WebCore/storage/IDBFactoryBackendImpl.cpp:134 > + "DROP TABLE ObjectStoreData", Add a comment that this depends on SQLite not enforcing the referential consistency. > WebCore/storage/IDBFactoryBackendImpl.cpp:140 > + if (!sqliteDatabase.executeCommand(commands[i])) { This all should be done in a transaction probably. > WebKit/chromium/public/WebIDBKey.h:-51 > - WebIDBKey(int32_t number) { assign(number); } So implicit type conversion is going to work without errors that break any of the builds?
> if (!sqliteDatabase.returnsAtLeastOneResult("SELECT sql FROM SQLITE_MASTER Ugh, this won't scale very well at all if we need another modification later. I'd definitely do what Jeremy suggested.
(In reply to comment #2) > (From update of attachment 75883 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=75883&action=review > > Looks great! > > > LayoutTests/storage/indexeddb/objectstore-cursor.html:19 > > 1, > > Each one increases the size of the test exponentially. Let's just get rid of 1 and 2 maybe? Done. > > Also add something to index-cursor. Replace 2 of the identical integers with identical doubles maybe? Done. > > > WebCore/storage/IDBFactoryBackendImpl.cpp:127 > > + if (!sqliteDatabase.returnsAtLeastOneResult("SELECT sql FROM SQLITE_MASTER WHERE type = 'table' AND tbl_name = 'ObjectStoreData' AND sql = 'CREATE TABLE ObjectStoreData (id INTEGER PRIMARY KEY, objectStoreId INTEGER NOT NULL REFERENCES ObjectStore(id), keyString TEXT, keyDate INTEGER, keyNumber INTEGER, value TEXT NOT NULL)'")) > > I wonder if this is too fragile. If some SQLite somewhere will not use this exact string or something like that. Maybe it is worth looking for a version table and having the migration create one and add a version number of 1? Doing this. I think this means we need to change the way createTables() work, though. Until now, we've been doing "CREATE TABLE IF NOT EXISTS foo ...", which looks like it is pretty fault tolerant, i.e. if one table or index is lost somehow (which doesn't really seem likely, it's more likely that it gets corrupted), it is re-created. With versioning, we can't really re-create tables like this, because if the database is using the schema of version 1, then we can't just create missing tables using schema from version 2, because that could cause trouble with migration. I think we have to trust that the tables are there, and fail if not. > > > WebCore/storage/IDBFactoryBackendImpl.cpp:134 > > + "DROP TABLE ObjectStoreData", > > Add a comment that this depends on SQLite not enforcing the referential consistency. Done. > > > WebCore/storage/IDBFactoryBackendImpl.cpp:140 > > + if (!sqliteDatabase.executeCommand(commands[i])) { > > This all should be done in a transaction probably. Done. > > > WebKit/chromium/public/WebIDBKey.h:-51 > > - WebIDBKey(int32_t number) { assign(number); } > > So implicit type conversion is going to work without errors that break any of the builds? Yes. Type conversion will keep the build from breaking. In multi-process mode this means non-integer keys won't work until the Chromium side is fixed, but running layout tests works fine. Assuming I thought of everything, that is :)
Created attachment 76053 [details] Patch
Comment on attachment 76053 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=76053&action=review > WebCore/storage/IDBFactoryBackendImpl.cpp:119 > + "INSERT INTO MetaData VALUES ('version', 2)", I think we should probably just keep this as it was originally and let the migrations bring it up to date so we dont' add antoher code path. > WebCore/storage/IDBFactoryBackendImpl.cpp:153 > + query.finalize(); Whats this? Should we be doing it elsewhere?
(In reply to comment #6) > (From update of attachment 76053 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=76053&action=review > > > WebCore/storage/IDBFactoryBackendImpl.cpp:119 > > + "INSERT INTO MetaData VALUES ('version', 2)", > > I think we should probably just keep this as it was originally and let the migrations bring it up to date so we dont' add antoher code path. Done. > > > WebCore/storage/IDBFactoryBackendImpl.cpp:153 > > + query.finalize(); > > Whats this? Should we be doing it elsewhere? Don't remember where I saw it, but now I realize it's unnecessary since it's done in the SQLiteStatment destructor. I realized we should migrate the IndexData table too. Doing that and tweaking the index-basics test to exercise it.
Created attachment 76067 [details] Patch
Comment on attachment 76067 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=76067&action=review > WebCore/storage/IDBFactoryBackendImpl.cpp:134 > + "BEGIN TRANSACTION", Use the SQLiteTransaction class instead? > WebCore/storage/IDBFactoryBackendImpl.cpp:170 > + "BEGIN TRANSACTION", Use the SQLiteTransaction class instead?
(In reply to comment #9) > (From update of attachment 76067 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=76067&action=review > > > WebCore/storage/IDBFactoryBackendImpl.cpp:134 > > + "BEGIN TRANSACTION", > > Use the SQLiteTransaction class instead? Okay.
Created attachment 76072 [details] Patch
Comment on attachment 76072 [details] Patch r=me
Committed r73616: <http://trac.webkit.org/changeset/73616>
http://trac.webkit.org/changeset/73616 might have broken Leopard Intel Release (Tests) The following tests are not passing: inspector/styles-source-offsets.html
This was rolled out in r73633 because it broke the windows build.
Committed r73697: <http://trac.webkit.org/changeset/73697>