Bug 62414 - IndexedDB: createObjectStore() name argument is required
Summary: IndexedDB: createObjectStore() name argument is required
Status: RESOLVED DUPLICATE of bug 63140
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-09 16:47 PDT by Mark Pilgrim (Google)
Modified: 2011-06-27 02:15 PDT (History)
5 users (show)

See Also:


Attachments
test case (1.91 KB, text/html)
2011-06-09 16:47 PDT, Mark Pilgrim (Google)
no flags Details
Patch (13.39 KB, patch)
2011-06-18 04:12 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (21.39 KB, patch)
2011-06-20 20:38 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (7.38 KB, patch)
2011-06-21 05:56 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Pilgrim (Google) 2011-06-09 16:47:47 PDT
Created attachment 96662 [details]
test case

http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#widl-IDBDatabase-createObjectStore-IDBObjectStore-DOMString-name-Object-optionalParameters states that the name argument is required and can not be null. This test calls createObjectStore with no name argument.

Expected behavior: throw IDBDatabaseException.NON_TRANSIENT_ERR
Actual behavior: does not throw, creates an object store with the 9-character name "undefined"

Test case attached.
Comment 1 Kentaro Hara 2011-06-18 04:12:02 PDT
Created attachment 97696 [details]
Patch
Comment 2 Kentaro Hara 2011-06-18 04:13:21 PDT
This patch includes the fix for bug 62415 also:
https://bugs.webkit.org/show_bug.cgi?id=62415
Comment 3 Dominic Cooney 2011-06-19 20:55:54 PDT
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 4 David Grogan 2011-06-20 01:37:22 PDT
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.
Comment 5 Kentaro Hara 2011-06-20 20:38:26 PDT
Created attachment 97922 [details]
Patch
Comment 6 Kentaro Hara 2011-06-20 20:40:40 PDT
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.
Comment 7 Kentaro Hara 2011-06-20 20:43:24 PDT
This patch includes the fix of |name| argument check for createObjectStore() (bug 62414), deleteObjectStore() (bug 62415), createIndex(), deleteIndex() and index().
Comment 8 David Grogan 2011-06-20 20:55:06 PDT
(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.
Comment 9 Mark Pilgrim (Google) 2011-06-20 21:50:30 PDT
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.
Comment 10 Mark Pilgrim (Google) 2011-06-21 05:56:40 PDT
Created attachment 97974 [details]
Patch
Comment 11 Kentaro Hara 2011-06-21 22:41:38 PDT
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?
Comment 12 Mark Pilgrim (Google) 2011-06-22 05:34:27 PDT
Aargh, no. Too many overlapping patches.

I'm going to make a consolidated patch that fixes everything in IDBDatabase.idl once and for all.
Comment 13 Mark Pilgrim (Google) 2011-06-22 08:13:00 PDT
Consolidating several bugs.

*** This bug has been marked as a duplicate of bug 63140 ***
Comment 14 Kentaro Hara 2011-06-23 18:15:49 PDT
> 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 15 Hajime Morrita 2011-06-27 02:14:53 PDT
Comment on attachment 97922 [details]
Patch

Marking obsolete as the bug closed.
Comment 16 Hajime Morrita 2011-06-27 02:15:12 PDT
Comment on attachment 97974 [details]
Patch

Marking obsolete as the bug closed.