New issue
Advanced search Search tips

Issue 791121 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug


Participants' hotlists:
Hotlist-Bindings-IDLCompiler


Sign in to add a comment

Bindings does not support union-ed sequences in dictionary IDLs

Project Member Reported by smcgruer@chromium.org, Dec 1 2017

Issue description

It appears that the bindings code doesn't support union-ed sequences in dictionary IDLs:

$ git diff --cached
diff --git a/third_party/WebKit/Source/core/animation/PropertyIndexedKeyframes.idl b/third_party/WebKit/Source/core/animation/PropertyIndexedKeyframes.idl
new file mode 100644
index 000000000000..e258cb537012
--- /dev/null
+++ b/third_party/WebKit/Source/core/animation/PropertyIndexedKeyframes.idl
@@ -0,0 +1,9 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+dictionary PropertyIndexedKeyframes {
+  (double? or sequence<double?>) offset = [];
+  (DOMString or sequence<DOMString>) easing = [];
+  (CompositeOperation or sequence<CompositeOperation>) composite = [];
+};
diff --git a/third_party/WebKit/Source/core/core_idl_files.gni b/third_party/WebKit/Source/core/core_idl_files.gni
index 6d332c6bf827..049985c039be 100644
--- a/third_party/WebKit/Source/core/core_idl_files.gni
+++ b/third_party/WebKit/Source/core/core_idl_files.gni
@@ -543,6 +543,7 @@ core_dictionary_idl_files =
                     "animation/DocumentTimelineOptions.idl",
                     "animation/KeyframeAnimationOptions.idl",
                     "animation/KeyframeEffectOptions.idl",
+                    "animation/PropertyIndexedKeyframes.idl",
                     "animation/ScrollTimelineOptions.idl",
                     "css/FontFaceDescriptors.idl",
                     "css/FontFaceSetLoadEventInit.idl",
$ ninja ...
...
Traceback (most recent call last):
  File "../../third_party/WebKit/Source/bindings/scripts/idl_compiler.py", line 210, in <module>
    sys.exit(main())
  File "../../third_party/WebKit/Source/bindings/scripts/idl_compiler.py", line 199, in main
    options, input_filenames)
  File "../../third_party/WebKit/Source/bindings/scripts/idl_compiler.py", line 153, in generate_dictionary_impl
    idl_compiler.compile_file(idl_filename)
  File "../../third_party/WebKit/Source/bindings/scripts/idl_compiler.py", line 125, in compile_file
    self.compile_and_write(idl_filename)
  File "../../third_party/WebKit/Source/bindings/scripts/idl_compiler.py", line 115, in compile_and_write
    target_definitions, interface_name)
  File "/usr/local/google/home/smcgruer/chromium/src/third_party/WebKit/Source/bindings/scripts/code_generator_v8.py", line 149, in generate_code
    return self.generate_code_internal(definitions, definition_name)
  File "/usr/local/google/home/smcgruer/chromium/src/third_party/WebKit/Source/bindings/scripts/code_generator_v8.py", line 283, in generate_code_internal
    dictionary, interfaces_info)
  File "/usr/local/google/home/smcgruer/chromium/src/third_party/WebKit/Source/bindings/scripts/v8_dictionary.py", line 177, in dictionary_impl_context
    for member in dictionary.members]
  File "/usr/local/google/home/smcgruer/chromium/src/third_party/WebKit/Source/bindings/scripts/v8_dictionary.py", line 221, in member_impl_context
    cpp_default_value = idl_type.literal_cpp_value(member.default_value)
  File "/usr/local/google/home/smcgruer/chromium/src/third_party/WebKit/Source/bindings/scripts/v8_types.py", line 1038, in union_literal_cpp_value
    raise ValueError('Unsupported literal type: ' + idl_literal.idl_type)
ValueError: Unsupported literal type: sequence

As far as I (and jbroman@, more importantly!) can tell, WebIDL does allow the syntax seen in the PropertyIndexedKeyframes dictionary I am attempting to add. 

Lack of support is blocking landing proper parsing of a keyframes argument from the web-animations spec (https://chromium-review.googlesource.com/c/chromium/src/+/760542).
 
Drive-by: from the stack trace, it looks like the issue here is that the [] sequence literal isn't accepted as a default value, though the WebIDL spec says that it should be supported.
Project Member

Comment 2 by bugdroid1@chromium.org, Dec 6 2017

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

commit 3aa53e44ff65065747f59b96dc689609a471aac6
Author: Jeremy Roman <jbroman@chromium.org>
Date: Wed Dec 06 15:38:03 2017

bindings: Support using the [] literal as the default value for a union containing a sequence.

Bug: 791121
Change-Id: Ief7110aae72a25201af687e03c82a88821fd7a2b
Reviewed-on: https://chromium-review.googlesource.com/809906
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Hitoshi Yoshida <peria@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522095}
[modify] https://crrev.com/3aa53e44ff65065747f59b96dc689609a471aac6/third_party/WebKit/Source/bindings/scripts/idl_types.py
[modify] https://crrev.com/3aa53e44ff65065747f59b96dc689609a471aac6/third_party/WebKit/Source/bindings/scripts/v8_types.py
[modify] https://crrev.com/3aa53e44ff65065747f59b96dc689609a471aac6/third_party/WebKit/Source/bindings/tests/idls/core/TestDictionary.idl
[modify] https://crrev.com/3aa53e44ff65065747f59b96dc689609a471aac6/third_party/WebKit/Source/bindings/tests/results/core/TestDictionary.cpp
[modify] https://crrev.com/3aa53e44ff65065747f59b96dc689609a471aac6/third_party/WebKit/Source/bindings/tests/results/core/TestDictionary.h
[modify] https://crrev.com/3aa53e44ff65065747f59b96dc689609a471aac6/third_party/WebKit/Source/bindings/tests/results/core/V8TestDictionary.cpp
[add] https://crrev.com/3aa53e44ff65065747f59b96dc689609a471aac6/third_party/WebKit/Source/bindings/tests/results/core/double_or_double_sequence.cc
[add] https://crrev.com/3aa53e44ff65065747f59b96dc689609a471aac6/third_party/WebKit/Source/bindings/tests/results/core/double_or_double_sequence.h

Project Member

Comment 3 by bugdroid1@chromium.org, Dec 8 2017

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

commit 74a43173297995475802e7cf5bf665c73fb2e819
Author: Jeremy Roman <jbroman@chromium.org>
Date: Fri Dec 08 01:02:28 2017

bindings: Remove null from the union type when invoking V8*::ToImpl.

The V8FooOrBar classes match their C++ counterparts, in that null is removed.
Currently the generator will, in such cases, emit calls to V8FooOrBarOrNull,
which does not exist.

This is covered by existing IDL bindings tests, and matches how v8_class
is determined in v8_union.py.

Bug: 791121
Change-Id: I9a1507c7611e73651004a4c0001e90b19e54230d
Reviewed-on: https://chromium-review.googlesource.com/809908
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Hitoshi Yoshida <peria@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522660}
[modify] https://crrev.com/74a43173297995475802e7cf5bf665c73fb2e819/third_party/WebKit/Source/bindings/scripts/v8_types.py
[modify] https://crrev.com/74a43173297995475802e7cf5bf665c73fb2e819/third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp
[modify] https://crrev.com/74a43173297995475802e7cf5bf665c73fb2e819/third_party/WebKit/Source/bindings/tests/results/core/V8TestTypedefs.cpp

Labels: -Pri-2 Hotlist-Bindings-IDLCompiler Pri-3
Owner: peria@chromium.org

Sign in to add a comment