Bug 121996

Summary: IndexedDB IDL Refactoring
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebCore Misc.Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, cdumez, commit-queue, eflews.bot, esprehn+autocc, gtk-ews, gyuyoung.kim, jsbell, kondapallykalyan, philn, xan.lopez, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch v1
gtk-ews: commit-queue-
Patch v2 - Minimal fixes to keep leveldb building
ap: review+, eflews.bot: commit-queue-
Patch v3 - Give LevelDB ewses another shot
eflews.bot: commit-queue-
Patch v4 - Now with more stdint.h
eflews.bot: commit-queue-
Patch v5 - Now with more manual INT64_MAX
eflews.bot: commit-queue-
Patch v6 - Glad their EWS is fast none

Description Brady Eidson 2013-09-26 16:47:37 PDT 
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...)
Comment 1 Brady Eidson 2013-09-26 16:52:24 PDT 
Created attachment 212768 [details]
Patch v1
Comment 2 kov's GTK+ EWS bot 2013-09-27 01:30:45 PDT 
Comment on attachment 212768 [details]
Patch v1

Attachment 212768 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/2613105
Comment 3 EFL EWS Bot 2013-09-27 02:32:02 PDT 
Comment on attachment 212768 [details]
Patch v1

Attachment 212768 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/2657088
Comment 4 EFL EWS Bot 2013-09-27 03:17:18 PDT 
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
Comment 5 Brady Eidson 2013-09-27 13:29:00 PDT 
Created attachment 212835 [details]
Patch v2 - Minimal fixes to keep leveldb building
Comment 6 WebKit Commit Bot 2013-09-27 13:30:15 PDT 
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.
Comment 7 EFL EWS Bot 2013-09-27 13:37:28 PDT 
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
Comment 8 Alexey Proskuryakov 2013-09-27 13:38:41 PDT 
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?
Comment 9 EFL EWS Bot 2013-09-27 13:50:29 PDT 
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
Comment 10 Brady Eidson 2013-09-27 14:25:38 PDT 
(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!
Comment 11 Brady Eidson 2013-09-27 14:26:41 PDT 
I'll make the requested changes from review, and give another crack at keeping the leveldb build going.
Comment 12 Brady Eidson 2013-09-27 15:10:39 PDT 
Created attachment 212845 [details]
Patch v3 - Give LevelDB ewses another shot
Comment 13 EFL EWS Bot 2013-09-27 15:15:08 PDT 
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
Comment 14 EFL EWS Bot 2013-09-27 15:15:28 PDT 
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
Comment 15 Brady Eidson 2013-09-27 15:46:46 PDT 
(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.
Comment 16 Brady Eidson 2013-09-27 15:47:43 PDT 
Created attachment 212847 [details]
Patch v4 - Now with more stdint.h
Comment 17 EFL EWS Bot 2013-09-27 15:54:45 PDT 
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
Comment 18 Brady Eidson 2013-09-27 16:00:56 PDT 
Created attachment 212849 [details]
Patch v5 - Now with more manual INT64_MAX
Comment 19 EFL EWS Bot 2013-09-27 16:16:18 PDT 
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
Comment 20 Brady Eidson 2013-09-27 16:21:14 PDT 
Created attachment 212851 [details]
Patch v6 - Glad their EWS is fast
Comment 21 Brady Eidson 2013-09-27 16:44:26 PDT 
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
Comment 22 Zan Dobersek 2013-09-29 10:26:04 PDT 
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.
Comment 23 Zan Dobersek 2013-09-29 10:31:24 PDT 
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.
Comment 24 Brady Eidson 2013-09-29 12:43:06 PDT 
(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.
Comment 25 Joshua Bell 2013-09-30 09:02:58 PDT 
(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.