Oh drat, I tested this on the wrong branch. Ok, it's only the sequence case that is broken:

[127563:127563:0205/155028.931798:INFO:CONSOLE(7)] "Composite: null case success!", source: file:///tmp/animate2.html (7)
[127563:127563:0205/155028.931858:INFO:CONSOLE(16)] "Exception for composite: [ null, "replace" ] - Failed to execute 'animate' on 'Element': The provided value '' is not a valid enum value of type CompositeOperationOrNullSequence.", source: file:///tmp/animate2.html (16)

Updated title.

This is the generated ToImpl:

void V8CompositeOperationOrCompositeOperationOrNullSequence::ToImpl(v8::Isolate* isolate, v8::Local<v8::Value> v8Value, CompositeOperationOrCompositeOperationOrNullSequence& impl, UnionTypeConversionMode conversionMode, ExceptionState& exceptionState) {
  if (v8Value.IsEmpty())
    return;

  if (conversionMode == UnionTypeConversionMode::kNullable && IsUndefinedOrNull(v8Value))
    return;

  if (HasCallableIteratorSymbol(isolate, v8Value, exceptionState)) {
    Vector<String> cppValue = NativeValueTraits<IDLSequence<IDLNullable<IDLString>>>::NativeValue(isolate, v8Value, exceptionState);
    if (exceptionState.HadException())
      return;
    const char* validValues[] = {
        "replace",
        "add",
        "accumulate",
    };
    if (!IsValidEnum(cppValue, validValues, WTF_ARRAY_LENGTH(validValues), "CompositeOperationOrNullSequence", exceptionState))
      return;
    impl.SetCompositeOperationOrNullSequence(cppValue);
    return;
  }

  {
    V8StringResource<> cppValue = v8Value;
    if (!cppValue.Prepare(exceptionState))
      return;
    const char* validValues[] = {
        "replace",
        "add",
        "accumulate",
    };
    if (!IsValidEnum(cppValue, validValues, WTF_ARRAY_LENGTH(validValues), "CompositeOperation", exceptionState))
      return;
    impl.SetCompositeOperation(cppValue);
    return;
  }
}


Note that it also misnames the enum in one of the IsValidEnum checks - which is web visible!

Thanks for filing the issue and working on it!

This is unrelated to unions and sequences; the problem is that nullable enums are not handled correctly. A simple "MyEnum? foo" will cause the same problematic IsValidEnum() code to be generated, it looks like you thought only the sequences of nullable unions case was broken because the union code has a check at the very beginning of ToImpl() for kNullable and IsUndefinedOrNull().

(I haven't checked nullable enums in general, though it would seem slightly surprising to me that it would not have come up to this point.)

Unions are special, though, because we lift the nullability of union members. i.e. we make (Foo or Bar), (Foo? or Bar), (Foo or Bar?) and (Foo or Bar)? all to the same union container, FooOrBar. Unions also have special handling for sequences (see "array_or_sequence_member" in the union stuff). It might be possible to factor it more nicely, but at the moment this stuff isn't trivially separable.

Unions are special, but AFAICS there's nothing wrong with the code we already have in the tree. For "(Foo or Bar)", ToImpl() is invoked with UnionTypeConversionMode::kNotNullable and then it's up to Foo and Bar to handle null values; for "(Foo? or Bar)" and "(Foo or Bar?)" ToImpl() is invoked with UnionTypeConversionMode::kNullable and a null value causes an earlier return and FooOrBar.IsNull() to return true.

This code specially handles sequences of enums in unions.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/templates/union_container.cpp.tmpl?l=111

The UnionTypeConversionMode isn't sufficient, because [null] is not itself null and so not handled by the IsUndefinedOrNull check. Nor does this delegate to generic conversion code elsewhere; it explicitly handles {% array_or_sequence_type.enum_values %}, and needs to account for the possibility that null may be legal in addition to those enumerators (and IdlNullable just forwards enum_values to its inner type).

Another approach might be to make IdlNullable handle enum_values specially (e.g. by adding a special sentinel value); that seems roughly equivalent to the approach in the current CL.

The code handling sequences of enums there isn't too different from the code handling enums in other template files.

My suggestion in the current CL is to add nullptr to the list of valid enum values when the enum is nullable; that would work everywhere we call IsValidEnum(). That, along with the work I've been doing in  bug 808995 , would leave out IdlNullable altogether and we'd have something like

  Vector<String> cppValue = NativeValueTraits<IDLSequence<IDLStringBase<kTreatNullAndUndefinedAsNullString>>>::NativeValue(...);
  const char* validValues = {
    nullptr,
    ...
  };
  if (!IsValidEnum(...))

(For clarity, I meant the Python IdlNullable, not the C++ one.)

Sorry about the confusion :-) I think ultimately the idea is the same

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

commit ecdb1462f5e9e86a0115e145abdbd6ffa442304e
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Thu Feb 08 00:42:45 2018

Fix enum name used for IsValidEnum for sequence<Enum> in unions

Previously this value was always taken from 'type_name', which for a
sequence is "EnumSequence" rather than the correct "Enum".

