Bug 85298 - IndexedDB: Store key paths in IDBKeyPath type instead of String
Summary: IndexedDB: Store key paths in IDBKeyPath type instead of String
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joshua Bell
URL:
Keywords:
Depends on:
Blocks: 84207 84303
  Show dependency treegraph
 
Reported: 2012-05-01 11:35 PDT by Joshua Bell
Modified: 2012-05-21 14:34 PDT (History)
11 users (show)

See Also:


Attachments
Patch (104.15 KB, patch)
2012-05-01 14:09 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (104.54 KB, patch)
2012-05-01 15:04 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (110.23 KB, patch)
2012-05-03 11:04 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (110.45 KB, patch)
2012-05-03 13:48 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (116.11 KB, patch)
2012-05-07 13:32 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (115.77 KB, patch)
2012-05-21 13:21 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch for landing (115.86 KB, patch)
2012-05-21 13:48 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2012-05-01 11:35:39 PDT
IndexedDB: Store key paths in IDBKeyPath type instead of String
Comment 1 Joshua Bell 2012-05-01 14:09:47 PDT
Created attachment 139674 [details]
Patch
Comment 2 Joshua Bell 2012-05-01 14:11:54 PDT
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.
Comment 3 WebKit Review Bot 2012-05-01 14:12:45 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 4 WebKit Review Bot 2012-05-01 14:49:27 PDT
Comment on attachment 139674 [details]
Patch

Attachment 139674 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12598482
Comment 5 Joshua Bell 2012-05-01 15:04:44 PDT
Created attachment 139691 [details]
Patch
Comment 6 Joshua Bell 2012-05-01 15:06:22 PDT
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.
Comment 7 Tony Chang 2012-05-02 14:02:41 PDT
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.
Comment 8 Joshua Bell 2012-05-02 15:17:14 PDT
> 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.
Comment 9 Kentaro Hara 2012-05-03 05:39:39 PDT
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.
Comment 10 Joshua Bell 2012-05-03 11:04:59 PDT
Created attachment 140048 [details]
Patch
Comment 11 Joshua Bell 2012-05-03 11:11:03 PDT
(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?
Comment 12 WebKit Review Bot 2012-05-03 11:43:37 PDT
Comment on attachment 140048 [details]
Patch

Attachment 140048 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12614275
Comment 13 Joshua Bell 2012-05-03 11:47:34 PDT
(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
Comment 14 Kentaro Hara 2012-05-03 12:16:57 PDT
(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
Comment 15 Joshua Bell 2012-05-03 13:28:19 PDT
(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.
Comment 16 Joshua Bell 2012-05-03 13:48:28 PDT
Created attachment 140082 [details]
Patch
Comment 17 Joshua Bell 2012-05-03 13:52:58 PDT
(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
Comment 18 Kentaro Hara 2012-05-03 14:06:28 PDT
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...:)
Comment 19 Joshua Bell 2012-05-03 14:07:50 PDT
> The spec around null or undefined is really confusing...:)

+1000!

jamesr@ or others - r? for Source/WebKit/chromium ?
Comment 20 Joshua Bell 2012-05-04 13:45:57 PDT
Note: this will require a rebase+merge after http://webkit.org/b/85557 lands
Comment 21 Joshua Bell 2012-05-07 13:32:04 PDT
Created attachment 140578 [details]
Patch
Comment 22 Joshua Bell 2012-05-07 13:34:29 PDT
(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@ ?
Comment 23 Joshua Bell 2012-05-21 13:21:58 PDT
Created attachment 143086 [details]
Patch
Comment 24 Joshua Bell 2012-05-21 13:23:47 PDT
New patch is just a rebase.

Any chromium webkit API reviewers want to r?
Comment 25 James Robinson 2012-05-21 13:26:36 PDT
Comment on attachment 143086 [details]
Patch

Chromium public WebKit API changes LGTM.
Comment 26 Joshua Bell 2012-05-21 13:48:31 PDT
Created attachment 143090 [details]
Patch for landing
Comment 27 WebKit Review Bot 2012-05-21 14:33:57 PDT
Comment on attachment 143090 [details]
Patch for landing

Clearing flags on attachment: 143090

Committed r117817: <http://trac.webkit.org/changeset/117817>
Comment 28 WebKit Review Bot 2012-05-21 14:34:08 PDT
All reviewed patches have been landed.  Closing bug.