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

[bugzilla-daemon at webkit.org]

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





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

Almost looks OK.

> LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseType.html:38
> +req.responseType = "ZdBrlAwOzlYbJII";
> +shouldBeEmptyString("req.responseType");
> +req.responseType = "U0q1V8kGQ";
> +shouldBeEmptyString("req.responseType");
> +req.responseType = "2T";
> +shouldBeEmptyString("req.responseType");
> +req.responseType = "XJaC9ON1mTIPIYEUNv";
> +shouldBeEmptyString("req.responseType");
> +req.responseType = "X02eXXO4J44r6Y";
> +shouldBeEmptyString("req.responseType");

Rather than testing various kind of strings, you can test various types ({foo: 1}, 123, 123.4, function(){}, document, null, undefined).

> Source/WebCore/bindings/scripts/IDLParser.pm:677
> +        my $enumValueToken = $self->getToken();
> +        $self->assertTokenType($enumValueToken, StringToken);
> +        my $quotedValue = $enumValueToken->value();
> +        my $unquotedValue;
> +        ($unquotedValue) = $quotedValue =~ /^"([^"]*)"$/;
> +        push(@values, $unquotedValue);
> +        push(@values, @{$self->parseEnumValues()});
> +        return \@values;

I understand some change like this is needed. Instead of using ad-hoc regular expressions, would you write it by strictly following the BNF of the Web IDL grammer? (http://www.w3.org/TR/WebIDL/#prod-Enum) Maybe you can use parseString() or something.

> Source/WebCore/bindings/scripts/IDLParser.pm:697
> +        my $enumValueToken = $self->getToken();
> +        $self->assertTokenType($enumValueToken, StringToken);
> +        my $quotedValue = $enumValueToken->value();
> +        my $unquotedValue;
> +        ($unquotedValue) = $quotedValue =~ /^"([^"]*)"$/;
> +        push(@values, $unquotedValue);
> +        push(@values, @{$self->parseEnumValues()});
> +        return \@values;

Ditto.

> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:41
> +#include "JSTestEnumType.h"

For JSC, you can fix it in a follow-up patch.

> Source/WebCore/bindings/scripts/test/TestObj.idl:33
> +#if defined(TESTING_JS) || defined(TESTING_V8)

Remove this. Alternatively, add a logic to CodeGenerator{ObjC,GObject,CPP}.pm to skip 'enum'. For now it's OK to keep the macro. Please fix it in a follow-up patch.

> Source/WebCore/bindings/scripts/test/TestObj.idl:34
> +enum TestEnumType { "", "foo", "bar", "  foo,   bar, or zork", "ホゲ" };

Remove multi-byte characters. Here we are not intending to test various strings. Rather, readability is important. So the test case could be:

enum TestEnumType { "Enum1", "Enum2", "Enum3" };

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