Bug 157520 – Change IDBObjectStore.createIndex to take an IDL dictionary

• Component: Bindings
• Status: RESOLVED FIXED

[Darin Adler]

Change IDBObjectStore.createIndex to take an IDL dictionary

[Darin Adler]

Created attachment 278501 [details]
Patch

[Darin Adler]

Haven’t finished rebuilding and running all the tests locally after rebasing, so there may be some surprise with EWS.

[Build Bot]

Comment on attachment 278501 [details]
Patch

Attachment 278501 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1299308

New failing tests:
storage/indexeddb/modern/create-index-failures-private.html
storage/indexeddb/index-basics.html
storage/indexeddb/keypath-basics.html
storage/indexeddb/modern/create-index-failures.html
storage/indexeddb/index-basics-private.html
storage/indexeddb/keypath-basics-private.html
storage/indexeddb/index-basics-workers.html

[Build Bot]

Created attachment 278508 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 278501 [details]
Patch

Attachment 278501 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1299314

New failing tests:
storage/indexeddb/modern/create-index-failures-private.html
storage/indexeddb/index-basics.html
storage/indexeddb/keypath-basics.html
storage/indexeddb/modern/create-index-failures.html
storage/indexeddb/index-basics-private.html
storage/indexeddb/keypath-basics-private.html
storage/indexeddb/index-basics-workers.html

[Build Bot]

Created attachment 278511 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 278501 [details]
Patch

Attachment 278501 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1299331

New failing tests:
storage/indexeddb/modern/create-index-failures-private.html
storage/indexeddb/index-basics.html
storage/indexeddb/modern/create-index-failures.html
storage/indexeddb/keypath-basics.html
storage/indexeddb/keypath-basics-private.html
storage/indexeddb/index-basics-workers.html
storage/indexeddb/index-basics-private.html

[Build Bot]

Created attachment 278512 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Darin Adler]

Created attachment 278579 [details]
Patch

[Darin Adler]

I expect this version to pass EWS given what I saw in my local testing.

[Brady Eidson]

Comment on attachment 278579 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBObjectStore.h:113
> -    // ActiveDOMObject
> -    const char* activeDOMObjectName() const;
> -    bool canSuspendForDocumentSuspension() const;
> -    bool hasPendingActivity() const;
> +    const char* activeDOMObjectName() const final;
> +    bool canSuspendForDocumentSuspension() const final;
> +    bool hasPendingActivity() const final;

By making the entire class final, aren't these finals not needed?

> LayoutTests/ChangeLog:32
> +        * storage/indexeddb/modern/resources/create-index-failures.js: Removed the test
> +        that expects failure when null is passed for the name and key path. In both
> +        cases, the Web IDL and IDB specifications call for the null value to be converted
> +        to the string "null", not an exception.

I'll trust you on the null name thing, but am not yet convinced you're right on the keyPath. From the published v1 IDB spec:
"If keyPath is not a valid key path, the implementation must throw a DOMException of type SyntaxError."

IDB further goes on to define "valid key path":
A valid key path is one of:
An empty DOMString.
An identifier, which is a DOMString matching the IdentifierName production from the ECMAScript Language Specification [ECMA-262].
A DOMString consisting of two or more identifiers separated by periods (ASCII character code 46).
A non-empty sequence<DOMString> containing only DOMStrings conforming to the above requirements.

The JS value null is not one of those 4.

WebIDL may define a default conversion for DOMStrings, but I doubt it defines a conversion for IDBKeyPath.

In addition to trying to reconcile the specs, I also urge testing what the other browsers do, as compatibility with what ships is likely more important than two specs that often butt heads.

> LayoutTests/ChangeLog:39
> +        * storage/indexeddb/resources/keypath-basics.js:
> +        (prepareDatabase): Added tests for both undefined and null. Both are legal values for
> +        the key path argument to createIndex. The Web IDL and IDB specifications call for
> +        them to be converted to the strings "undefined" and "null", not to trigger exceptions.
> +        (testInvalidKeyPaths): Removed tests that expect exceptions when calling createIndex
> +        with undefined and null.

