121996 – IndexedDB IDL Refactoring

Bug 121996 - IndexedDB IDL Refactoring

Summary: IndexedDB IDL Refactoring

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	WebCore Misc. (show other bugs)
Version:	528+ (Nightly build)
Hardware:	All All

Importance:	P2 Normal
Assignee:	Brady Eidson

URL:
Keywords:

Depends on:
Blocks:

Reported:	2013-09-26 16:47 PDT by Brady Eidson
Modified:	2013-09-30 09:02 PDT (History)
CC List:	12 users (show)

See Also:

Attachments
Patch v1 (37.14 KB, patch) 2013-09-26 16:52 PDT, Brady Eidson	gtk-ews: commit-queue-	Details \| Formatted Diff \| Diff
Patch v2 - Minimal fixes to keep leveldb building (42.18 KB, patch) 2013-09-27 13:29 PDT, Brady Eidson	ap: review+ eflews.bot: commit-queue-	Details \| Formatted Diff \| Diff
Patch v3 - Give LevelDB ewses another shot (53.62 KB, patch) 2013-09-27 15:10 PDT, Brady Eidson	eflews.bot: commit-queue-	Details \| Formatted Diff \| Diff
Patch v4 - Now with more stdint.h (53.80 KB, patch) 2013-09-27 15:47 PDT, Brady Eidson	eflews.bot: commit-queue-	Details \| Formatted Diff \| Diff
Patch v5 - Now with more manual INT64_MAX (53.87 KB, patch) 2013-09-27 16:00 PDT, Brady Eidson	eflews.bot: commit-queue-	Details \| Formatted Diff \| Diff
Patch v6 - Glad their EWS is fast (54.69 KB, patch) 2013-09-27 16:21 PDT, Brady Eidson	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (5) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

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.