IndexedDB: Store key paths in IDBKeyPath type instead of String

• Component: New Bugs
• Status: RESOLVED FIXED

[Joshua Bell]

IndexedDB: Store key paths in IDBKeyPath type instead of String

[Joshua Bell]

Created attachment 139674 [details]
Patch

[Joshua Bell]

dgrogan@, alecflett@ - preliminary review?

This is a subset of https://bugs.webkit.org/show_bug.cgi?id=84207 that basically does s/String keyPath/IDBKeyPath keyPath/ without adding new logic for arrays coming in or being used, although they can be stored and transported.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Comment on attachment 139674 [details]
Patch

Attachment 139674 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12598482

[Joshua Bell]

Created attachment 139691 [details]
Patch

[Joshua Bell]

chromium ews failing since chromium DEPS was old. New patch bump it to 134581 to pick up the cr side patch, but will want to land that separately.

[Tony Chang]

Comment on attachment 139691 [details]
Patch

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

LGTM, but someone else should probably review the bindings code and an API reviewer will have to OK the WebKit API changes.

> Source/WebCore/Modules/indexeddb/IDBLevelDBCoding.cpp:641
> +    case IDBKeyPath::ArrayType:
> +        {

Nit: It looks like for case statements, we put the { on the case: line and the closing } indented to line up with |case|.

> Source/WebCore/Modules/indexeddb/IDBLevelDBCoding.cpp:669
> +    case IDBKeyPath::StringType:
> +        {

ditto

> Source/WebCore/Modules/indexeddb/IDBLevelDBCoding.cpp:673
> +    case IDBKeyPath::ArrayType:
> +        {

ditto

> Source/WebKit/chromium/DEPS:35
> -  'chromium_rev': '134537'
> +  'chromium_rev': '134581'

Please do this in a separate change.  It doesn't need to be reviewed if it compiles.

[Joshua Bell]

> LGTM, but someone else should probably review the bindings code and an API reviewer will have to OK the WebKit API changes.

jamesr@ could you take a look at the WebKit API changes, since you reviewed my earlier patch that uglified the code?

haraken@ can you review the binding changes?

> > Source/WebCore/Modules/indexeddb/IDBLevelDBCoding.cpp:641
> > +    case IDBKeyPath::ArrayType:
> > +        {
> 
> Nit: It looks like for case statements, we put the { on the case: line and the closing } indented to line up with |case|.

Thanks, will fix. (Just got dinged for this on the Chromium side too, d'oh!)

> > Source/WebKit/chromium/DEPS:35
> > -  'chromium_rev': '134537'
> > +  'chromium_rev': '134581'
> 
> Please do this in a separate change.  It doesn't need to be reviewed if it compiles.

Yep, just wanted to make sure that was the only thing failing on chrome ews. 

New patch coming shortly to address (just) the above issues.

[Kentaro Hara]

Comment on attachment 139691 [details]
Patch

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

> Source/WebCore/bindings/v8/Dictionary.cpp:427
> +#if ENABLE(INDEXED_DATABASE)

Let's remove #if. This is a general method and we can use the method in other code.

> Source/WebCore/bindings/v8/Dictionary.cpp:439
> +        v8::Local<v8::Value> indexedValue = v8Array->Get(v8::Integer::New(i));

v8::Integer::New(i) => v8::Uint32::New(i)

> Source/WebCore/bindings/v8/Dictionary.h:86
> +#if ENABLE(INDEXED_DATABASE)

Let's remove #if.

> Source/WebCore/bindings/v8/custom/V8IDBAnyCustom.cpp:79
> +    case IDBAny::StringType:
> +        return v8String(impl->string());

Shouldn't this be v8StringOrNull()? Previously [TreatReturnedNullStringAs=Null] was specified in keyPath. If you want to keep the behavior, I guess that you need to use v8StringOrNull() instead of v8String(). In any case, please add a test case where keyPath returns a null string.

[Joshua Bell]

Created attachment 140048 [details]
Patch

[Joshua Bell]

(In reply to comment #9)
> 
> Let's remove #if. This is a general method and we can use the method in other code.

Done.
 
> v8::Integer::New(i) => v8::Uint32::New(i)

Done.

> Let's remove #if.

Done

> > Source/WebCore/bindings/v8/custom/V8IDBAnyCustom.cpp:79
> > +    case IDBAny::StringType:
> > +        return v8String(impl->string());
> 
> Shouldn't this be v8StringOrNull()? Previously [TreatReturnedNullStringAs=Null] was specified in keyPath. If you want to keep the behavior, I guess that you need to use v8StringOrNull() instead of v8String(). In any case, please add a test case where keyPath returns a null string.

Previously, the absence of a keyPath was stored as a null string. Now it is stored as a NullType IDBKeyPath. IDBKeyPath's operator PassRefPtr<IDBAny>() produces a NullType IDBAny rather than a null String. 

I added an assert in the IDBKeyPath constructors to preclude null strings, and added tests that verify null/undefined as arguments. This detected a behavior change in this patch which brings it in line with my reading of the WebIDL spec: passing |null| or |undefined| to a string argument yields "null" or "undefined" strings.

r?

[WebKit Review Bot]

Comment on attachment 140048 [details]
Patch

Attachment 140048 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12614275

[Joshua Bell]

(In reply to comment #12)
> (From update of attachment 140048 [details])
> Attachment 140048 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12614275

Weird... chromium DEPS roll should at r115921 have taken care of this. o_O

[Kentaro Hara]

(In reply to comment #11)
> Previously, the absence of a keyPath was stored as a null string. Now it is stored as a NullType IDBKeyPath. IDBKeyPath's operator PassRefPtr<IDBAny>() produces a NullType IDBAny rather than a null String. 

Previously a null String was treated as a JavaScript null (since [TreatReturnedNullAs=Null] was specified). In your patch, what is the NullType IDBKeyPath treated as in JavaScript?

> I added an assert in the IDBKeyPath constructors to preclude null strings, and added tests that verify null/undefined as arguments. This detected a behavior change in this patch which brings it in line with my reading of the WebIDL spec: passing |null| or |undefined| to a string argument yields "null" or "undefined" strings.

The test case looks good, but I am afraid that the added test cases are not the test case for our current concern. [TreatReturnedNullAs=Null] affects the behavior of when WebCore tries to return a null String (i.e. NullType in your patch), not the behavior of when JavaScript passes null or undefined.

c.f. https://trac.webkit.org/wiki/WebKitIDL#TreatNullAs

[Joshua Bell]

(In reply to comment #14)
> (In reply to comment #11)
> > Previously, the absence of a keyPath was stored as a null string. Now it is stored as a NullType IDBKeyPath. IDBKeyPath's operator PassRefPtr<IDBAny>() produces a NullType IDBAny rather than a null String. 
> 
> Previously a null String was treated as a JavaScript null (since [TreatReturnedNullAs=Null] was specified). In your patch, what is the NullType IDBKeyPath treated as in JavaScript?

The keyPath() accessor returns IDBAny; IDBKeyPath of NullType produces IDBAny of NullType which produces a JavaScript null value.

> 
> > I added an assert in the IDBKeyPath constructors to preclude null strings, and added tests that verify null/undefined as arguments. This detected a behavior change in this patch which brings it in line with my reading of the WebIDL spec: passing |null| or |undefined| to a string argument yields "null" or "undefined" strings.
> 
> The test case looks good, but I am afraid that the added test cases are not the test case for our current concern. [TreatReturnedNullAs=Null] affects the behavior of when WebCore tries to return a null String (i.e. NullType in your patch), not the behavior of when JavaScript passes null or undefined.

Ah, I understand. That's covered in objectstore-basics.html: shouldBeNull("store.keyPath");

But I'll add it to keypath-basics.html as well.

[Joshua Bell]

Created attachment 140082 [details]
Patch

[Joshua Bell]

(In reply to comment #11)
> This detected a behavior change in this patch which brings it in line 
> with my reading of the WebIDL spec: passing |null| or |undefined| to a 
> string argument yields "null" or "undefined" strings.

Double-checking the IDB spec, this was in error, and has been reverted. The spec is explicit:

createObjectStore: "If the optionalParameters argument is specified and has a keyPath property which is not undefined or null, then set keyPath to the value of this property. If keyPath is an Array, then each item in the array is converted to a string. If keyPath is not an Array, it is converted to a string." => changed the Dictionary accessor to getWithUndefinedOrNullCheck(), reverted the tests (so |null| and |undefined| yield IDBKeyPath::NullType)

createIndex: "If the keyPath argument is an Array, then each item in the array is converted to a DOMString. If keyPath is not an Array, it is converted to a DOMString." => |null| or |undefined| are converted to strings ("null", "undefined"), which aligns with DOMString (not DOMString?) in WebIDL

[Kentaro Hara]

r+ for *.idl, v8/Dictionary.*, v8/custom/V8IDBAnyCustom.cpp, and their test cases.

> Double-checking the IDB spec, this was in error, and has been reverted. The spec is explicit:

The spec around null or undefined is really confusing...:)

[Joshua Bell]

> The spec around null or undefined is really confusing...:)

+1000!

jamesr@ or others - r? for Source/WebKit/chromium ?

[Joshua Bell]

Note: this will require a rebase+merge after http://webkit.org/b/85557 lands

[Joshua Bell]

Created attachment 140578 [details]
Patch

[Joshua Bell]

(In reply to comment #20)
> Note: this will require a rebase+merge after http://webkit.org/b/85557 lands

New patch is rebased, and adds unit tests for encode/decodeIDBKeyPath. Also switches the new-style encoding of string-type key paths to encoded-with-length which is more robust.

Still waiting on a review of the WebKit API changes, jamesr@ / fishd@ ?

[Joshua Bell]

Created attachment 143086 [details]
Patch

[Joshua Bell]

New patch is just a rebase.

Any chromium webkit API reviewers want to r?

[James Robinson]

Comment on attachment 143086 [details]
Patch

Chromium public WebKit API changes LGTM.

[Joshua Bell]

Created attachment 143090 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 143090 [details]
Patch for landing

Clearing flags on attachment: 143090

Committed r117817: <http://trac.webkit.org/changeset/117817>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.