[Webkit-unassigned] [Bug 106553] [V8] Add IDL Enum support WIP

Tue Jan 22 01:47:38 PST 2013

https://bugs.webkit.org/show_bug.cgi?id=106553

--- Comment #5 from Kentaro Hara <haraken at chromium.org>  2013-01-22 01:49:30 PST ---
(From update of attachment 183905)
View in context: https://bugs.webkit.org/attachment.cgi?id=183905&action=review

Thanks for the patch! First round of comments.

> 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.

> 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.

> Source/WebCore/ChangeLog:10
> +	Binding generators currently cannot handle IDL enumerations.

Nit: Correct indentation please. Ditto for other parts.

> 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

> Source/WebCore/bindings/scripts/CodeGenerator.pm:56
> +my %enumTypeHash = (); # initially empty b/c populated from IDL

Nit: Remove the comment.

> 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.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1259
> +    const String sv = String(v);

sv => string

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:4095
> +    if ($codeGenerator->IsStringType($type) ||
> +        $codeGenerator->IsEnumType($type)) {

Nit: You don't need to wrap the line.

> 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?

> 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.

> 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.

> 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?

> 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'.

> Source/WebCore/xml/XMLHttpRequest.idl:29
> +enum ResponseType { "", "arraybuffer", "blob", "document", "json", "text" };

Remove "json" because we've not yet implemented it.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.