RESOLVED FIXED 85298
IndexedDB: Store key paths in IDBKeyPath type instead of String
https://bugs.webkit.org/show_bug.cgi?id=85298
Summary IndexedDB: Store key paths in IDBKeyPath type instead of String
Joshua Bell
Reported 2012-05-01 11:35:39 PDT
IndexedDB: Store key paths in IDBKeyPath type instead of String
Attachments
Patch (104.15 KB, patch)
2012-05-01 14:09 PDT, Joshua Bell
no flags
Patch (104.54 KB, patch)
2012-05-01 15:04 PDT, Joshua Bell
no flags
Patch (110.23 KB, patch)
2012-05-03 11:04 PDT, Joshua Bell
no flags
Patch (110.45 KB, patch)
2012-05-03 13:48 PDT, Joshua Bell
no flags
Patch (116.11 KB, patch)
2012-05-07 13:32 PDT, Joshua Bell
no flags
Patch (115.77 KB, patch)
2012-05-21 13:21 PDT, Joshua Bell
no flags
Patch for landing (115.86 KB, patch)
2012-05-21 13:48 PDT, Joshua Bell
no flags
Joshua Bell
Comment 1 2012-05-01 14:09:47 PDT
Joshua Bell
Comment 2 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.
WebKit Review Bot
Comment 3 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.
WebKit Review Bot
Comment 4 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
Joshua Bell
Comment 5 2012-05-01 15:04:44 PDT
Joshua Bell
Comment 6 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.
Tony Chang
Comment 7 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.
Joshua Bell
Comment 8 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.
Kentaro Hara
Comment 9 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.
Joshua Bell
Comment 10 2012-05-03 11:04:59 PDT
Joshua Bell
Comment 11 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?
WebKit Review Bot
Comment 12 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
Joshua Bell
Comment 13 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
Kentaro Hara
Comment 14 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
Joshua Bell
Comment 15 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.
Joshua Bell
Comment 16 2012-05-03 13:48:28 PDT
Joshua Bell
Comment 17 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
Kentaro Hara
Comment 18 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...:)
Joshua Bell
Comment 19 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 ?
Joshua Bell
Comment 20 2012-05-04 13:45:57 PDT
Note: this will require a rebase+merge after http://webkit.org/b/85557 lands
Joshua Bell
Comment 21 2012-05-07 13:32:04 PDT
Joshua Bell
Comment 22 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@ ?
Joshua Bell
Comment 23 2012-05-21 13:21:58 PDT
Joshua Bell
Comment 24 2012-05-21 13:23:47 PDT
New patch is just a rebase. Any chromium webkit API reviewers want to r?
James Robinson
Comment 25 2012-05-21 13:26:36 PDT
Comment on attachment 143086 [details] Patch Chromium public WebKit API changes LGTM.
Joshua Bell
Comment 26 2012-05-21 13:48:31 PDT
Created attachment 143090 [details] Patch for landing
WebKit Review Bot
Comment 27 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>
WebKit Review Bot
Comment 28 2012-05-21 14:34:08 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.