Bug 50674 - IndexedDB: Numeric keys are floats.
Summary: IndexedDB: Numeric keys are floats.
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: Hans Wennborg
URL:
Keywords:
Depends on: 50772
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-08 02:47 PST by Hans Wennborg
Modified: 2010-12-10 02:13 PST (History)
5 users (show)

See Also:


Attachments
Patch (87.73 KB, patch)
2010-12-08 03:12 PST, Hans Wennborg
no flags Details | Formatted Diff | Diff
Patch (22.40 KB, patch)
2010-12-09 05:41 PST, Hans Wennborg
no flags Details | Formatted Diff | Diff
Patch (27.04 KB, patch)
2010-12-09 08:16 PST, Hans Wennborg
no flags Details | Formatted Diff | Diff
Patch (27.19 KB, patch)
2010-12-09 09:01 PST, Hans Wennborg
jorlow: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hans Wennborg 2010-12-08 02:47:46 PST
IndexedDB: Numeric keys are floats.
Comment 1 Hans Wennborg 2010-12-08 03:12:37 PST
Created attachment 75883 [details]
Patch
Comment 2 Jeremy Orlow 2010-12-08 03:34:36 PST
Comment on attachment 75883 [details]
Patch

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

Looks great!

> LayoutTests/storage/indexeddb/objectstore-cursor.html:19
>      1,

Each one increases the size of the test exponentially.  Let's just get rid of 1 and 2 maybe?

Also add something to index-cursor.  Replace 2 of the identical integers with identical doubles maybe?

> WebCore/storage/IDBFactoryBackendImpl.cpp:127
> +    if (!sqliteDatabase.returnsAtLeastOneResult("SELECT sql FROM SQLITE_MASTER WHERE type = 'table' AND tbl_name = 'ObjectStoreData' AND sql = 'CREATE TABLE ObjectStoreData (id INTEGER PRIMARY KEY, objectStoreId INTEGER NOT NULL REFERENCES ObjectStore(id), keyString TEXT, keyDate INTEGER, keyNumber INTEGER, value TEXT NOT NULL)'"))

I wonder if this is too fragile.  If some SQLite somewhere will not use this exact string or something like that.  Maybe it is worth looking for a version table and having the migration create one and add a version number of 1?

> WebCore/storage/IDBFactoryBackendImpl.cpp:134
> +        "DROP TABLE ObjectStoreData",

Add a comment that this depends on SQLite not enforcing the referential consistency.