Bug:  809190 
Change-Id: I27f2b6e10fa441d9bc766342c99cceb0173604f6
Reviewed-on: https://chromium-review.googlesource.com/906674
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Reviewed-by: Hitoshi Yoshida <peria@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535220}
[modify] https://crrev.com/ecdb1462f5e9e86a0115e145abdbd6ffa442304e/third_party/WebKit/LayoutTests/bindings/idl-dictionary-unittest-expected.txt
[modify] https://crrev.com/ecdb1462f5e9e86a0115e145abdbd6ffa442304e/third_party/WebKit/Source/bindings/scripts/v8_union.py
[modify] https://crrev.com/ecdb1462f5e9e86a0115e145abdbd6ffa442304e/third_party/WebKit/Source/bindings/templates/union_container.cpp.tmpl
[modify] https://crrev.com/ecdb1462f5e9e86a0115e145abdbd6ffa442304e/third_party/WebKit/Source/bindings/tests/results/core/test_enum_or_test_enum_sequence.cc

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

commit 263398dfcb0694f1bbbb5261517f82492d2175cb
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Fri Feb 09 14:07:32 2018

Add support for nullable enums in IdlNullable

When a nullable enum is declared, we now add a nullptr argument
to the list of valid strings for IsValidEnum. This matches the
generated C++ type for the JavaScript 'null' value.

Bug:  809190 
Change-Id: I051cbe9faab768fbaed85e6b9c4ce43c2083c31e
Reviewed-on: https://chromium-review.googlesource.com/902724
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Cr-Commit-Position: refs/heads/master@{#535701}
[modify] https://crrev.com/263398dfcb0694f1bbbb5261517f82492d2175cb/third_party/WebKit/Source/bindings/scripts/idl_types.py
[modify] https://crrev.com/263398dfcb0694f1bbbb5261517f82492d2175cb/third_party/WebKit/Source/bindings/templates/utilities.cpp.tmpl
[modify] https://crrev.com/263398dfcb0694f1bbbb5261517f82492d2175cb/third_party/WebKit/Source/bindings/tests/idls/core/TestDictionary.idl
[modify] https://crrev.com/263398dfcb0694f1bbbb5261517f82492d2175cb/third_party/WebKit/Source/bindings/tests/results/core/TestDictionary.cpp
[modify] https://crrev.com/263398dfcb0694f1bbbb5261517f82492d2175cb/third_party/WebKit/Source/bindings/tests/results/core/TestDictionary.h
[modify] https://crrev.com/263398dfcb0694f1bbbb5261517f82492d2175cb/third_party/WebKit/Source/bindings/tests/results/core/V8TestDictionary.cpp
[modify] https://crrev.com/263398dfcb0694f1bbbb5261517f82492d2175cb/third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp
[add] https://crrev.com/263398dfcb0694f1bbbb5261517f82492d2175cb/third_party/WebKit/Source/bindings/tests/results/core/test_enum_or_test_enum_or_null_sequence.cc
[add] https://crrev.com/263398dfcb0694f1bbbb5261517f82492d2175cb/third_party/WebKit/Source/bindings/tests/results/core/test_enum_or_test_enum_or_null_sequence.h

This is mostly fixed now. sequence<Enum?> will now work, using the nullptr mechanic, but it's my understanding that things like:

Foo? foo;
(Foo? or Bar) foobar;

null-check out *before* the IsValidEnum and so don't actually use the new code. However I'm not 100% certain on that.

I'm not going to work on this further though as the current behavior now meets our needs in Web Animations, so passing to bashi@ to either close or continue work on as they see fit.

"Foo? foo" will go through the new IsValidEnum() code, as the generated code will first convert the value into a WTF::String via V8StringResource<kTreatNullAndUndefinedAsNullString> and, if the JS value turns out to be null, we'll have an empty String() whose |impl_| is checked against nullptr in IsValidEnum().

"(Foo? or Bar)" will include nullptr in the list of valid values, but since it's a union with a nullable member the conversion will bail out earlier as described in comment #7.

I'll add a few unittests-written-as-layout-tests, but I think the work here is pretty much done.

OK, I can see there are a few loose ends (some of which are related to  bug 808995 ) we need to tie up before closing this bug:
* sequence<Enum> and sequence<Enum?> are both represented as Vector<String>, so ToV8() won't work as expected in the latter case (it will turn String() into an empty JS string instead of null).
* Enum? dictionary members still have a check for v8Value->IsNull() in ToImpl() that shouldn't be there after  bug 808995 . There's no difference in practice, but this should be fixed for consistency.

Fixing the latter is pretty easy and I'll send a CL next week, but fixing the former requires some more thought.

I've filed bug 811319 to deal with sequence<Enum> vs sequence<Enum?>, as it's not enum-specific and needs additional discussions. The other item I mentioned is being fixed in https://chromium-review.googlesource.com/c/chromium/src/+/913349

bashi: I no longer have any objections to closing this bug.

Raphael: Thanks! Let me close this bug then :)

(Changing owner as credit goes to smcgruer@)

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