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

Issue 792241 link

Starred by 1 user
Status: Fixed
Owner:
jbroman@chromium.org
Closed: Jan 2018
Cc:
smcgruer@chromium.org
█ raphael....@intel.com
bashi@chromium.org
lfg@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 777971



Sign in to add a comment

Sequences of nullable IDL types lose nullability

Project Member Reported by jbroman@chromium.org, Dec 5 2017 
For example, creating a dictionary member of type sequence<double?> uses    
 NativeValueTraits<IDLSequence<IDLDouble>>::NativeValue to produce a Vector<double>.

Unfortunately, this coerces the elements of the sequence to number, and thus null elements in the sequence become 0. This is equivalent to using sequence<double>, and doesn't empower the callee to distinguish between null and zero.

Instead, we should map sequence<double?> to Vector<Optional<double>> or similar, so that we can carry this difference through the bindings.

cc smcgruer, since this affects Web Animations IDL he is trying to support

Comment 1 by bashi@chromium.org, Dec 8 2017 

Thanks for the report. Probably the path to fix would be:
- Add Nullable support in NativeValueTraits
- Fix the code generator.

The latter would be relatively easy but I'm not sure how difficult the former is.

Comment 2 by smcgruer@chromium.org, Dec 8 2017

Blocking: 777971

Comment 3 by smcgruer@chromium.org, Dec 12 2017

Cc: jbroman@chromium.org
Is there a priority/expected fix date for this issue? I understand that end of quarter is coming up so people are probably rushing for their existing commitments (and going on vacations!), but wanted to get a rough idea of the expected impact on my Web Animations patch. (I.e. by end of this year, early next year, nearer end of Q1, etc).

Thanks! :)

Comment 4 by bashi@chromium.org, Dec 14 2017

Cc: raphael....@intel.com
Hopefully end of this year, but depends on how difficult to addd Nullable support in NativeValueTraits. Please feel free to increase the priority if fixing this is important to your work.

Comment 5 by smcgruer@chromium.org, Dec 14 2017

Labels: -Pri-3 Pri-2
Bumped to P2; end-of-year should be fine for a fix. Thanks for all your (and jbroman's) efforts extending our IDL support just for web-animations!

Comment 6 by raphael....@intel.com, Dec 14 2017 

I'm still catching up with my bug mail after a few weeks OoO, but I think this is something I started discussing earlier this year in  issue 702145 .

I never got around to implementing my plan though :/

Comment 7 by jbroman@chromium.org, Dec 18 2017 

Alright, I've got a crack at this. Seems to work for me.
Project Member

Comment 8 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

Comment 9 by jbroman@chromium.org, Jan 2 2018

Cc: -jbroman@chromium.org bashi@chromium.org
Owner: jbroman@chromium.org
Status: Fixed (was: Assigned)
I'm gonna call this bug fixed (even though there are still more general cases that don't work).

Comment 10 by raphael....@intel.com, Jan 8 2018

Labels: M-65
►

Sign in to add a comment