Bug 214160

Created attachment 403916 [details]
Patch

Comment on attachment 403916 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403916&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2214
> +    # In case of union types, assume the type of the default value has same type as first type in the union.
> +    $type = (GetFlattenedMemberTypes($type))[0] if $type->isUnion and !($defaultValue eq "null" && $type->isNullable);

Is this a WebIDL requirement?

(In reply to Sam Weinig from comment #2)
> Comment on attachment 403916 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403916&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2214
> > +    # In case of union types, assume the type of the default value has same type as first type in the union.
> > +    $type = (GetFlattenedMemberTypes($type))[0] if $type->isUnion and !($defaultValue eq "null" && $type->isNullable);
> 
> Is this a WebIDL requirement?

Good question. I don't think so but I'll check the spec right now. I did this because trying to do better would likely be a decent amount more code and this unblocks our intern working on Web Audio.

(In reply to Chris Dumez from comment #3)
> (In reply to Sam Weinig from comment #2)
> > Comment on attachment 403916 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=403916&action=review
> > 
> > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2214
> > > +    # In case of union types, assume the type of the default value has same type as first type in the union.
> > > +    $type = (GetFlattenedMemberTypes($type))[0] if $type->isUnion and !($defaultValue eq "null" && $type->isNullable);
> > 
> > Is this a WebIDL requirement?
> 
> Good question. I don't think so but I'll check the spec right now. I did
> this because trying to do better would likely be a decent amount more code
> and this unblocks our intern working on Web Audio.

I don't see any such restriction in the Web IDL specification. This is a simplification on my part so that we can generate the correct code with minor or no IDL changes.

(In reply to Chris Dumez from comment #4)
> (In reply to Chris Dumez from comment #3)
> > (In reply to Sam Weinig from comment #2)
> > > Comment on attachment 403916 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=403916&action=review
> > > 
> > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2214
> > > > +    # In case of union types, assume the type of the default value has same type as first type in the union.
> > > > +    $type = (GetFlattenedMemberTypes($type))[0] if $type->isUnion and !($defaultValue eq "null" && $type->isNullable);
> > > 
> > > Is this a WebIDL requirement?
> > 
> > Good question. I don't think so but I'll check the spec right now. I did
> > this because trying to do better would likely be a decent amount more code
> > and this unblocks our intern working on Web Audio.
> 
> I don't see any such restriction in the Web IDL specification. This is a
> simplification on my part so that we can generate the correct code with
> minor or no IDL changes.

Seems like this will lead to confusing error cases if someone doesn't know about this restriction. What do you see as the complexity of getting this fully supported?

(In reply to Sam Weinig from comment #5)
> (In reply to Chris Dumez from comment #4)
> > (In reply to Chris Dumez from comment #3)
> > > (In reply to Sam Weinig from comment #2)
> > > > Comment on attachment 403916 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=403916&action=review
> > > > 
> > > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2214
> > > > > +    # In case of union types, assume the type of the default value has same type as first type in the union.
> > > > > +    $type = (GetFlattenedMemberTypes($type))[0] if $type->isUnion and !($defaultValue eq "null" && $type->isNullable);
> > > > 
> > > > Is this a WebIDL requirement?
> > > 
> > > Good question. I don't think so but I'll check the spec right now. I did
> > > this because trying to do better would likely be a decent amount more code
> > > and this unblocks our intern working on Web Audio.
> > 
> > I don't see any such restriction in the Web IDL specification. This is a
> > simplification on my part so that we can generate the correct code with
> > minor or no IDL changes.
> 
> Seems like this will lead to confusing error cases if someone doesn't know
> about this restriction.

But currently it does not work already. So my opinion is that this was a step in the right direction, even though definitely not perfect.

> What do you see as the complexity of getting this
> fully supported?

I honestly have no idea how do map a particular default value to a given type in the union. I guess we could implementation some kind of heuristic like:
- If it is quoted, then it is either a DOMString or enum
- If it is false/true, then it is a boolean
- If it is a number, then it is one of the IDL number types
- ...

Seems complex for such an edge case and would likely still not be perfect.

Comment on attachment 403916 [details]
Patch

Let me try and come up with something slightly better.

(In reply to Chris Dumez from comment #7)
> Comment on attachment 403916 [details]
> Patch
> 
> Let me try and come up with something slightly better.

These seem like the relevant sections:

https://heycam.github.io/webidl/#dfn-optional-argument-default-value
https://heycam.github.io/webidl/#dfn-constant

I think it should be a pretty small mapping as the grammar only allows the following for default values:


ConstValue ::
    BooleanLiteral
    FloatLiteral
    integer

DefaultValue ::
    ConstValue
    string
    [ ]
    { }
    null


string -> DOMString, ByteString, USVString, enum 
BooleanLiteral -> boolean
integer -> any of the ints
FloatLiteral -> any of the floats
[] -> " sequence type, a nullable type whose inner type is a sequence type or a union type or nullable union type that has a sequence type in its flattened member types"
{} -> " dictionary type, or a union type that has a dictionary type in its flattened member types."

And bail if it's ambiguous.

Created attachment 403921 [details]
Patch

Created attachment 403922 [details]
Patch

Comment on attachment 403922 [details]
Patch

Ok. Hopefully less controversial now.

Comment on attachment 403922 [details]
Patch

Thank you :)

Committed r264201: <https://trac.webkit.org/changeset/264201>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403922 [details].

<rdar://problem/65295454>

Comment on attachment 403922 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403922&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2227
>          return "nullptr" if $codeGenerator->IsWrapperType($type) || $codeGenerator->IsBufferSourceType($type);
> -        return "String()" if $codeGenerator->IsStringType($type);
> +        if ($codeGenerator->IsStringType($type)) {
> +            my $useAtomString = $type->extendedAttributes->{AtomString};
> +            return $useAtomString ? "nullAtom()" : "String()";
> +        }
>          return "WTF::nullopt";

Can we just use "{ }" for all of these?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2251
> +                return $useAtomString ? "emptyAtom()" : "emptyString()";

Why not always use emptyAtom()?

Comment on attachment 403922 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403922&action=review

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2251
>> +                return $useAtomString ? "emptyAtom()" : "emptyString()";
> 
> Why not always use emptyAtom()?

It is used in ternaries:
/Volumes/Work/WebKit/OpenSource/WebKitBuild/Release/DerivedSources/WebCore/JSHTMLMediaElement.cpp:1764:50: error: conditional expression is ambiguous; 'const WTF::AtomString' can be converted to 'typename Converter<IDLDOMString>::ReturnType' (aka 'WTF::String') and vice versa
    auto label = argument1.value().isUndefined() ? emptyAtom() : convert<IDLDOMString>(*lexicalGlobalObject, argument1.value());

Comment on attachment 403922 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403922&action=review

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2227
>>          return "WTF::nullopt";
> 
> Can we just use "{ }" for all of these?

It is used in ternaries:
In file included from /Volumes/Work/WebKit/OpenSource/WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource5.cpp:13:
/Volumes/Work/WebKit/OpenSource/WebKitBuild/Release/DerivedSources/WebCore/JSDOMImplementation.cpp:259:52: error: initializer list cannot be used on the right hand side of operator '?'
    auto title = argument0.value().isUndefined() ? { } : convert<IDLDOMString>(*lexicalGlobalObject, argument0.value());