New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 702145 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner: ----
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

bindings: records and sequences do not handle nullable types correctly

Project Member Reported by raphael....@intel.com, Mar 16 2017

Issue description

Both sequence<T> and record<K, V> must be able to handle cases where T and V are nullable.

At the moment, "void foo(sequence<long?> arg)" generates the following excerpt:
  Vector<int32_t> arg;
  arg = toImplArray<Vector<int32_t>>(info[0], 1, info.GetIsolate(), exceptionState);
which will coerce null values into numbers instead of just accepting them.

Similarly, "void foo(record<DOMString, long?> arg)" generates:
  Vector<std::pair<String, int32_t>> arg;
  arg = NativeValueTraits<IDLRecord<IDLString, IDLLong>>::nativeValue(info.GetIsolate(), info[0], exceptionState);
which has the same issue.

Passing types back to JavaScript is also a problem, as we don't even have a ToV8() overload for blink::Nullable.
 
Hmm, fixing this requires some wider changes, as the code for attribute getters and setters is not very Nullable-friendly at the moment.

  attribute byte? byteOrNullAttribute;

generates a getter like this one:

  bool isNull = false;
  int8_t cppValue(impl->byteOrNullAttribute(isNull));
  if (isNull) {
    v8SetReturnValueNull(info);
    return;
  }
  v8SetReturnValueInt(info, cppValue);

and a setter like this:

  int8_t cppValue = NativeValueTraits<IDLByte>::nativeValue(info.GetIsolate(), v8Value, exceptionState, NormalConversion);
  if (exceptionState.hadException())
    return;
  impl->setByteOrNullAttribute(cppValue);

instead of using Nullable<>, so the actual implementations would need to be changed as well.
I thought that you're going to support "sequence<T?>" specifically, but now it seems like you're going to support nullable in general, like "sequence<T?>?" ?

I think that #c1 wouldn't directly affect sequence<T?> because sequence<T?> itself is NOT nullable.

I mentioned the bits in #c1 because I started out by looking at the record<> code, where handling "byte?", "record<DOMString, byte?>" and "record<DOMString, byte>?" is essentially done by the same recursive function. They should generate "Nullable<int8_t>", "Vector<std::pair<String, Nullable<int8_t>>>" and "Nullable<Vector<std::pair<String, int8_t>>>", respectively.
Sounds a nice plan.  :)

Project Member

Comment 5 by bugdroid1@chromium.org, Jan 2 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b

commit fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b
Author: Jeremy Roman <jbroman@chromium.org>
Date: Tue Jan 02 18:10:29 2018

bindings: Implement basic support for converting to/from Optional<> for nullable types in records and sequences.

This enables NativeValueTraits and ToV8 to support this mapping, which allows
constructs like sequence<double?> to work without losing the nullability of the
element type (which would cause coercion of null to 0 in this case).

This only handles some fairly simple types as the T in sequence<T?>:
- primitive types (numbers, booleans, etc.) map to Optional<T>
- string types map null to String()
- GC object types map null to nullptr
- unions map null to the default (null) union value

In particular, this doesn't handle:
- dictionaries (where the right solution isn't immediately clear)
- sequences of nullable sequences or records (which already doesn't
  work correctly in many cases, even without nullability)

Such cases are punted to later.

Bug:  792241 , 702145 
Change-Id: I6dc1f488d4c98b908ec03c6789e6b713c05becb7
Reviewed-on: https://chromium-review.googlesource.com/831497
Reviewed-by: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526503}
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/core/v8/IDLTypes.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/core/v8/IDLTypesTest.cpp
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/core/v8/ToV8Test.cpp
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/scripts/v8_types.py
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/templates/interface.h.tmpl
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/templates/union_container.h.tmpl
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/idls/core/TestDictionary.idl
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/idls/core/TestInterface.idl
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/TestDictionary.cpp
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/TestDictionary.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8ArrayBuffer.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8ArrayBufferView.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8DataView.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8SVGTestInterface.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestAttributeGetters.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestCallbackFunctions.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestConstants.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestDictionary.cpp
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestIntegerIndexed.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestIntegerIndexedGlobal.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestIntegerIndexedPrimaryGlobal.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface.cpp
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface2.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterface3.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceCheckSecurity.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceConstructor.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceConstructor2.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceConstructor3.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceConstructor4.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceCustomConstructor.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceDocument.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceEmpty.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceEventInitConstructor.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceEventTarget.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceGarbageCollected.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceNamedConstructor.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceNamedConstructor2.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceNode.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceOriginTrialEnabled.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceSecureContext.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestNode.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestSpecialOperations.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestSpecialOperationsNotEnumerable.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestTypedefs.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8TestVariadicConstructorArguments.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/V8Uint8ClampedArray.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/array_buffer_or_array_buffer_view_or_dictionary.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/boolean_or_element_sequence.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/boolean_or_string_or_unrestricted_double.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/boolean_or_test_callback_interface.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/byte_string_or_node_list.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/byte_string_sequence_sequence_or_byte_string_byte_string_record.h
[add] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/double_or_double_or_null_sequence.cc
[add] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/double_or_double_or_null_sequence.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/double_or_double_sequence.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/double_or_long_or_boolean_sequence.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/double_or_string.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/double_or_string_or_double_or_string_sequence.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/element_sequence_or_byte_string_double_or_string_record.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/float_or_boolean.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/long_or_boolean.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/long_or_test_dictionary.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/long_sequence_or_event.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/nested_union_type.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/node_or_node_list.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/string_or_array_buffer_or_array_buffer_view.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/string_or_double.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/string_or_string_sequence.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/test_enum_or_double.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/test_enum_or_test_enum_sequence.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/test_interface_2_or_uint8_array.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/test_interface_garbage_collected_or_string.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/test_interface_or_long.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/test_interface_or_test_interface_empty.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/unrestricted_double_or_string.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/unsigned_long_long_or_boolean_or_test_callback_interface.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/core/xml_http_request_or_string.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/modules/V8TestInheritedLegacyUnenumerableNamedProperties.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/modules/V8TestInterface5.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/modules/V8TestNotEnumerableNamedGetter.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/modules/V8TestSubObject.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/bindings/tests/results/modules/boolean_or_string.h
[modify] https://crrev.com/fc2c306616c3ed03bfb34f9dd5ffd92206f08c2b/third_party/WebKit/Source/platform/bindings/ToV8.h

Status: Fixed (was: Available)
Some corner cases remain, but jbroman's CL works for most cases we've seen so far.

Sign in to add a comment