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

[bugzilla-daemon at webkit.org]

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


Nils Barth <nbarth at google.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #184415|0                           |1
        is obsolete|                            |




--- Comment #12 from Nils Barth <nbarth at google.com>  2013-01-28 19:53:56 PST ---
Created an attachment (id=185136)
 --> (https://bugs.webkit.org/attachment.cgi?id=185136&action=review)
updated

(In reply to comment #11)
> > LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseType.html:38
> > +req.responseType = "ZdBrlAwOzlYbJII";
> > +shouldBeEmptyString("req.responseType");
[...]
> Rather than testing various kind of strings, you can test various types ({foo: 1}, 123, 123.4, function(){}, document, null, undefined).

Got it -- done.

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

OIC -- the problem was dumping a regex in the middle of the parsing code, so you can't follow the BNF grammar.
Per offline discussion, I've put the unquoting code in a utility function (unquoteString), so parseEnumValueList and parseEnumValues are legible. StringToken itself is still quoted (a string literal has quotes), so this doesn't affect any other code.

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

Ok -- for first cut just have V8 (with macro), then implement JSC, then comment out legacy (ObjC etc.) and remove macro from TestObj.idl

> > 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" };

Got it -- this is "code generation", not "torture test". I've changed as indicated, though I've retained the empty "", as that seems an important corner case (esp. if default value).

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