WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(155.64 KB, patch)
2010-09-14 06:24 PDT
,
Jeremy Orlow
no flags
Details
Formatted Diff
Diff
Patch
(156.53 KB, patch)
2010-09-15 09:37 PDT
,
Jeremy Orlow
steveblock
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Jeremy Orlow
Comment 1
2010-09-08 07:14:03 PDT
Created
attachment 66894
[details]
Patch
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
Created
attachment 67544
[details]
Patch
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
Created
attachment 67684
[details]
Patch
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
Committed
r67600
: <
http://trac.webkit.org/changeset/67600
>
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.
Top of Page
Format For Printing
XML
Clone This Bug