Bug 45386 - Complete index support for IndexedDB
Summary: Complete index support for IndexedDB
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Jeremy Orlow
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-08 06:47 PDT by Jeremy Orlow
Modified: 2010-09-16 03:54 PDT (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Orlow 2010-09-08 06:47:25 PDT
Complete index support for IndexedDB
Comment 1 Jeremy Orlow 2010-09-08 07:14:03 PDT
Created attachment 66894 [details]
Patch
Comment 2 Jeremy Orlow 2010-09-08 09:54:28 PDT
Don't worry: 3/4 of this is layout test related stuff.  :-)
Comment 3 Marcus Bulach 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 {
Comment 4 Jeremy Orlow 2010-09-09 06:17:18 PDT
Steve: I can has review?
Comment 5 Steve Block 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.
Comment 6 Jeremy Orlow 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.
Comment 7 Jeremy Orlow 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.
Comment 8 Jeremy Orlow 2010-09-14 06:24:19 PDT
Created attachment 67544 [details]
Patch
Comment 9 Andrei Popescu 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.
Comment 10 Jeremy Orlow 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.
Comment 11 Jeremy Orlow 2010-09-15 09:37:37 PDT
Created attachment 67684 [details]
Patch
Comment 12 Jeremy Orlow 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
Comment 13 Steve Block 2010-09-16 03:07:51 PDT
Comment on attachment 67684 [details]
Patch

r=me
Comment 14 Jeremy Orlow 2010-09-16 03:24:19 PDT
Committed r67600: <http://trac.webkit.org/changeset/67600>
Comment 15 WebKit Review Bot 2010-09-16 03:54:17 PDT
http://trac.webkit.org/changeset/67600 might have broken Chromium Win Release