WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(28.88 KB, patch)
2010-08-18 08:08 PDT
,
Jeremy Orlow
no flags
Details
Formatted Diff
Diff
Patch
(29.07 KB, patch)
2010-08-18 09:17 PDT
,
Jeremy Orlow
no flags
Details
Formatted Diff
Diff
Patch
(29.46 KB, patch)
2010-08-18 10:18 PDT
,
Jeremy Orlow
no flags
Details
Formatted Diff
Diff
Patch
(29.83 KB, patch)
2010-08-19 05:11 PDT
,
Jeremy Orlow
steveblock
: review+
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Jeremy Orlow
Comment 1
2010-08-18 07:18:07 PDT
Created
attachment 64703
[details]
Patch
Jeremy Orlow
Comment 2
2010-08-18 08:08:16 PDT
Created
attachment 64713
[details]
Patch
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
Created
attachment 64718
[details]
Patch
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
Created
attachment 64727
[details]
Patch
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
Created
attachment 64831
[details]
Patch
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
Committed
r65667
: <
http://trac.webkit.org/changeset/65667
>
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.
Top of Page
Format For Printing
XML
Clone This Bug