Bug 121996 – IndexedDB IDL Refactoring

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brady Eidson]

IndexedDB IDL Refactoring

This includes:
1 - Leveraging EventTarget.idl inheritance
2 - Removing comments
3 - Reordering to match the spec's IDLs
4 - Removing nonstandard, WebKit-specific methods
5 - Updating interfaces to match the spec (versions are only uint64_t's now...)
6 - Updating implementation code as needed (versions are only uint64_t's now...)

[Brady Eidson]

Created attachment 212768 [details]
Patch v1

[kov's GTK+ EWS bot]

Comment on attachment 212768 [details]
Patch v1

Attachment 212768 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/2613105

[EFL EWS Bot]

Comment on attachment 212768 [details]
Patch v1

Attachment 212768 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/2657088

[EFL EWS Bot]

Comment on attachment 212768 [details]
Patch v1

Attachment 212768 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/2649121

[Brady Eidson]

Created attachment 212835 [details]
Patch v2 - Minimal fixes to keep leveldb building

[WebKit Commit Bot]

Attachment 212835 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/Modules/indexeddb/IDBCallbacks.h', u'Source/WebCore/Modules/indexeddb/IDBCursor.idl', u'Source/WebCore/Modules/indexeddb/IDBDatabase.cpp', u'Source/WebCore/Modules/indexeddb/IDBDatabase.h', u'Source/WebCore/Modules/indexeddb/IDBDatabase.idl', u'Source/WebCore/Modules/indexeddb/IDBDatabaseCallbacks.h', u'Source/WebCore/Modules/indexeddb/IDBDatabaseCallbacksImpl.cpp', u'Source/WebCore/Modules/indexeddb/IDBDatabaseCallbacksImpl.h', u'Source/WebCore/Modules/indexeddb/IDBFactory.cpp', u'Source/WebCore/Modules/indexeddb/IDBFactory.h', u'Source/WebCore/Modules/indexeddb/IDBFactory.idl', u'Source/WebCore/Modules/indexeddb/IDBIndex.idl', u'Source/WebCore/Modules/indexeddb/IDBKeyRange.idl', u'Source/WebCore/Modules/indexeddb/IDBMetadata.h', u'Source/WebCore/Modules/indexeddb/IDBObjectStore.idl', u'Source/WebCore/Modules/indexeddb/IDBOpenDBRequest.cpp', u'Source/WebCore/Modules/indexeddb/IDBOpenDBRequest.h', u'Source/WebCore/Modules/indexeddb/IDBRequest.cpp', u'Source/WebCore/Modules/indexeddb/IDBRequest.h', u'Source/WebCore/Modules/indexeddb/IDBRequest.idl', u'Source/WebCore/Modules/indexeddb/IDBTransaction.cpp', u'Source/WebCore/Modules/indexeddb/IDBTransaction.h', u'Source/WebCore/Modules/indexeddb/IDBTransaction.idl', u'Source/WebCore/Modules/indexeddb/IDBVersionChangeEvent.cpp', u'Source/WebCore/Modules/indexeddb/IDBVersionChangeEvent.h', u'Source/WebCore/Modules/indexeddb/IDBVersionChangeEvent.idl', u'Source/WebCore/Modules/indexeddb/IndexedDB.h', u'Source/WebCore/Modules/indexeddb/leveldb/IDBBackingStoreLevelDB.cpp', u'Source/WebCore/Modules/indexeddb/leveldb/IDBLevelDBCoding.cpp']" exit_code: 1
Source/WebCore/ChangeLog:74:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:75:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 2 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EFL EWS Bot]

Comment on attachment 212835 [details]
Patch v2 - Minimal fixes to keep leveldb building

Attachment 212835 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/2692065

[Alexey Proskuryakov]

Comment on attachment 212835 [details]
Patch v2 - Minimal fixes to keep leveldb building

View in context: https://bugs.webkit.org/attachment.cgi?id=212835&action=review

Nice!

> Source/WebCore/Modules/indexeddb/IDBDatabaseCallbacksImpl.h:51
> -    void connect(IDBDatabase*);
> +    virtual void connect(IDBDatabase*);

Should this class be FINAL, and all virtual functions OVERRIDE? Not sure if the former implies the latter...

> Source/WebCore/Modules/indexeddb/IDBFactory.cpp:118
> +    IDB_TRACE("IDBFactory::open");

What's IDB_TRACE, and why is it not LOG()?

> Source/WebCore/Modules/indexeddb/IndexedDB.h:53
> +enum VersionNullness {

What's better, nullness or nullity?

Generally, having enums inside other headers results in including too much, and slowing down compilation, so we factor them out eventually. I don't know if that's a practical issue here.

> Source/WebCore/Modules/indexeddb/leveldb/IDBBackingStoreLevelDB.cpp:538
> +    // If the magic negative value is retrieved from the database

Unfinished comment?

[EFL EWS Bot]

Comment on attachment 212835 [details]
Patch v2 - Minimal fixes to keep leveldb building

Attachment 212835 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/2660237

[Brady Eidson]

(In reply to comment #8)
> (From update of attachment 212835 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212835&action=review
> 
> Nice!
> 
> > Source/WebCore/Modules/indexeddb/IDBDatabaseCallbacksImpl.h:51
> > -    void connect(IDBDatabase*);
> > +    virtual void connect(IDBDatabase*);
> 
> Should this class be FINAL, and all virtual functions OVERRIDE? Not sure if the former implies the latter...

I think it can be.

> > Source/WebCore/Modules/indexeddb/IDBFactory.cpp:118
> > +    IDB_TRACE("IDBFactory::open");
> 
> What's IDB_TRACE, and why is it not LOG()?

I have no idea - long standing code from Google.  =/

> > Source/WebCore/Modules/indexeddb/IndexedDB.h:53
> > +enum VersionNullness {
> 
> What's better, nullness or nullity?

I waffled back and forth.  Not sure.

> Generally, having enums inside other headers results in including too much, and slowing down compilation, so we factor them out eventually. I don't know if that's a practical issue here.

This one is legitimately needed from multiple compilation units *for now*

> > Source/WebCore/Modules/indexeddb/leveldb/IDBBackingStoreLevelDB.cpp:538
> > +    // If the magic negative value is retrieved from the database
> 
> Unfinished comment?

Yup!

[Brady Eidson]

I'll make the requested changes from review, and give another crack at keeping the leveldb build going.

[Brady Eidson]

Created attachment 212845 [details]
Patch v3 - Give LevelDB ewses another shot

[EFL EWS Bot]

Comment on attachment 212845 [details]
Patch v3 - Give LevelDB ewses another shot

Attachment 212845 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/2654221

[EFL EWS Bot]

Comment on attachment 212845 [details]
Patch v3 - Give LevelDB ewses another shot

Attachment 212845 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/2655237

[Brady Eidson]

(In reply to comment #14)
> (From update of attachment 212845 [details])
> Attachment 212845 [details] did not pass efl-ews (efl):
> Output: http://webkit-queues.appspot.com/results/2655237

I *think* I only botched the <stdint.h> include.

[Brady Eidson]

Created attachment 212847 [details]
Patch v4 - Now with more stdint.h

[EFL EWS Bot]

Comment on attachment 212847 [details]
Patch v4 - Now with more stdint.h

Attachment 212847 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/2660273

[Brady Eidson]

Created attachment 212849 [details]
Patch v5 - Now with more manual INT64_MAX

[EFL EWS Bot]

Comment on attachment 212849 [details]
Patch v5 - Now with more manual INT64_MAX

Attachment 212849 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/2692103

[Brady Eidson]

Created attachment 212851 [details]
Patch v6 - Glad their EWS is fast

[Brady Eidson]

Sweet, EWS build is happy.  I'll assume that's good for the other leveldb builds also.

Landed in http://trac.webkit.org/changeset/156590

[Zan Dobersek]

This regressed pretty badly on the GTK port where the IDB tests are run.
I understand this is a work in progress so I'll skip the tests until you enable them on the Mac port.

I'll point out two crash-inducing problems, but there are other problems as well that make most of the tests timing out.

[Zan Dobersek]

Comment on attachment 212851 [details]
Patch v6 - Glad their EWS is fast

View in context: https://bugs.webkit.org/attachment.cgi?id=212851&action=review

> Source/WebCore/Modules/indexeddb/IDBDatabase.idl:32
> +] interface IDBDatabase : EventTarget {

The JSGenerateToNativeObject and JSGenerateToJSObject attributes are required on the interface so the proper wrapper and unwrapper functions are generated that are then used in WebCore::toJS(ExecState*, JSDOMGlobalObject*, EventTarget*). If not generated, the EventTarget wrapper/unwrapper function is called recursively, which ends in a crash.

> Source/WebCore/Modules/indexeddb/IDBFactory.cpp:119
> +    return openInternal(context, name, 0, IndexedDB::NullVersion, ec);

IDBDatabaseMetadata::NoIntVersion should be passed into openInternal() here instead of 0.

[Brady Eidson]

(In reply to comment #23)
> (From update of attachment 212851 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212851&action=review
> 
> > Source/WebCore/Modules/indexeddb/IDBDatabase.idl:32
> > +] interface IDBDatabase : EventTarget {
> 
> The JSGenerateToNativeObject and JSGenerateToJSObject attributes are required on the interface so the proper wrapper and unwrapper functions are generated that are then used in WebCore::toJS(ExecState*, JSDOMGlobalObject*, EventTarget*). If not generated, the EventTarget wrapper/unwrapper function is called recursively, which ends in a crash.

Inheriting from EventTarget is definitely the way to go forward.  Thanks for skipping for now, and I'll definitely get to it soon once we get to the point of enabling tests across the board.

This is tangential enough to the refactoring/new code I'm working on that anybody with spare cycles is free to take a look in the meantime.

> > Source/WebCore/Modules/indexeddb/IDBFactory.cpp:119
> > +    return openInternal(context, name, 0, IndexedDB::NullVersion, ec);
> 
> IDBDatabaseMetadata::NoIntVersion should be passed into openInternal() here instead of 0.

That's not quite correct. The spec has changed and only int versions exist, and databases always start with a version of 0.  So while this line might currently be a bit wrong, the entire notion of having a string version is completely wrong.

The concept of the string version is so pervasive in the LevelDB implementation that I didn't try to get rid of it... but going forward somebody who lives on the LevelDB implementation will need to correctly factor out the string version altogether.  It has no place in the cross-platform bindings implementation at all.

[Joshua Bell]

(In reply to comment #24)
> 
> The concept of the string version is so pervasive in the LevelDB implementation that I didn't try to get rid of it... but going forward somebody who lives on the LevelDB implementation will need to correctly factor out the string version altogether.  It has no place in the cross-platform bindings implementation at all.

FYI, Chromium shipped IndexedDB with the older string-based versioning; once the spec changed the implementation needed to support both int and string versions so that web apps could migrate. Ports that didn't enable IDB before the change don't need this, so the string-versioning code can be nuked from WebKit.