[Webkit-unassigned] [Bug 106553] [V8] Add IDL Enum support WIP
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jan 22 05:49:17 PST 2013
https://bugs.webkit.org/show_bug.cgi?id=106553
--- Comment #6 from Nils Barth <nbarth at google.com> 2013-01-22 05:51:09 PST ---
(In reply to comment #5)
> (From update of attachment 183905 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183905&action=review
>
> Thanks for the patch! First round of comments.
Thanks for the detailed comments!
I'll answer below, then upload updated patch addressing them.
Question:
Once json is supported by responseType, I'll need to update the IDL and the layout test. This depends on WebKit Bug 73648.
Where's the proper place to make a note of this to fix it? (Should I add a comment over at that bug once we've applied this patch, saying "don't close until update IDL & test"?)
https://bugs.webkit.org/show_bug.cgi?id=73648
The most substantive comment below is regarding
struct domDefinition
Basically, I can use ref() instead.
I was just using a wrapper instead of doing proper type introspection.
http://en.wikipedia.org/wiki/Type_introspection#Perl
Detailed (and at times lengthy) comments follow.
> > LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseType.html:4
> > +<html>
> > +<head>
> > +<title>Test XmlHttpRequest responseType attribute handling</title>
> > +<meta http-equiv="content-type" content="text/html;charset=utf-8">
>
> Once you include fast/resources/js/js-test-pre.js, you can use a bunch of utility functions for tests. For example, please refer to fast/events/constructors/progress-event-constructor.html.
Thanks -- that's really useful!
I just copied the simplest (i.e., shortest) existing test and worked from that; I'll rewrite so it's clean & proper.
> > LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseType.html:11
> > + // Based on:
> > + // response-encoding.html
> > + // xmlhttprequest-setrequestheader-no-name.html
>
> I think you can remove almost all comments from the file. WebKit is likely to leave really helpful comments only.
Will do -- this was my first code (from Dec), so lots of "notes to self".
> > Source/WebCore/ChangeLog:10
> > + Binding generators currently cannot handle IDL enumerations.
>
> Nit: Correct indentation please. Ditto for other parts.
Oops!
(Didn't realize WebKit uses spaces even in ChangeLogs! Fixed these, and fixed vim indent/highlighting to give correct behavior.)
> > Source/WebCore/ChangeLog:19
> > + Test: http/tests/xmlhttprequest/xmlhttprequest-responseType.html
>
> You need to add run-bindings-tests. You can just add some enum to TestInterface.idl. See https://trac.webkit.org/wiki/WebKitIDL#RunBindingsTests
Done locally, will upload.
> > Source/WebCore/bindings/scripts/CodeGenerator.pm:56
> > +my %enumTypeHash = (); # initially empty b/c populated from IDL
>
> Nit: Remove the comment.
Done.
> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1257
> > + my $valBoolExp = join(" || ", map { "sv == \"$_\"" } @enumValues);
>
> $valBoolExp => $enumValidationExpression (WebKit uses fully qualified variable names)
>
> For readability, we're unlikely to use complicated syntax of Perl. You can write it using a for loop.
Thanks -- I didn't know where to draw the line on variable names and concise/fancy code; I'll keep it clear.
> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1259
> > + const String sv = String(v);
>
> sv => string
(As above: tnx, didn't have good feel for conventions)
> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:4095
> > + if ($codeGenerator->IsStringType($type) ||
> > + $codeGenerator->IsEnumType($type)) {
>
> Nit: You don't need to wrap the line.
! Ok -- looks like we don't wrap long lines.
> > Source/WebCore/bindings/scripts/IDLParser.pm:47
> > +# Used to represent definition with kind
> > +struct( domDefinition => {
> > + kind => '$', # Kind of definition e.g., interface, enum, etc.
> > + definition => '$', # Contents of definition
> > });
>
> Why do you need this?
See discussion at top;
I just added a wrapper instead of doing type introspection.
In more detail:
The fundamental problem is that the current IDLParser.pm code assumes all definitions are interfaces, and thus if you have a non-interface definition (such as an enumeration) there's no data structure to store it (or what *kind* of definition it is), nor any code path to handle it.
So I made a new domEnumeration struct etc. for new definitions, and then I wrapped the existing domInterface and domEnumeration in domDefinition so we could just have a simple (typed) array of definitions (e.g., to return from parseDefinitions), and then handle them differently at the document level.
This allowed pretty generic code and minimized changes to existing code (replace "interface" with "generic definition"), and just requires one packing and unpacking step per definition, but now that you mention it, it is clearer to just check the type.
> > Source/WebCore/bindings/scripts/IDLParser.pm:100
> > + extendedAttributes => '$', # Extended attributes (none in Web IDL, but potentially in Webkit IDL)
>
> This is not speced, and we have no use cases. So let's remove it.
Ok -- I'd just included for consistency, since the parser checked for them and other data structures had them, but as you note, spec says there are none, and but w/o use cases, it's just cruft.
As per spec:
http://www.w3.org/TR/WebIDL/#idl-enums
"No extended attributes defined in this specification are applicable to enumerations."
...so I've removed this and make the parser bail if it finds any.
> > Source/WebCore/bindings/scripts/IDLParser.pm:186
> > +# Helper function for assertEnumerationValuesDistinct
> > +sub duplicateValue
> > +{
> > + my %hash = ();
> > + foreach my $key (@_) {
> > + if (exists $hash{$key}) {
> > + return $key; # duplicate found
> > + }
> > + $hash{$key} = 1;
> > + }
> > + return; # no duplicates
> > +}
> > +
> > +sub assertEnumerationValuesDistinct
> > +{
> > + my $self = shift;
> > + my $enum = shift;
> > + my $line = shift;
> > + my $value = duplicateValue(@{$enum->values});
> > + if (defined $value) {
> > + my $msg = "Duplicate value \"" . $value . "\" in enumeration " . $enum->name . " at " . $self->{Line};
> > + if (defined ($line)) {
> > + $msg .= " IDLParser.pm:" . $line;
> > + }
> > + die $msg;
> > + }
> > +}
> > +
>
> I think you can remove this. Per the Web IDL spec, it looks like value duplication is not a problem.
Ok -- we're not validating the IDL (i.e., it's a non-validating parser).
Spec says:
http://www.w3.org/TR/WebIDL/#idl-enums
"The list of enumeration values MUST NOT include duplicates."
...which is why I added a check, but I've removed it.
> > Source/WebCore/bindings/scripts/IDLParser.pm:321
> > sub parseDefinitions
> > {
> > my $self = shift;
> > - my @definitions = ();
> > + my $document = idlDocument->new();
>
> Per the Web IDL spec, parseDefinition should return an array of definitions. Why do you need this change?
Oops -- I messed up; easy to fix.
You're right, parseDefinition should just return an array of definitions,
and then "sub Parse" should handle them.
I incorrectly moved the boundary between code in "sub parseDefinition" and in "sub Parse" (parse the whole document) -- given a list of all the definitions, we want to sort this into interfaces, exceptions, enumerations, dictionaries etc.
Previously the code assumed "definition = interface", but when I fixed it to distinguish these, I distinguished them too early.
> > Source/WebCore/bindings/scripts/IDLParser.pm:482
> > + my $definition = domDefinition->new();
> > $self->assertTokenValue($self->getToken(), "interface", __LINE__);
> > $self->assertTokenType($self->getToken(), IdentifierToken);
> > $self->assertTokenValue($self->getToken(), "{", __LINE__);
> > $self->parseInterfaceMembers();
> > $self->assertTokenValue($self->getToken(), "}", __LINE__);
> > $self->assertTokenValue($self->getToken(), ";", __LINE__);
> > - return;
> > + $definition->kind("partial interface");
> > + # FIXME: implement
> > + return $definition;
>
> It looks like you're introducing 'domDefinition' and saying "FIXME". Unless there is any strong need, I don't want to introduce 'domDefinition'.
(See discussion at top.)
I'm looking into this, to see if we can get rid of domDefinition.
I do use domDefinition to distinguish interfaces, exceptions, and enumerations, and then other definitions in future.
(Thus I added "domDefinition" to all definition kinds.)
The added "FIXME" notes are a bit separate -- currently the code just silently ignores (parses but doesn't record) other types of definition, which confused me at first; adding a FIXME flags this for unfamiliar readers, and also shows where we've still got work in future, so I figured it was useful.
> > Source/WebCore/xml/XMLHttpRequest.idl:29
> > +enum ResponseType { "", "arraybuffer", "blob", "document", "json", "text" };
>
> Remove "json" because we've not yet implemented it.
Ok -- didn't know where to handle "not yet implemented".
This'll turn the layout test to a pass; I'll add a note in the IDL.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list