RESOLVED FIXED 45386
Complete index support for IndexedDB
https://bugs.webkit.org/show_bug.cgi?id=45386
Summary Complete index support for IndexedDB
Jeremy Orlow
Reported 2010-09-08 06:47:25 PDT
Complete index support for IndexedDB
Attachments
Patch (158.21 KB, patch)
2010-09-08 07:14 PDT, Jeremy Orlow
no flags
Patch (155.64 KB, patch)
2010-09-14 06:24 PDT, Jeremy Orlow
no flags
Patch (156.53 KB, patch)
2010-09-15 09:37 PDT, Jeremy Orlow
steveblock: review+
Jeremy Orlow
Comment 1 2010-09-08 07:14:03 PDT
Jeremy Orlow
Comment 2 2010-09-08 09:54:28 PDT
Don't worry: 3/4 of this is layout test related stuff. :-)
Marcus Bulach
Comment 3 2010-09-09 05:44:47 PDT
nice stuff! overall looks fine by me, a few small comments below, nothing really major: View in context: https://bugs.webkit.org/attachment.cgi?id=66894&action=prettypatch > LayoutTests/storage/indexeddb/index-basics.html:98 > + result = evalAndLog("indexObject.getObject('value')"); could also test get/getObject for non existing keys. > LayoutTests/storage/indexeddb/index-cursor.html:149 > + while (upper > 0 && testData[upper] === testData[upper-1]) space around -/+ on afew places around here. > LayoutTests/storage/indexeddb/index-cursor.html:221 > + setTimeout(runNextTest, 0); is it needed or is testFailed terminal? > WebCore/storage/IDBIndexBackendImpl.cpp:61 > +static void openCursorInternal(SQLiteDatabase& database, IDBIndexBackendImpl* index, IDBKeyRange* range, unsigned short tmpDirection, bool objectCursor, IDBCallbacks* callbacks) perhaps sDirection (to mimic the prp style)? > WebCore/storage/IDBIndexBackendImpl.cpp:87 > + int currentColumn = 1; indexColumn? > WebCore/storage/IDBKey.cpp:92 > +String IDBKey::whereSyntax(String prefix) const perhaps s/prefix/qualifiedTableName/ ? > WebCore/storage/IDBKey.cpp:108 > +String IDBKey::leftCursorWhereFragment(String comparisonOperator, String prefix) ditto for prefix > WebCore/storage/IDBKeyRange.cpp:82 > + return "="; to avoid depending on the caller, I think "=" should be returned only if SINGLE, otherwise just an empty string. > WebCore/storage/IDBKeyRange.cpp:93 > + return "="; ditto > WebKit/chromium/public/WebIDBCursor.h:65 > + serializedScriptValue = value(); the diff is confusing, but looks like value() is calling value(serialized, key) which in turn calls value() again? granted, they're virtuals, but.. > WebKit/chromium/src/WebIDBCursorImpl.cpp:65 > + if (any->type() == IDBAny::SerializedScriptValueType) { remove {
Jeremy Orlow
Comment 4 2010-09-09 06:17:18 PDT
Steve: I can has review?
Steve Block
Comment 5 2010-09-10 03:11:14 PDT
Comment on attachment 66894 [details] Patch > +function cursorIteration() > +{ > + if (expectedIndex === null) { > + shouldBeNull("event.result"); > + setTimeout(runNextTest, 0); Does this need to be done asynchronously? I thought all the callbacks were asynchronous anyway, so the calls aren't recursive even without this? > diff --git a/LayoutTests/storage/indexeddb/script-tests/objectstore-basics.js b/LayoutTests/storage/indexeddb/script-tests/objectstore-basics.js > deleted file mode 100644 I thought all of the script-tests JS files had already gone? I guess this just needs a rebase? On a related note, I just noticed that the script-tests directory (though empty) is still in the tree. We should probably remove it. > void IDBAny::set(PassRefPtr<IDBIndex> value) > { > ASSERT(m_type == UndefinedType); > - m_type = IDBDatabaseType; > + m_type = IDBIndexType; Was this a bug? - just trying to understand the patch I certainly don't understand all the details of this. You can talk me through it if you like, or I trust Marcus' review.
Jeremy Orlow
Comment 6 2010-09-10 05:25:08 PDT
(In reply to comment #5) > (From update of attachment 66894 [details]) > > +function cursorIteration() > > +{ > > + if (expectedIndex === null) { > > + shouldBeNull("event.result"); > > + setTimeout(runNextTest, 0); > Does this need to be done asynchronously? I thought all the callbacks were asynchronous anyway, so the calls aren't recursive even without this? It doesn't, but I guess I figured it'd be more consistent that way. I don't think it matters much either way though. This code is mostly from the object store cursor test. > > diff --git a/LayoutTests/storage/indexeddb/script-tests/objectstore-basics.js b/LayoutTests/storage/indexeddb/script-tests/objectstore-basics.js > > deleted file mode 100644 > I thought all of the script-tests JS files had already gone? I guess this just needs a rebase? > > On a related note, I just noticed that the script-tests directory (though empty) is still in the tree. We should probably remove it. Hm...I deleted it in git. I guess we need a svn checkout to do the actual delete? > > void IDBAny::set(PassRefPtr<IDBIndex> value) > > { > > ASSERT(m_type == UndefinedType); > > - m_type = IDBDatabaseType; > > + m_type = IDBIndexType; > Was this a bug? - just trying to understand the patch Yeah, this was a bug not exposed previously. > I certainly don't understand all the details of this. You can talk me through it if you like, or I trust Marcus' review. More eyes are better. I'll walk you through.
Jeremy Orlow
Comment 7 2010-09-14 05:29:54 PDT
(In reply to comment #3) > nice stuff! overall looks fine by me, a few small comments below, nothing really major: All fixed. Except... > > WebKit/chromium/public/WebIDBCursor.h:65 > > + serializedScriptValue = value(); > the diff is confusing, but looks like value() is calling value(serialized, key) which in turn calls value() again? > granted, they're virtuals, but.. This is intentional and the only way it can work since the chromium side implements one version and the WebKit side implements the other. Will upload another in a moment.
Jeremy Orlow
Comment 8 2010-09-14 06:24:19 PDT
Andrei Popescu
Comment 9 2010-09-14 08:30:31 PDT
LGTM WebCore/storage/IDBFactoryBackendImpl.cpp:101 > CREATE TABLE IF NOT EXISTS IndexData (id INTEGER PRIMARY KEY, indexId INTEGER NOT NULL REFERENCES Indexs(id) The referenced table name is obviously a typo.
Jeremy Orlow
Comment 10 2010-09-15 07:17:45 PDT
Comment on attachment 67544 [details] Patch Reviewed with Steve in person. He had these few comments. Will fix momentarily. View in context: https://bugs.webkit.org/attachment.cgi?id=67544&action=prettypatch > WebCore/storage/IDBCursorBackendImpl.cpp:107 > + if (m_objectCursor) Need to do something if it's not an object cursor as well. > WebCore/storage/IDBCursorBackendImpl.h:82 > + bool m_objectCursor; m_isSerializedScriptValueCursor would be more clear > WebCore/storage/IDBIndexBackendImpl.cpp:131 > + ASSERT((key->type() == IDBKey::StringType) != query.isColumnNull(0)); If we delete these asserts, we don't need to select this stuff above. It might be simpler to remove.
Jeremy Orlow
Comment 11 2010-09-15 09:37:37 PDT
Jeremy Orlow
Comment 12 2010-09-15 09:38:25 PDT
(In reply to comment #10) > (From update of attachment 67544 [details]) > Reviewed with Steve in person. He had these few comments. Will fix momentarily. > > View in context: https://bugs.webkit.org/attachment.cgi?id=67544&action=prettypatch > > > WebCore/storage/IDBCursorBackendImpl.cpp:107 > > + if (m_objectCursor) > Need to do something if it's not an object cursor as well. Actually, this is just plain broken at the moment. For now, I changed it (and remove) to just error out. Will fix properly in a subsequent patch. > > WebCore/storage/IDBCursorBackendImpl.h:82 > > + bool m_objectCursor; > m_isSerializedScriptValueCursor would be more clear > > > WebCore/storage/IDBIndexBackendImpl.cpp:131 > > + ASSERT((key->type() == IDBKey::StringType) != query.isColumnNull(0)); > If we delete these asserts, we don't need to select this stuff above. It might be simpler to remove. done and done
Steve Block
Comment 13 2010-09-16 03:07:51 PDT
Comment on attachment 67684 [details] Patch r=me
Jeremy Orlow
Comment 14 2010-09-16 03:24:19 PDT
WebKit Review Bot
Comment 15 2010-09-16 03:54:17 PDT
http://trac.webkit.org/changeset/67600 might have broken Chromium Win Release
Note You need to log in before you can comment on or make changes to this bug.