WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Joshua Bell
Comment 1
2012-05-01 14:09:47 PDT
Created
attachment 139674
[details]
Patch
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
Created
attachment 139691
[details]
Patch
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
Created
attachment 140048
[details]
Patch
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
Created
attachment 140082
[details]
Patch
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
Created
attachment 140578
[details]
Patch
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
Created
attachment 143086
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug