Bug 232734

<rdar://problem/85318712>

Created attachment 453360 [details] Patch

I implemented this as the spec specifies, although `GPUDeviceLostReason?` maybe be more idiomatic IDL.

Comment on attachment 453360 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453360&action=review This should include new tests in Source/WebCore/bindings/scripts/tests/ (or at least extend one of the existing tests). > Source/WebCore/bindings/IDLTypes.h:89 > +struct IDLUnionUndefined : IDLType<std::monostate> { > +}; It would be nice if could find a way to just have a single type representing undefined. I expect that the other one uses IDLType<void> as its base, which is why you aren't using it, but I'd like to understand what would have to change if updated IDLUndefined to this. Probably would need a specialization of toJS and CallbackResult. We already special case undefined for operations in the code generator. Another option would be to extend IDLUndefined and add a new type alias override for union member like we already have for things like ParameterType, SequenceStorageType, etc. If we go with this, please move the trailing } to the previous line to match IDLNull. > Source/WebCore/bindings/js/JSDOMConvertUnion.h:417 > +template<> struct JSConverter<IDLUnionUndefined> { This should go in its own JSDOMConvertUnionUndefined.h > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:6911 > + my $numberOfUndefinedMembers = > + GetNumberOfUndefinedMemberTypes($idlUnionType); This should be on one line. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:6912 > + assert("Union types must only hav 0 or 1 undefined types.") if $numberOfUndefinedMembers > 1; Typo. "hav" -> "have" That said, I am not sure checking for multiple undefined types is necessary. In general we expect all the types to be unique (or rather distinguishable https://webidl.spec.whatwg.org/#dfn-distinguishable), and we do the nullable check because you can have disperate types that are nullable (e.g. long? and object?) which we want to avoid. I don't remember if we currently check for mutual distinguishability, but that would be the place where this would happen.

Not making this union specific would have the additional benefit that it should work for operation arguments as well (which I would be don't work today), e.g. float `foo(undefined arg1, boolean arg2)`, which we would probably want the c++ to model as `float foo(std::monostate arg1, bool arg2)`.

(In reply to Sam Weinig from comment #4) > Comment on attachment 453360 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=453360&action=review > > This should include new tests in Source/WebCore/bindings/scripts/tests/ (or > at least extend one of the existing tests). Thanks. I'll add some tests. > > Source/WebCore/bindings/IDLTypes.h:89 > > +struct IDLUnionUndefined : IDLType<std::monostate> { > > +}; > > It would be nice if could find a way to just have a single type representing > undefined. I expect that the other one uses IDLType<void> as its base, which > is why you aren't using it, but I'd like to understand what would have to > change if updated IDLUndefined to this. Probably would need a specialization > of toJS and CallbackResult. We already special case undefined for operations > in the code generator. The problem is that `void` is to a valid type for `std::variant`, you need to use `std::monostate`, and it must be the first type if you want a default constructible variant with an "uninitialised state". > Another option would be to extend IDLUndefined and add a new type alias > override for union member like we already have for things like > ParameterType, SequenceStorageType, etc. I did consider that. If your happy that I add another type alias, such as `UnionStorageType`, I'll make that change. > If we go with this, please move the trailing } to the previous line to match > IDLNull. > > > Source/WebCore/bindings/js/JSDOMConvertUnion.h:417 > > +template<> struct JSConverter<IDLUnionUndefined> { > > This should go in its own JSDOMConvertUnionUndefined.h > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:6912 > > + assert("Union types must only hav 0 or 1 undefined types.") if $numberOfUndefinedMembers > 1; > > Typo. "hav" -> "have" Oops. > That said, I am not sure checking for multiple undefined types is necessary. > In general we expect all the types to be unique (or rather distinguishable > https://webidl.spec.whatwg.org/#dfn-distinguishable), and we do the nullable > check because you can have disperate types that are nullable (e.g. long? and > object?) which we want to avoid. I added the check for undefined to move it to the front of the list. I cribbed the check of 0-or-1 from the Nullable check but I can remove that part. > I don't remember if we currently check for mutual distinguishability, but > that would be the place where this would happen. Thanks.

(In reply to Sam Weinig from comment #5) > Not making this union specific would have the additional benefit that it > should work for operation arguments as well (which I would be don't work > today), e.g. float `foo(undefined arg1, boolean arg2)`, which we would > probably want the c++ to model as `float foo(std::monostate arg1, bool > arg2)`. Is that allowed by the spec? https://webidl.spec.whatwg.org/#idl-undefined ``` undefined must not be used as the type of an argument in any circumstance (in an operation, callback function, constructor operation, etc), or as the type of a dictionary member, whether directly or in a union. Instead, use an optional argument or a non-required dictionary member. ```

(In reply to Dan Glastonbury from comment #7) > (In reply to Sam Weinig from comment #5) > > Not making this union specific would have the additional benefit that it > > should work for operation arguments as well (which I would be don't work > > today), e.g. float `foo(undefined arg1, boolean arg2)`, which we would > > probably want the c++ to model as `float foo(std::monostate arg1, bool > > arg2)`. > > Is that allowed by the spec? https://webidl.spec.whatwg.org/#idl-undefined > > ``` > undefined must not be used as the type of an argument in any circumstance > (in an operation, callback function, constructor operation, etc), or as the > type of a dictionary member, whether directly or in a union. Instead, use an > optional argument or a non-required dictionary member. > ``` Ah, fair point. I did a quick scan of the grammar but missed this.

*** Bug 236723 has been marked as a duplicate of this bug. ***

Chromium / Blink - https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/webgpu/gpu_device_lost_info.idl?q=GPUDeviceLostReason%20IDL and Gecko - https://searchfox.org/mozilla-central/source/testing/web-platform/tests/interfaces/webgpu.idl Webkit - https://github.com/WebKit/WebKit/blob/004c145fcf53a9781a9d225eaf65d0c6443a6994/Source/WebCore/Modules/WebGPU/GPUDeviceLostReason.idl ______ Just want to add updated IDL files links to all browsers. Thanks!

Looks like Ryosuke would need this for https://bugs.webkit.org/show_bug.cgi?id=255778 too.

I believe it was decided that `GPUDeviceLostReason or undefined` is not idiomatic IDL and should be `GPUDeviceLostReason?`

(In reply to Dan Glastonbury from comment #12) > I believe it was decided that `GPUDeviceLostReason or undefined` is not > idiomatic IDL and should be `GPUDeviceLostReason?` I'm proposing the same for customElements.getName (https://bugs.webkit.org/show_bug.cgi?id=255778).