RESOLVED FIXED 44164
Implement persistance for IndexedDB ObjectStores
https://bugs.webkit.org/show_bug.cgi?id=44164
Summary Implement persistance for IndexedDB ObjectStores
Jeremy Orlow
Reported 2010-08-18 04:58:41 PDT
Implement persistance for IndexedDB ObjectStores
Attachments
Patch (26.06 KB, patch)
2010-08-18 07:18 PDT, Jeremy Orlow
no flags
Patch (28.88 KB, patch)
2010-08-18 08:08 PDT, Jeremy Orlow
no flags
Patch (29.07 KB, patch)
2010-08-18 09:17 PDT, Jeremy Orlow
no flags
Patch (29.46 KB, patch)
2010-08-18 10:18 PDT, Jeremy Orlow
no flags
Patch (29.83 KB, patch)
2010-08-19 05:11 PDT, Jeremy Orlow
steveblock: review+
Jeremy Orlow
Comment 1 2010-08-18 07:18:07 PDT
Jeremy Orlow
Comment 2 2010-08-18 08:08:16 PDT
Marcus Bulach
Comment 3 2010-08-18 08:38:57 PDT
nice stuff! few comments below: WebCore/storage/IDBDatabaseBackendImpl.cpp:101 + importObjectStores(); loadObjectStoreadMetadata() might be better.. "import" sounds like your transforming / dealing with actual data. WebCore/platform/sql/SQLiteStatement.cpp:265 + if (!m_statement) does the "{" rule apply per statement or per lines? should this be a block? WebCore/storage/IDBDatabaseBackendImpl.cpp:137 + insert.bindInt(3, (int)autoIncrement); are c-style casts allowed? WebCore/storage/IDBObjectStoreBackendImpl.cpp:206 + insert.bindInt(3, (int)unique); ditto WebCore/storage/IDBObjectStoreBackendImpl.cpp:262 + void IDBObjectStoreBackendImpl::importIndexes() loadIndexesMetadata() ? WebCore/storage/IDBFactoryBackendImpl.cpp:85 + "CREATE TABLE IF NOT EXISTS Indexes (id INTEGER PRIMARY KEY, name TEXT, keyPath TEXT, isUnique INTEGER)", shouldn't index have an FK to ObjectStores(id) ? also, is there "alternate key"? I suppose (name, objectStoreId) should be unique. WebCore/storage/IDBObjectStoreBackendImpl.cpp:66 + static String whereClause(IDBKey* key) passing IDBKeyType would be clearer (I thought this would be dealing with the actual value..) WebCore/storage/IDBObjectStoreBackendImpl.cpp:101 + SQLiteStatement query(sqliteDatabase(), "SELECT value FROM ObjectStoreData WHERE" + whereClause(key.get())); yeah, passing type would be clearer. also, adding the space after the WHERE and removing from the three places above would look simpler. WebCore/storage/IDBObjectStoreBackendImpl.cpp:111 + callbacks->onSuccess(SerializedScriptValue::createFromWire(query.getColumnText(0))); hmm, AFAICT this will break JSC, it doesn't provide "fromWire" :( WebCore/storage/IDBObjectStoreBackendImpl.cpp:169 + putQuery.bindText(4, value->toWireString()); ditto
Jeremy Orlow
Comment 4 2010-08-18 09:15:33 PDT
(In reply to comment #3) > nice stuff! > few comments below: > > WebCore/storage/IDBDatabaseBackendImpl.cpp:101 > + importObjectStores(); > loadObjectStoreadMetadata() might be better.. "import" sounds like your transforming / dealing with actual data. done > WebCore/platform/sql/SQLiteStatement.cpp:265 > + if (!m_statement) > does the "{" rule apply per statement or per lines? should this be a block? I'd agree normally, but the rest of the file does it this way. > WebCore/storage/IDBDatabaseBackendImpl.cpp:137 > + insert.bindInt(3, (int)autoIncrement); > are c-style casts allowed? fixed > WebCore/storage/IDBObjectStoreBackendImpl.cpp:206 > + insert.bindInt(3, (int)unique); > ditto fixed > WebCore/storage/IDBObjectStoreBackendImpl.cpp:262 > + void IDBObjectStoreBackendImpl::importIndexes() > loadIndexesMetadata() ? fixed > WebCore/storage/IDBFactoryBackendImpl.cpp:85 > + "CREATE TABLE IF NOT EXISTS Indexes (id INTEGER PRIMARY KEY, name TEXT, keyPath TEXT, isUnique INTEGER)", > shouldn't index have an FK to ObjectStores(id) ? > also, is there "alternate key"? I suppose (name, objectStoreId) should be unique. did foreign key...couldn't do the alternate key as far as I could tell (without making it be my primary key) > WebCore/storage/IDBObjectStoreBackendImpl.cpp:66 > + static String whereClause(IDBKey* key) > passing IDBKeyType would be clearer (I thought this would be dealing with the actual value..) done > WebCore/storage/IDBObjectStoreBackendImpl.cpp:101 > + SQLiteStatement query(sqliteDatabase(), "SELECT value FROM ObjectStoreData WHERE" + whereClause(key.get())); > yeah, passing type would be clearer. also, adding the space after the WHERE and removing from the three places above would look simpler. done > WebCore/storage/IDBObjectStoreBackendImpl.cpp:111 > + callbacks->onSuccess(SerializedScriptValue::createFromWire(query.getColumnText(0))); > hmm, AFAICT this will break JSC, it doesn't provide "fromWire" :( It will be a major change to implement that for JSC. Given that no one from JSC land seems to care about IndexedDB at the moment, we're going to punt on it. > WebCore/storage/IDBObjectStoreBackendImpl.cpp:169 > + putQuery.bindText(4, value->toWireString()); > ditto yup
Jeremy Orlow
Comment 5 2010-08-18 09:17:58 PDT
Andrei Popescu
Comment 6 2010-08-18 09:36:28 PDT
> CREATE TABLE IF NOT EXISTS ObjectStores (id INTEGER PRIMARY KEY, name TEXT, keyPath TEXT, doAutoIncrement INTEGER)" You are doing a delete based on name. Please create an index on 'name'. > CREATE TABLE IF NOT EXISTS Indexes (id INTEGER PRIMARY KEY, name TEXT, keyPath TEXT, isUnique INTEGER)" You may want to add a foreign key on ObjectStores and use cascade delete like so: objectStoreID INTEGER REFERENCES ObjectStore(id) ON DELETE CASCADE
Jeremy Orlow
Comment 7 2010-08-18 10:17:44 PDT
(In reply to comment #6) > > CREATE TABLE IF NOT EXISTS ObjectStores (id INTEGER PRIMARY KEY, name TEXT, keyPath TEXT, doAutoIncrement INTEGER)" > > You are doing a delete based on name. Please create an index on 'name'. > > > > CREATE TABLE IF NOT EXISTS Indexes (id INTEGER PRIMARY KEY, name TEXT, keyPath TEXT, isUnique INTEGER)" > > You may want to add a foreign key on ObjectStores and use cascade delete like so: > > objectStoreID INTEGER REFERENCES ObjectStore(id) ON DELETE CASCADE Added foreign keys, indexes, and fixed some style issue. Please recheck all SQL stuff in this.
Jeremy Orlow
Comment 8 2010-08-18 10:18:17 PDT
Andrei Popescu
Comment 9 2010-08-18 10:59:35 PDT
Umm, what happened to IndexData table? You had that in the previous patch.
Jeremy Orlow
Comment 10 2010-08-18 13:17:02 PDT
(In reply to comment #9) > Umm, what happened to IndexData table? You had that in the previous patch. Deleted since it's not used yet. Will re add in next patch.
Marcus Bulach
Comment 11 2010-08-19 03:07:19 PDT
(In reply to comment #10) > (In reply to comment #9) > > Umm, what happened to IndexData table? You had that in the previous patch. > > Deleted since it's not used yet. Will re add in next patch. looks fine by me, ignorable comment: WebCore/storage/IDBFactoryBackendImpl.cpp:93 + "DROP TABLE IF EXISTS ObjectStoreData", perhads drop "Data" ? it doesn't seem to add much.
Steve Block
Comment 12 2010-08-19 04:21:07 PDT
Comment on attachment 64727 [details] Patch WebCore/storage/IDBFactoryBackendImpl.cpp:81 + "CREATE TABLE IF NOT EXISTS MetaData (id INTEGER PRIMARY KEY, name TEXT, description TEXT, version TEXT)", What's this id used for? WebCore/storage/IDBObjectStoreBackendImpl.cpp:55 + loadIndexes(); Can we do this lazily? It looks like m_indexes isn't needed until script requests it? WebCore/storage/IDBObjectStoreBackendImpl.cpp:75 + return "keyString IS NULL AND keyDate IS NULL AND keyNumber IS NULL AND objectStoreId = ?"; Do we need the NULL tests for the 'other' columns (except maybe the 'NULL' type)? These should be NULL unless we have a logic error, right? If we want to test for logic errors, we could do that in an ASSERT/debug statement? WebCore/storage/IDBObjectStoreBackendImpl.cpp:66 + static String whereClause(IDBKey::Type type) When I first read this method, I assumed the returned clause would include 'WHERE'. I think that makes more sense. WebCore/storage/IDBObjectStoreBackendImpl.cpp:106 + query.bindInt64(2, m_id); Could you use constants for these column indexes? Especially as they're implementation details of whereClause(). WebCore/storage/IDBObjectStoreBackendImpl.cpp:207 + SQLiteStatement insert(sqliteDatabase(), "INSERT INTO Indexes (name, keyPath, isUnique) VALUES (?, ?, ?)"); This is missing the foreign key value
Jeremy Orlow
Comment 13 2010-08-19 04:38:22 PDT
Comment on attachment 64727 [details] Patch (In reply to comment #12) > (From update of attachment 64727 [details]) > WebCore/storage/IDBFactoryBackendImpl.cpp:81 > + "CREATE TABLE IF NOT EXISTS MetaData (id INTEGER PRIMARY KEY, name TEXT, description TEXT, version TEXT)", > What's this id used for? Nothing. But if you don't have one, SQLite will create one anyway and just leave it unnamed. So there's no reason not to. > WebCore/storage/IDBObjectStoreBackendImpl.cpp:55 > + loadIndexes(); > Can we do this lazily? It looks like m_indexes isn't needed until script requests it? No, because meta-data is accessed synchronously. > WebCore/storage/IDBObjectStoreBackendImpl.cpp:75 > + return "keyString IS NULL AND keyDate IS NULL AND keyNumber IS NULL AND objectStoreId = ?"; > Do we need the NULL tests for the 'other' columns (except maybe the 'NULL' type)? These should be NULL unless we have a logic error, right? If we want to test for logic errors, we could do that in an ASSERT/debug statement? Yeah, I guess that makes sense. > WebCore/storage/IDBObjectStoreBackendImpl.cpp:66 > + static String whereClause(IDBKey::Type type) > When I first read this method, I assumed the returned clause would include 'WHERE'. I think that makes more sense. k > WebCore/storage/IDBObjectStoreBackendImpl.cpp:106 > + query.bindInt64(2, m_id); > Could you use constants for these column indexes? Especially as they're implementation details of whereClause(). I don't see a point to using constants. They're tied to the SELECT statements directly above. Putting a constant at the beginning of the file makes no sense. Putting it next to the select seems only marginally better. > WebCore/storage/IDBObjectStoreBackendImpl.cpp:207 > + SQLiteStatement insert(sqliteDatabase(), "INSERT INTO Indexes (name, keyPath, isUnique) VALUES (?, ?, ?)"); > This is missing the foreign key value Indeed. Will post another version soon.
Steve Block
Comment 14 2010-08-19 04:41:32 PDT
> > WebCore/storage/IDBObjectStoreBackendImpl.cpp:106 > > + query.bindInt64(2, m_id); > > Could you use constants for these column indexes? Especially as they're implementation details of whereClause(). > > I don't see a point to using constants. They're tied to the SELECT statements > directly above. Putting a constant at the beginning of the file makes no > sense. Putting it next to the select seems only marginally better. My point is that they's tied to the implementation of whereClause(), so having a numbers like this elsewhere in the code seems brittle. Maybe put the constants next to whereClause()?
Jeremy Orlow
Comment 15 2010-08-19 04:43:47 PDT
(In reply to comment #14) > > > WebCore/storage/IDBObjectStoreBackendImpl.cpp:106 > > > + query.bindInt64(2, m_id); > > > Could you use constants for these column indexes? Especially as they're implementation details of whereClause(). > > > > I don't see a point to using constants. They're tied to the SELECT statements > > directly above. Putting a constant at the beginning of the file makes no > > sense. Putting it next to the select seems only marginally better. > My point is that they's tied to the implementation of whereClause(), so having a numbers like this elsewhere in the code seems brittle. Maybe put the constants next to whereClause()? Actually I see another problem with it as well. Will try to make it better.
Jeremy Orlow
Comment 16 2010-08-19 05:11:54 PDT
Steve Block
Comment 17 2010-08-19 07:12:52 PDT
Comment on attachment 64831 [details] Patch WebCore/storage/IDBObjectStoreBackendImpl.cpp:83 + static int bindKey(SQLiteStatement& query, int column, IDBKey* key) I'm not sure it makes much sense for this to be a separate method WebCore/storage/IDBObjectStoreBackendImpl.cpp:101 + static void bindWhereClause(SQLiteStatement& query, int64_t id, IDBKey* key) Nice. Is bindWhereClauseStatement() more clear?
Jeremy Orlow
Comment 18 2010-08-19 07:23:11 PDT
(In reply to comment #17) > (From update of attachment 64831 [details]) > WebCore/storage/IDBObjectStoreBackendImpl.cpp:83 > + static int bindKey(SQLiteStatement& query, int column, IDBKey* key) > I'm not sure it makes much sense for this to be a separate method In my next patch, I use it. :-) > WebCore/storage/IDBObjectStoreBackendImpl.cpp:101 > + static void bindWhereClause(SQLiteStatement& query, int64_t id, IDBKey* key) > Nice. Is bindWhereClauseStatement() more clear? Personally, I don't think so.
Jeremy Orlow
Comment 19 2010-08-19 07:38:56 PDT
WebKit Review Bot
Comment 20 2010-08-19 08:03:17 PDT
http://trac.webkit.org/changeset/65667 might have broken Chromium Mac Release
Note You need to log in before you can comment on or make changes to this bug.