Bug 62414

Created attachment 97696 [details] Patch

This patch includes the fix for bug 62415 also: https://bugs.webkit.org/show_bug.cgi?id=62415

Comment on attachment 97696 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97696&action=review > LayoutTests/storage/indexeddb/create-and-delete-object-store.html:18 > +function test() Do these tests need to use the moz- prefixed names? This is a WebKit test. Or is this how other tests in this suite do it? > LayoutTests/storage/indexeddb/create-and-delete-object-store.html:20 > + indexedDB = evalAndLog("indexedDB = window.indexedDB || window.webkitIndexedDB || window.mozIndexedDB;"); I think you need less evalAndLog here. Just log results that are pertinent to the test. > LayoutTests/storage/indexeddb/create-and-delete-object-store.html:58 > + debug("objectStore.name.length is " + store.name.length); This feels redundant. You could just use shouldBe and test that the string has the right value and length in one go. > LayoutTests/storage/indexeddb/create-and-delete-object-store.html:76 > + evalAndExpectException("db.deleteObjectStore()", "IDBDatabaseException.NON_TRANSIENT_ERR"); How is this different to what you’re testing on the previous line? > LayoutTests/storage/indexeddb/create-and-delete-object-store.html:94 > + store = evalAndLog("store = db.createObjectStore('undefined')"); I think this test would be more readable if you did something like: var validNames = ['null', 'undefined', 'valid_name']; validNames.forEach(function (name) { // same assertions in here }, this); to emphasize that these should behave the same. It will make the test shorter, too.

Comment on attachment 97696 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97696&action=review >> LayoutTests/storage/indexeddb/create-and-delete-object-store.html:18 >> +function test() > > Do these tests need to use the moz- prefixed names? This is a WebKit test. Or is this how other tests in this suite do it? This is how other indexeddb tests do it too. I started asking for this after trying to use our layout tests to see how mozilla interpreted parts of the spec. Though it's only really useful if all the type assignments do it, not just this first one.

Created attachment 97922 [details] Patch

Comment on attachment 97696 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97696&action=review >> LayoutTests/storage/indexeddb/create-and-delete-object-store.html:20 >> + indexedDB = evalAndLog("indexedDB = window.indexedDB || window.webkitIndexedDB || window.mozIndexedDB;"); > > I think you need less evalAndLog here. Just log results that are pertinent to the test. This is how other indexeddb tests do it too. I would like to wait other reviewers' opinions. >> LayoutTests/storage/indexeddb/create-and-delete-object-store.html:58 >> + debug("objectStore.name.length is " + store.name.length); > > This feels redundant. You could just use shouldBe and test that the string has the right value and length in one go. Done. >> LayoutTests/storage/indexeddb/create-and-delete-object-store.html:76 >> + evalAndExpectException("db.deleteObjectStore()", "IDBDatabaseException.NON_TRANSIENT_ERR"); > > How is this different to what you’re testing on the previous line? Done. Removed this line. >> LayoutTests/storage/indexeddb/create-and-delete-object-store.html:94 >> + store = evalAndLog("store = db.createObjectStore('undefined')"); > > I think this test would be more readable if you did something like: > > var validNames = ['null', 'undefined', 'valid_name']; > validNames.forEach(function (name) { > // same assertions in here > }, this); > > to emphasize that these should behave the same. It will make the test shorter, too. Done. > Source/WebCore/storage/IDBDatabase.cpp:82 > + if (name.isNull() || name.isEmpty()) { I removed name.isEmpty() in the latest patch, since an empty string is a valid keypath according to the spec.

This patch includes the fix of |name| argument check for createObjectStore() (bug 62414), deleteObjectStore() (bug 62415), createIndex(), deleteIndex() and index().

(In reply to comment #6) > (From update of attachment 97696 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97696&action=review > > >> LayoutTests/storage/indexeddb/create-and-delete-object-store.html:20 > >> + indexedDB = evalAndLog("indexedDB = window.indexedDB || window.webkitIndexedDB || window.mozIndexedDB;"); > > > > I think you need less evalAndLog here. Just log results that are pertinent to the test. > > This is how other indexeddb tests do it too. I would like to wait other reviewers' opinions. Most of our recent tests have that stuff. It doesn't really matter though.

Terribly sorry to throw a wrench in the works, but my original bug report was mistaken. As per WebIDL, the expected behavior here is to throw a TypeError, not a database exception. The proper place to fix it is the IDL file.

Created attachment 97974 [details] Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97974&action=review > LayoutTests/storage/indexeddb/setVersion-undefined-expected.txt:17 > +PASS db.setVersion(null); threw exception Error: NON_TRANSIENT_ERR: DOM IDBDatabase Exception 2. Is it correct to throw NON_TRANSIENT_ERR instead of TypeError when the argument is null?

Aargh, no. Too many overlapping patches. I'm going to make a consolidated patch that fixes everything in IDBDatabase.idl once and for all.

Consolidating several bugs. *** This bug has been marked as a duplicate of bug 63140 ***

> I'm going to make a consolidated patch that fixes everything in IDBDatabase.idl once and for all. OK. I will stop to upload a patch for this bug. >> Is it correct to throw NON_TRANSIENT_ERR instead of TypeError when the argument is null? > > Aargh, no. [1] Do you have any idea about how to throw TypeError when 'Nullable' name is passed to createObjectStore()? As far as I know, there seems to be no convenient IDL option (like [ConvertUndefinedOrNullToNullString] or [StrictTypeChecking]...) that helps us throw TypeError for a 'Nullable' argument. Another possible way is to throw TypeError at the head of IDBDatabase::createObjectStore(), like this: PassRefPtr<IDBObjectStore> IDBDatabase::createObjectStore(String& name, ...) { ...; if (name.isNull()) { ec = TypeErrorExceptionCode; return 0; } } However, I could not find the exception code for TypeError anywhere in xxxxxxException.h. Maybe do we have to write a code for throwing TypeError for each JavaScript engine??? [2] According to the spec (http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#widl-IDBDatabase-createObjectStore-IDBObjectStore-DOMString-name-Object-optionalParameters), it just says name must not be 'Nullable', and seems not to say which exception should be thrown. Why did you think we should throw TypeError in this case? (TypeError makes sense to me, but I would like to confirm the spec that supports our implementation.) I am asking this since I am encountering the same problem at the patch for bug 62288.

Comment on attachment 97922 [details] Patch Marking obsolete as the bug closed.

Comment on attachment 97974 [details] Patch Marking obsolete as the bug closed.