Bug 163885 – [CodeGeneratorJS] Support enums for standalone dictionaries

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

[CodeGeneratorJS] Support enums for standalone dictionaries

[Zan Dobersek]

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

[Zan Dobersek]

Created attachment 292599 [details]
Patch

[youenn fablet]

Comment on attachment 292599 [details]
Patch

LGTM, just one question below.

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

> Source/WebCore/bindings/scripts/CodeGenerator.pm:219
> +            $codeGenerator->GenerateDictionary($dictionary, $useDocument->enumerations);

I think I saw some IDL files with more than one dictionary inside (probably with a defined interface though).
Should we die or would it work?

[Zan Dobersek]

Comment on attachment 292599 [details]
Patch

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

>> Source/WebCore/bindings/scripts/CodeGenerator.pm:219
>> +            $codeGenerator->GenerateDictionary($dictionary, $useDocument->enumerations);
> 
> I think I saw some IDL files with more than one dictionary inside (probably with a defined interface though).
> Should we die or would it work?

It would work if there were no enumerations. Each dictionary would have the bindings code generated into separate files. Any enumerations specified in the IDL files would have their bindings code generated in each of those files, which would throw a link-time error because of multiple definitions.

Generation of interfaces has the same problem, and the FIXME there just supposes that there are no multiple interfaces in an IDL file that also contains enumerations or dictionaries. An exit in case of multiple interfaces or dictionaries would probably make the most sense at this point.

[youenn fablet]

Comment on attachment 292599 [details]
Patch

r=me

[youenn fablet]

> Generation of interfaces has the same problem, and the FIXME there just
> supposes that there are no multiple interfaces in an IDL file that also
> contains enumerations or dictionaries. An exit in case of multiple
> interfaces or dictionaries would probably make the most sense at this point.

If that is just one line, adding such a check might make sense, at least in the case of dictionaries where there could be multiple dictionaries in the same file (TypeConversions.idl e.g.).

[Zan Dobersek]

(In reply to comment #6)
> > Generation of interfaces has the same problem, and the FIXME there just
> > supposes that there are no multiple interfaces in an IDL file that also
> > contains enumerations or dictionaries. An exit in case of multiple
> > interfaces or dictionaries would probably make the most sense at this point.
> 
> If that is just one line, adding such a check might make sense, at least in
> the case of dictionaries where there could be multiple dictionaries in the
> same file (TypeConversions.idl e.g.).

Bug #163889.

[Chris Dumez]

Comment on attachment 292599 [details]
Patch

It is hard to say without seeing where this is actually going to be used but are there cases where the enum is used in the dictionary but not in the interfaces using the dictionary? Because if it the enum is used in both the interfaces and the standalone dictionary, then we likely want to add support for standalone enumerations (i.e. having enumerations in their own IDL file).

[Zan Dobersek]

(In reply to comment #8)
> Comment on attachment 292599 [details]
> Patch
> 
> It is hard to say without seeing where this is actually going to be used but
> are there cases where the enum is used in the dictionary but not in the
> interfaces using the dictionary? Because if it the enum is used in both the
> interfaces and the standalone dictionary, then we likely want to add support
> for standalone enumerations (i.e. having enumerations in their own IDL file).

One future case where an enum is only used in a dictionary is the MediaKeySystemConfiguration dictionary and the accompanying MediaKeysRequirement enum.
https://w3c.github.io/encrypted-media/#mediakeysystemconfiguration-dictionary

It's likely that standalone enum support will be needed at some point in the future.

[Zan Dobersek]

Committed r207768: <http://trac.webkit.org/changeset/207768>