Bug 45386 – Complete index support for IndexedDB

• Component: New Bugs
• Status: RESOLVED FIXED

[Jeremy Orlow]

Complete index support for IndexedDB

[Jeremy Orlow]

Created attachment 66894 [details]
Patch

[Jeremy Orlow]

Don't worry: 3/4 of this is layout test related stuff.  :-)

[Marcus Bulach]

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]

Steve: I can has review?

[Steve Block]

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]

(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]

(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]

Created attachment 67544 [details]
Patch

[Andrei Popescu]

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 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]

Created attachment 67684 [details]
Patch

[Jeremy Orlow]

(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 on attachment 67684 [details]
Patch

r=me

[Jeremy Orlow]

Committed r67600: <http://trac.webkit.org/changeset/67600>

[WebKit Review Bot]

http://trac.webkit.org/changeset/67600 might have broken Chromium Win Release