Bug 163885 - [CodeGeneratorJS] Support enums for standalone dictionaries
Summary: [CodeGeneratorJS] Support enums for standalone dictionaries
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
: 163880 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-10-24 03:10 PDT by Zan Dobersek
Modified: 2016-10-24 11:27 PDT (History)
5 users (show)

See Also:


Attachments
Patch (11.00 KB, patch)
2016-10-24 03:20 PDT, Zan Dobersek
youennf: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2016-10-24 03:10:53 PDT
[CodeGeneratorJS] Support enums for standalone dictionaries
Comment 1 Zan Dobersek 2016-10-24 03:18:07 PDT
*** Bug 163880 has been marked as a duplicate of this bug. ***
Comment 2 Zan Dobersek 2016-10-24 03:20:25 PDT
Created attachment 292599 [details]
Patch
Comment 3 youenn fablet 2016-10-24 04:56:53 PDT
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?
Comment 4 Zan Dobersek 2016-10-24 06:41:20 PDT
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.
Comment 5 youenn fablet 2016-10-24 06:53:28 PDT
Comment on attachment 292599 [details]
Patch

r=me
Comment 6 youenn fablet 2016-10-24 06:56:05 PDT
> 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.).
Comment 7 Zan Dobersek 2016-10-24 07:41:00 PDT
(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.
Comment 8 Chris Dumez 2016-10-24 09:35:22 PDT
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).
Comment 9 Zan Dobersek 2016-10-24 10:54:17 PDT
(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.
Comment 10 Zan Dobersek 2016-10-24 11:27:02 PDT
Committed r207768: <http://trac.webkit.org/changeset/207768>