[webkit-reviews] review denied: [Bug 50674] IndexedDB: Numeric keys are floats. : [Attachment 75883] Patch

[bugzilla-daemon at webkit.org]

Jeremy Orlow <jorlow at chromium.org> has denied Hans Wennborg
<hans at chromium.org>'s request for review:
Bug 50674: IndexedDB: Numeric keys are floats.
https://bugs.webkit.org/show_bug.cgi?id=50674

Attachment 75883: Patch
https://bugs.webkit.org/attachment.cgi?id=75883&action=review

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
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?