> WebCore/storage/IDBFactoryBackendImpl.cpp:140
> +        if (!sqliteDatabase.executeCommand(commands[i])) {

This all should be done in a transaction probably.

> WebKit/chromium/public/WebIDBKey.h:-51
> -    WebIDBKey(int32_t number) { assign(number); }

So implicit type conversion is going to work without errors that break any of the builds?
Comment 3 Andrei Popescu 2010-12-08 07:28:00 PST
> if (!sqliteDatabase.returnsAtLeastOneResult("SELECT sql FROM SQLITE_MASTER 

Ugh, this won't scale very well at all if we need another modification later. I'd definitely do what Jeremy suggested.
Comment 4 Hans Wennborg 2010-12-09 05:40:54 PST
(In reply to comment #2)
> (From update of attachment 75883 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75883&action=review
> 
> Looks great!
> 
> > LayoutTests/storage/indexeddb/objectstore-cursor.html:19
> >      1,
> 
> Each one increases the size of the test exponentially.  Let's just get rid of 1 and 2 maybe?
Done.

> 
> Also add something to index-cursor.  Replace 2 of the identical integers with identical doubles maybe?
Done.

> 
> > WebCore/storage/IDBFactoryBackendImpl.cpp:127
> > +    if (!sqliteDatabase.returnsAtLeastOneResult("SELECT sql FROM SQLITE_MASTER WHERE type = 'table' AND tbl_name = 'ObjectStoreData' AND sql = 'CREATE TABLE ObjectStoreData (id INTEGER PRIMARY KEY, objectStoreId INTEGER NOT NULL REFERENCES ObjectStore(id), keyString TEXT, keyDate INTEGER, keyNumber INTEGER, value TEXT NOT NULL)'"))
> 
> I wonder if this is too fragile.  If some SQLite somewhere will not use this exact string or something like that.  Maybe it is worth looking for a version table and having the migration create one and add a version number of 1?
Doing this.

I think this means we need to change the way createTables() work, though. Until now, we've been doing "CREATE TABLE IF NOT EXISTS foo ...", which looks like it is pretty fault tolerant, i.e. if one table or index is lost somehow (which doesn't really seem likely, it's more likely that it gets corrupted), it is re-created.

With versioning, we can't really re-create tables like this, because if the database is using the schema of version 1, then we can't just create missing tables using schema from version 2, because that could cause trouble with migration. I think we have to trust that the tables are there, and fail if not.


> 
> > WebCore/storage/IDBFactoryBackendImpl.cpp:134
> > +        "DROP TABLE ObjectStoreData",
> 
> Add a comment that this depends on SQLite not enforcing the referential consistency.
Done.

> 
> > WebCore/storage/IDBFactoryBackendImpl.cpp:140
> > +        if (!sqliteDatabase.executeCommand(commands[i])) {
> 
> This all should be done in a transaction probably.
Done.

> 
> > WebKit/chromium/public/WebIDBKey.h:-51
> > -    WebIDBKey(int32_t number) { assign(number); }
> 
> So implicit type conversion is going to work without errors that break any of the builds?
Yes.

Type conversion will keep the build from breaking. In multi-process mode this means non-integer keys won't work until the Chromium side is fixed, but running layout tests works fine.

Assuming I thought of everything, that is :)
Comment 5 Hans Wennborg 2010-12-09 05:41:22 PST
Created attachment 76053 [details]
Patch
Comment 6 Jeremy Orlow 2010-12-09 05:55:18 PST
Comment on attachment 76053 [details]
Patch

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

> WebCore/storage/IDBFactoryBackendImpl.cpp:119
> +        "INSERT INTO MetaData VALUES ('version', 2)",

I think we should probably just keep this as it was originally and let the migrations bring it up to date so we dont' add antoher code path.

> WebCore/storage/IDBFactoryBackendImpl.cpp:153
> +        query.finalize();

Whats this?  Should we be doing it elsewhere?
Comment 7 Hans Wennborg 2010-12-09 08:15:40 PST
(In reply to comment #6)
> (From update of attachment 76053 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76053&action=review
> 
> > WebCore/storage/IDBFactoryBackendImpl.cpp:119
> > +        "INSERT INTO MetaData VALUES ('version', 2)",
> 
> I think we should probably just keep this as it was originally and let the migrations bring it up to date so we dont' add antoher code path.
Done.

> 
> > WebCore/storage/IDBFactoryBackendImpl.cpp:153
> > +        query.finalize();
> 
> Whats this?  Should we be doing it elsewhere?
Don't remember where I saw it, but now I realize it's unnecessary since it's done in the SQLiteStatment destructor.


I realized we should migrate the IndexData table too. Doing that and tweaking the index-basics test to exercise it.
Comment 8 Hans Wennborg 2010-12-09 08:16:11 PST
Created attachment 76067 [details]
Patch
Comment 9 Jeremy Orlow 2010-12-09 08:22:46 PST
Comment on attachment 76067 [details]
Patch

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

> WebCore/storage/IDBFactoryBackendImpl.cpp:134
> +        "BEGIN TRANSACTION",

Use the SQLiteTransaction class instead?

> WebCore/storage/IDBFactoryBackendImpl.cpp:170
> +            "BEGIN TRANSACTION",

Use the SQLiteTransaction class instead?
Comment 10 Hans Wennborg 2010-12-09 09:01:05 PST
(In reply to comment #9)
> (From update of attachment 76067 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76067&action=review
> 
> > WebCore/storage/IDBFactoryBackendImpl.cpp:134
> > +        "BEGIN TRANSACTION",
> 
> Use the SQLiteTransaction class instead?
Okay.
Comment 11 Hans Wennborg 2010-12-09 09:01:34 PST
Created attachment 76072 [details]
Patch
Comment 12 Jeremy Orlow 2010-12-09 09:04:32 PST
Comment on attachment 76072 [details]
Patch

r=me
Comment 13 Hans Wennborg 2010-12-09 09:18:42 PST
Committed r73616: <http://trac.webkit.org/changeset/73616>
Comment 14 WebKit Review Bot 2010-12-09 10:52:59 PST
http://trac.webkit.org/changeset/73616 might have broken Leopard Intel Release (Tests)
The following tests are not passing:
inspector/styles-source-offsets.html
Comment 15 Hans Wennborg 2010-12-10 01:32:45 PST
This was rolled out in r73633 because it broke the windows build.
Comment 16 Hans Wennborg 2010-12-10 02:13:15 PST
Committed r73697: <http://trac.webkit.org/changeset/73697>