bindings: ByteString? and USVString? are not handled correctly |
||
Issue descriptionSee bug 808018 . The Web IDL string types are not handled just via NativeValueTraits like most other types: - DOMString was not touched during the NVT conversion because it currently does not need an |ExceptionState| and we'd end up introducing performance regressions with unnecessary ExceptionState's if we had performed the conversion. So we just create a V8StringResource<> and call Prepare() convert JS values to DOMStrings. - The ByteString and USVString's NativeValueTraits specializations are just wrappers around ToByteString() and ToUSVString(), respectively. It turns out V8StringResource has all the infrastructure to convert |null| and |undefined| to an empty WTF::String if we choose to, but ToByteString() and ToUSVString() just follow the conversion steps described in the Web IDL spec. Namely, it just calls ECMAScript's ToString() on the JS values it receives. In practice, this means if we have a Web IDL operation "void frob(USVString? foo)" and call "foo(null)", the C++ implementation of |frob()| will be passed a String whose value is "null" (a 4-byte string) instead of an empty one like we'd expect (and which is passed if |foo| had been a DOMString?).
,
Feb 7 2018
To those interested: the main CL is https://chromium-review.googlesource.com/c/chromium/src/+/904525, which I intend to land tomorrow if all goes well.
,
Feb 8 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/892e94a754deb3e95a68b3b728714898177b9e13 commit 892e94a754deb3e95a68b3b728714898177b9e13 Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com> Date: Thu Feb 08 01:01:07 2018 bindings: Handle V8StringResourceMode in string type NativeValueTraits specializations So far, converting DOMString's from JS to C++ does not go through NativeValueTraits for performance reasons (there may not be an ExceptionState in scope) and we just use V8StringResource::Prepare() directly instead (unless the DOMString is inside a sequence or record). ByteString's and USVString's, on the other hand, go through NativeValueTraits, which would just call ToByteString() or ToUSVString(), respectively. It turns out V8StringResource has all the infrastructure to convert |null| and |undefined| to an empty or null WTF::String if we choose to, but ToByteString() and ToUSVString() just follow the conversion steps described in the Web IDL spec. Namely, it just calls ECMAScript's ToString() on the JS values it receives. In practice, this means if we have a Web IDL operation "void frob(USVString? foo)" and call "foo(null)", the C++ implementation of |frob()| will be passed a String whose value is "null" (a 4-byte string) instead of an null one like we'd expect (and which is passed if |foo| had been a DOMString?). In simple terms, we solve the bug by essentially templatizing the existing IDL{ByteString,String,USVString} types on a V8StringResourceMode and letting V8StringResource handle JS->C++ string conversions, as it already has all the code to handle null and undefined values that cover nullable types as well as all the values we support for the [TreatNullAs] extended attribute. In practice, "USVString? foo" used to be converted like this: V8StringResource<kTreatNullAndUndefinedAsNullString> cppValue = NativeValueTraits<IDLUSVString>::NativeValue(...); and is now converted like this: V8StringResource<kTreatNullAndUndefinedAsNullString> cppValue = NativeValueTraits<IDLUSVStringBase<kTreatNullAndUndefinedAsNullString>>::NativeValue(...); The actual implementation requires us to: * Rename IDLByteString & co. to IDLByteStringBase and make them template classes that take a V8StringResourceMode. IDLByteString & co are now typedefs for IDLByteStringBase<kDefaultMode> (and so forth) for simplicity. * Merge ToByteString() and ToUSVString() into NativeValueTraits (and update callers). HasUnmatchedSurrogates() no longer requires a non-empty string to work. * On the Python side, extract cpp_type()'s string_mode() into a free function that is now also called from native_value_traits_type_name(). The latter now special-cases string types so that it can return either the Base classes with a template paramter or the template-less aliases. It also means the IDLNullable<> wrapper is no longer used with string types. Bug: 808995 Change-Id: I92ec7db7b28b5510e80ced6125ecd0ea561a753a Reviewed-on: https://chromium-review.googlesource.com/904525 Reviewed-by: Kenichi Ishibashi <bashi@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Reviewed-by: Hitoshi Yoshida <peria@chromium.org> Reviewed-by: Yuki Shiino <yukishiino@chromium.org> Commit-Queue: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com> Cr-Commit-Position: refs/heads/master@{#535227} [modify] https://crrev.com/892e94a754deb3e95a68b3b728714898177b9e13/third_party/WebKit/LayoutTests/fast/js/webidl-type-mapping-expected.txt [modify] https://crrev.com/892e94a754deb3e95a68b3b728714898177b9e13/third_party/WebKit/LayoutTests/fast/js/webidl-type-mapping.html [modify] https://crrev.com/892e94a754deb3e95a68b3b728714898177b9e13/third_party/WebKit/Source/bindings/core/v8/IDLTypes.h [modify] https://crrev.com/892e94a754deb3e95a68b3b728714898177b9e13/third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h [modify] https://crrev.com/892e94a754deb3e95a68b3b728714898177b9e13/third_party/WebKit/Source/bindings/core/v8/V8BindingForCore.cpp [modify] https://crrev.com/892e94a754deb3e95a68b3b728714898177b9e13/third_party/WebKit/Source/bindings/core/v8/V8BindingForCore.h [modify] https://crrev.com/892e94a754deb3e95a68b3b728714898177b9e13/third_party/WebKit/Source/bindings/scripts/v8_dictionary.py [modify] https://crrev.com/892e94a754deb3e95a68b3b728714898177b9e13/third_party/WebKit/Source/bindings/scripts/v8_methods.py [modify] https://crrev.com/892e94a754deb3e95a68b3b728714898177b9e13/third_party/WebKit/Source/bindings/scripts/v8_types.py [modify] https://crrev.com/892e94a754deb3e95a68b3b728714898177b9e13/third_party/WebKit/Source/bindings/templates/dictionary_v8.cpp.tmpl [modify] https://crrev.com/892e94a754deb3e95a68b3b728714898177b9e13/third_party/WebKit/Source/bindings/tests/idls/core/TestDictionary.idl [modify] https://crrev.com/892e94a754deb3e95a68b3b728714898177b9e13/third_party/WebKit/Source/bindings/tests/idls/core/TestInterface.idl [modify] https://crrev.com/892e94a754deb3e95a68b3b728714898177b9e13/third_party/WebKit/Source/bindings/tests/idls/core/TestInterfaceConstructor.idl [modify] https://crrev.com/892e94a754deb3e95a68b3b728714898177b9e13/third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl [modify] https://crrev.com/892e94a754deb3e95a68b3b728714898177b9e13/third_party/WebKit/Source/bindings/tests/results/core/TestDictionary.h [modify] https://crrev.com/892e94a754deb3e95a68b3b728714898177b9e13/third_party/WebKit/Source/bindings/tests/results/core/V8TestDictionary.cpp [modify] https://crrev.com/892e94a754deb3e95a68b3b728714898177b9e13/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface.cpp [modify] https://crrev.com/892e94a754deb3e95a68b3b728714898177b9e13/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface.h [modify] https://crrev.com/892e94a754deb3e95a68b3b728714898177b9e13/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceConstructor.cpp [modify] https://crrev.com/892e94a754deb3e95a68b3b728714898177b9e13/third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp [modify] https://crrev.com/892e94a754deb3e95a68b3b728714898177b9e13/third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.h [modify] https://crrev.com/892e94a754deb3e95a68b3b728714898177b9e13/third_party/WebKit/Source/core/fetch/RequestInit.cpp [modify] https://crrev.com/892e94a754deb3e95a68b3b728714898177b9e13/third_party/WebKit/Source/core/fetch/Response.cpp [modify] https://crrev.com/892e94a754deb3e95a68b3b728714898177b9e13/third_party/WebKit/Source/core/testing/TypeConversions.h [modify] https://crrev.com/892e94a754deb3e95a68b3b728714898177b9e13/third_party/WebKit/Source/core/testing/TypeConversions.idl
,
Feb 8 2018
,
Feb 13 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3560577158bf8452278223fa4dfa702d33ec1b78 commit 3560577158bf8452278223fa4dfa702d33ec1b78 Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com> Date: Tue Feb 13 07:36:43 2018 bindings: Properly treat nullable enums in dictionaries like strings v8_dictionary.py's member_context() had a few bugs when it comes to handling enums like strings: * |enum_values| was acting on |unwrapped_idl_type|, which essentially means a nullable type's inner type. In practice, it meant IdlNullable' enum_values() was not being called and we never added nullptr to the list of valid enum values. * |is_string_type| was acting on the non-preprocessed IDL type, which means the enum type was not converted to a DOMString, causing |idl_type.is_string_type| to return False and the code generator to manually calling v8::Value::IsNull() instead of letting V8StringResource handle it. Bug: 808995 , 809190 Change-Id: Ia04d08d6110af583342289858d590be9a000ee66 Reviewed-on: https://chromium-review.googlesource.com/913349 Commit-Queue: Yuki Shiino <yukishiino@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Reviewed-by: Jeremy Roman <jbroman@chromium.org> Reviewed-by: Kenichi Ishibashi <bashi@chromium.org> Reviewed-by: Hitoshi Yoshida <peria@chromium.org> Reviewed-by: Yuki Shiino <yukishiino@chromium.org> Cr-Commit-Position: refs/heads/master@{#536277} [modify] https://crrev.com/3560577158bf8452278223fa4dfa702d33ec1b78/third_party/WebKit/LayoutTests/bindings/idl-dictionary-unittest-expected.txt [modify] https://crrev.com/3560577158bf8452278223fa4dfa702d33ec1b78/third_party/WebKit/LayoutTests/bindings/idl-dictionary-unittest.html [modify] https://crrev.com/3560577158bf8452278223fa4dfa702d33ec1b78/third_party/WebKit/Source/bindings/scripts/v8_dictionary.py [modify] https://crrev.com/3560577158bf8452278223fa4dfa702d33ec1b78/third_party/WebKit/Source/bindings/tests/idls/core/TestDictionary.idl [modify] https://crrev.com/3560577158bf8452278223fa4dfa702d33ec1b78/third_party/WebKit/Source/bindings/tests/idls/core/TestInterface.idl [modify] https://crrev.com/3560577158bf8452278223fa4dfa702d33ec1b78/third_party/WebKit/Source/bindings/tests/results/core/TestDictionary.h [modify] https://crrev.com/3560577158bf8452278223fa4dfa702d33ec1b78/third_party/WebKit/Source/bindings/tests/results/core/V8TestDictionary.cpp [modify] https://crrev.com/3560577158bf8452278223fa4dfa702d33ec1b78/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface.cpp [modify] https://crrev.com/3560577158bf8452278223fa4dfa702d33ec1b78/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface.h |
||
►
Sign in to add a comment |
||
Comment 1 by bugdroid1@chromium.org
, Feb 6 2018