Per my above comment, I'm not sure I agree here. I have not looked at WebIDL, but the v1 IDB spec is pretty clear on what valid key paths are.

[Darin Adler]

Comment on attachment 278579 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/IDBObjectStore.h:113
>> +    bool hasPendingActivity() const final;
> 
> By making the entire class final, aren't these finals not needed?

We discussed this on the mailing list when talking about our virtual vs. override vs. final change.

The idea is that we write either override or final when overriding rather than virtual. When we can write either, final is preferred. It’s true that if the class is marked final, then all the final keywords on the individual functions have no effect different than override, but if refactoring causes us to add a derived class, and we remove final from the class, it’s good for efficiency to have functions be marked final unless we want to override them further.

So the guideline is to always use final unless are overriding and plan to override further.

>> LayoutTests/ChangeLog:32
>> +        to the string "null", not an exception.
> 
> I'll trust you on the null name thing, but am not yet convinced you're right on the keyPath. From the published v1 IDB spec:
> "If keyPath is not a valid key path, the implementation must throw a DOMException of type SyntaxError."
> 
> IDB further goes on to define "valid key path":
> A valid key path is one of:
> An empty DOMString.
> An identifier, which is a DOMString matching the IdentifierName production from the ECMAScript Language Specification [ECMA-262].
> A DOMString consisting of two or more identifiers separated by periods (ASCII character code 46).
> A non-empty sequence<DOMString> containing only DOMStrings conforming to the above requirements.
> 
> The JS value null is not one of those 4.
> 
> WebIDL may define a default conversion for DOMStrings, but I doubt it defines a conversion for IDBKeyPath.
> 
> In addition to trying to reconcile the specs, I also urge testing what the other browsers do, as compatibility with what ships is likely more important than two specs that often butt heads.

I’m not going to be testing other browsers at this time--someone else would need to help me with that--but I am absolutely convinced this is correctly matching the specification.

Start with what the Indexed DB specification says about the type of the createIndex argument; not how it’s processed, but how it’s passed to the function. I’m referring to <http://www.w3.org/TR/IndexedDB/>, the “W3C Recommendation 08 January 2015” version. The type of this argument to createIndex is:

    “(DOMString or sequence<DOMString>) keyPath”

Note that the type of this argument is not “key path”; it’s a union of DOMString and sequence<DOMString> that is then interpreted as a key path by the rules you cited above. Now lets look at what WebIDL <http://www.w3.org/TR/WebIDL/>, the "W3C Candidate Recommendation 19 April 2012" version, says about union types in JavaScript. To convert a union type if the value is null or undefined, and the union does not include a nullable type, then the first rule that applies is the one that says:

    “If types includes DOMString or an enumeration type, then return the result of converting V to that type.”

Then we need to look at how you convert null or undefined to a DOMString. That section of the WebIDL specification lists special cases that turn null and undefined into empty strings if there are extended attributes, but those attributes are not specified here, and then refers us to the JavaScript specification’s ToString algorithm. In the JavaScript specification it says that to make a string from these values, we convert null to "null" and undefined to "undefined".

So it’s incorrect for us to say that null is not a DOMString; when passed to this function it is converted to a DOMString. And "null" is an identifier that matches the IdentifierName production from the ECMAScript language specification, so it’s a valid key path. Same for undefined.

>> LayoutTests/ChangeLog:39
>> +        with undefined and null.
> 
> Per my above comment, I'm not sure I agree here. I have not looked at WebIDL, but the v1 IDB spec is pretty clear on what valid key paths are.

Yes, what you cited above was correct, but our interpretation of what that means when we pass a null or undefined from JavaScript was not correct.

[Darin Adler]

I also looked at future drafts of all these specifications and none of this has changed in the latest working drafts of all of them.

[Chris Dumez]

Based on the Web IDL spec, I agree with Darin that passing null to a parameter defined as “(DOMString or sequence<DOMString>) keyPath” should be identical as passing the string "null".

[Brady Eidson]

As far as I recall, our current behavior on this test matches both Chrome and Firefox, meaning this would be a compatibility break.

I don't think we should land this until we run targeted tests in the other browsers and figure out if I'm wrong.

[Chris Dumez]

(In reply to comment #15)
> As far as I recall, our current behavior on this test matches both Chrome
> and Firefox, meaning this would be a compatibility break.
> 
> I don't think we should land this until we run targeted tests in the other
> browsers and figure out if I'm wrong.

Chrome:


Key paths which are never valid:
Expecting exception from db.createObjectStore('name').createIndex('name', null)
FAIL No exception thrown! Should have been DOMException.SYNTAX_ERR
Deleted all object stores.
Expecting exception from db.createObjectStore('name').createIndex('name', undefined)
FAIL No exception thrown! Should have been DOMException.SYNTAX_ERR
Deleted all object stores.
PASS successfullyParsed is true

So Chrome does not throw when passing null or undefined, which I believe is right.

[Chris Dumez]

(In reply to comment #16)
> (In reply to comment #15)
> > As far as I recall, our current behavior on this test matches both Chrome
> > and Firefox, meaning this would be a compatibility break.
> > 
> > I don't think we should land this until we run targeted tests in the other
> > browsers and figure out if I'm wrong.
> 
> Chrome:
> 
> 
> Key paths which are never valid:
> Expecting exception from db.createObjectStore('name').createIndex('name',
> null)
> FAIL No exception thrown! Should have been DOMException.SYNTAX_ERR
> Deleted all object stores.
> Expecting exception from db.createObjectStore('name').createIndex('name',
> undefined)
> FAIL No exception thrown! Should have been DOMException.SYNTAX_ERR
> Deleted all object stores.
> PASS successfullyParsed is true
> 
> So Chrome does not throw when passing null or undefined, which I believe is
> right.

Firefox:

Key paths which are never valid:
Expecting exception from db.createObjectStore('name').createIndex('name', null)
FAIL No exception thrown! Should have been DOMException.SYNTAX_ERR
Deleted all object stores.
Expecting exception from db.createObjectStore('name').createIndex('name', undefined)
FAIL No exception thrown! Should have been DOMException.SYNTAX_ERR
Deleted all object stores.
PASS successfullyParsed is true

happy?

[Brady Eidson]

I further confirmed that `null` and `undefined` transform to the strings "null" and "undefined" in the others, as well.

I withdraw my object on compatibility grounds since there are none!

[Chris Dumez]

(In reply to comment #18)
> I further confirmed that `null` and `undefined` transform to the strings
> "null" and "undefined" in the others, as well.
> 
> I withdraw my object on compatibility grounds since there are none!

My tests showed the same thing in Chrome and Firefox, they use "null" and "undefined".

[Chris Dumez]

Comment on attachment 278579 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 278579 [details]
Patch

Clearing flags on attachment: 278579

Committed r200699: <http://trac.webkit.org/changeset/200699>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

It looks like this change broke the Windows build:
<https://build.webkit.org/builders/Apple%20Win%20Release%20(Build)/builds/77647>

C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/PassRefPtr.h(42): error C2027: use of undefined type 'WebCore::DOMWrapperWorld' (compiling source file C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\bindings\js\JSIDBObjectStoreCustom.cpp) [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
  c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\bindings\js\JSDOMGlobalObject.h(39): note: see declaration of 'WebCore::DOMWrapperWorld' (compiling source file C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\bindings\js\JSIDBObjectStoreCustom.cpp)
  C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/RefPtr.h(59): note: see reference to function template instantiation 'void WTF::derefIfNotNull<T>(T *)' being compiled
          with
          [
              T=WebCore::DOMWrapperWorld
          ] (compiling source file C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\bindings\js\JSIDBObjectStoreCustom.cpp)
  C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/RefPtr.h(59): note: while compiling class template member function 'WTF::RefPtr<WebCore::DOMWrapperWorld>::~RefPtr(void)' (compiling source file C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\bindings\js\JSIDBObjectStoreCustom.cpp)
  c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\bindings\js\JSDOMGlobalObject.h(98): note: see reference to function template instantiation 'WTF::RefPtr<WebCore::DOMWrapperWorld>::~RefPtr(void)' being compiled (compiling source file C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\bindings\js\JSIDBObjectStoreCustom.cpp)
  c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\bindings\js\JSDOMGlobalObject.h(90): note: see reference to class template instantiation 'WTF::RefPtr<WebCore::DOMWrapperWorld>' being compiled (compiling source file C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\bindings\js\JSIDBObjectStoreCustom.cpp)
C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/PassRefPtr.h(42): error C2227: left of '->deref' must point to class/struct/union/generic type (compiling source file C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\bindings\js\JSIDBObjectStoreCustom.cpp) [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]

[Chris Dumez]

(In reply to comment #23)
> It looks like this change broke the Windows build:
> <https://build.webkit.org/builders/Apple%20Win%20Release%20(Build)/builds/
> 77647>
> 
> C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/
> PassRefPtr.h(42): error C2027: use of undefined type
> 'WebCore::DOMWrapperWorld' (compiling source file
> C:\cygwin\home\buildbot\slave\win-
> release\build\Source\WebCore\bindings\js\JSIDBObjectStoreCustom.cpp)
> [C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
>  
> c:\cygwin\home\buildbot\slave\win-
> release\build\source\webcore\bindings\js\JSDOMGlobalObject.h(39): note: see
> declaration of 'WebCore::DOMWrapperWorld' (compiling source file
> C:\cygwin\home\buildbot\slave\win-
> release\build\Source\WebCore\bindings\js\JSIDBObjectStoreCustom.cpp)
>  
> C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/
> RefPtr.h(59): note: see reference to function template instantiation 'void
> WTF::derefIfNotNull<T>(T *)' being compiled
>           with
>           [
>               T=WebCore::DOMWrapperWorld
>           ] (compiling source file
> C:\cygwin\home\buildbot\slave\win-
> release\build\Source\WebCore\bindings\js\JSIDBObjectStoreCustom.cpp)
>  
> C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/
> RefPtr.h(59): note: while compiling class template member function
> 'WTF::RefPtr<WebCore::DOMWrapperWorld>::~RefPtr(void)' (compiling source
> file
> C:\cygwin\home\buildbot\slave\win-
> release\build\Source\WebCore\bindings\js\JSIDBObjectStoreCustom.cpp)
>  
> c:\cygwin\home\buildbot\slave\win-
> release\build\source\webcore\bindings\js\JSDOMGlobalObject.h(98): note: see
> reference to function template instantiation
> 'WTF::RefPtr<WebCore::DOMWrapperWorld>::~RefPtr(void)' being compiled
> (compiling source file
> C:\cygwin\home\buildbot\slave\win-
> release\build\Source\WebCore\bindings\js\JSIDBObjectStoreCustom.cpp)
>  
> c:\cygwin\home\buildbot\slave\win-
> release\build\source\webcore\bindings\js\JSDOMGlobalObject.h(90): note: see
> reference to class template instantiation
> 'WTF::RefPtr<WebCore::DOMWrapperWorld>' being compiled (compiling source
> file
> C:\cygwin\home\buildbot\slave\win-
> release\build\Source\WebCore\bindings\js\JSIDBObjectStoreCustom.cpp)
> C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/
> PassRefPtr.h(42): error C2227: left of '->deref' must point to
> class/struct/union/generic type (compiling source file
> C:\cygwin\home\buildbot\slave\win-
> release\build\Source\WebCore\bindings\js\JSIDBObjectStoreCustom.cpp)
> [C:\cygwin\home\buildbot\slave\win-
> release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]

I can take a look

[Chris Dumez]

(In reply to comment #24)
> (In reply to comment #23)
> > It looks like this change broke the Windows build:
> > <https://build.webkit.org/builders/Apple%20Win%20Release%20(Build)/builds/
> > 77647>
> > 
> > C:\cygwin\home\buildbot\slave\win-
> > release\build\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/
> > PassRefPtr.h(42): error C2027: use of undefined type
> > 'WebCore::DOMWrapperWorld' (compiling source file
> > C:\cygwin\home\buildbot\slave\win-
> > release\build\Source\WebCore\bindings\js\JSIDBObjectStoreCustom.cpp)
> > [C:\cygwin\home\buildbot\slave\win-
> > release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
> >  
> > c:\cygwin\home\buildbot\slave\win-
> > release\build\source\webcore\bindings\js\JSDOMGlobalObject.h(39): note: see
> > declaration of 'WebCore::DOMWrapperWorld' (compiling source file
> > C:\cygwin\home\buildbot\slave\win-
> > release\build\Source\WebCore\bindings\js\JSIDBObjectStoreCustom.cpp)
> >  
> > C:\cygwin\home\buildbot\slave\win-
> > release\build\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/
> > RefPtr.h(59): note: see reference to function template instantiation 'void
> > WTF::derefIfNotNull<T>(T *)' being compiled
> >           with
> >           [
> >               T=WebCore::DOMWrapperWorld
> >           ] (compiling source file
> > C:\cygwin\home\buildbot\slave\win-
> > release\build\Source\WebCore\bindings\js\JSIDBObjectStoreCustom.cpp)
> >  
> > C:\cygwin\home\buildbot\slave\win-
> > release\build\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/
> > RefPtr.h(59): note: while compiling class template member function
> > 'WTF::RefPtr<WebCore::DOMWrapperWorld>::~RefPtr(void)' (compiling source
> > file
> > C:\cygwin\home\buildbot\slave\win-
> > release\build\Source\WebCore\bindings\js\JSIDBObjectStoreCustom.cpp)
> >  
> > c:\cygwin\home\buildbot\slave\win-
> > release\build\source\webcore\bindings\js\JSDOMGlobalObject.h(98): note: see
> > reference to function template instantiation
> > 'WTF::RefPtr<WebCore::DOMWrapperWorld>::~RefPtr(void)' being compiled
> > (compiling source file
> > C:\cygwin\home\buildbot\slave\win-
> > release\build\Source\WebCore\bindings\js\JSIDBObjectStoreCustom.cpp)
> >  
> > c:\cygwin\home\buildbot\slave\win-
> > release\build\source\webcore\bindings\js\JSDOMGlobalObject.h(90): note: see
> > reference to class template instantiation
> > 'WTF::RefPtr<WebCore::DOMWrapperWorld>' being compiled (compiling source
> > file
> > C:\cygwin\home\buildbot\slave\win-
> > release\build\Source\WebCore\bindings\js\JSIDBObjectStoreCustom.cpp)
> > C:\cygwin\home\buildbot\slave\win-
> > release\build\WebKitBuild\Release\DerivedSources\ForwardingHeaders\wtf/
> > PassRefPtr.h(42): error C2227: left of '->deref' must point to
> > class/struct/union/generic type (compiling source file
> > C:\cygwin\home\buildbot\slave\win-
> > release\build\Source\WebCore\bindings\js\JSIDBObjectStoreCustom.cpp)
> > [C:\cygwin\home\buildbot\slave\win-
> > release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
> 
> I can take a look

<http://trac.webkit.org/changeset/200704>