| Summary: | Implement persistance for IndexedDB ObjectStores | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jeremy Orlow <jorlow> | ||||||||||||
| Component: | New Bugs | Assignee: | Jeremy Orlow <jorlow> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | abarth, andreip, bulach, eric, steveblock, webkit.review.bot | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Other | ||||||||||||||
| OS: | OS X 10.5 | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Jeremy Orlow
2010-08-18 04:58:41 PDT
Created attachment 64703 [details]
Patch
Created attachment 64713 [details]
Patch
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
(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 Created attachment 64718 [details]
Patch
> 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 (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. Created attachment 64727 [details]
Patch
Umm, what happened to IndexData table? You had that in the previous patch. (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. (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. 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
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. > > 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()?
(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. Created attachment 64831 [details]
Patch
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?
(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. Committed r65667: <http://trac.webkit.org/changeset/65667> http://trac.webkit.org/changeset/65667 might have broken Chromium Mac Release |