[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 #9 from Nils Barth <nbarth at google.com>  2013-01-23 23:21:24 PST ---
(In reply to comment #8)

Revised, mostly cleaned up layout test and merged bindings test to TestObj.idl.
(Layout test now very simple.)
Also, understood re: point of code comments -- only to explain tricky parts of code, not general “TODO (in future)”.

Most significant issue is "why am I making parser changes at all?"
(comment follows code below)

Also a *question* (below) on how to deal with extended attributes on enumerations.
Also a *question* (below) on whether we should only do bindings test for JSC and V8?

> > Source/WebCore/bindings/scripts/IDLParser.pm:701
> >  sub parseEnumValueList
> >  {
> >      my $self = shift;
> > +    my @values = ();
> >      my $next = $self->nextToken();
> >      if ($next->type() == StringToken) {
> > -        $self->assertTokenType($self->getToken(), StringToken);
> > -        $self->parseEnumValues();
> > -        return;
> > +        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;
> >      }
> > +    # value list must be non-empty
> >      $self->assertUnexpectedToken($next->value(), __LINE__);
> >  }
> >  
> >  sub parseEnumValues
> >  {
> >      my $self = shift;
> > +    my @values = ();
> >      my $next = $self->nextToken();
> >      if ($next->value() eq ",") {
> >          $self->assertTokenValue($self->getToken(), ",", __LINE__);
> > -        $self->assertTokenType($self->getToken(), StringToken);
> > -        $self->parseEnumValues();
> > +        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;
> >      }
> > +    return \@values; # empty list (end of enumeration-values)
> 
> Why do you need this change? The current IDLParser is written strictly following the BNF of the Web IDL spec, so I think it can already parse enums correctly.

The current parser _parses_ IDLs, but _discards_ the data, except for interfaces and exceptions.
In order to _record_ the data for enums, I have to make these changes -- they're essentially identical to the existing code for parseInterface.

Contrast:
- interface (where the data is recorded) and
- dictionary (where the data is parsed but discarded).

The code looks like:

sub parseInterface
    ...
        my $interfaceNameToken = $self->getToken();
        $self->assertTokenType($interfaceNameToken, IdentifierToken);
        $interface->name($interfaceNameToken->value());
        ...
        return $interface;

sub parseDictionary
    ...
        $self->assertTokenType($self->getToken(), IdentifierToken);
        ...
        return;

parseInterface stores the token value in the $interface data structure and returns it, while parseDictionary just checks that it’s a valid identifier but discards its value and doesn’t return anything from the function -- it just validates the syntax.

This confused me at first -- the IDLParser code clearly strictly follows the grammar in parsing, but at first I didn’t notice that it didn’t *do anything* with the data for unimplemented definitions.
So to implement enum (or other definition kinds), at the parser level you have to change the parseEnum function (and related parseEnumValueList etc.), which I did following parseInterface.

> > Source/WebCore/bindings/scripts/IDLParser.pm:338
> > +        my $numberOfExtendedAttributes = keys %{$extendedAttributeList};
> > +        die "Extended attributes are not applicable to enumerations, but " . $numberOfExtendedAttributes . " present at " . $self->{Line} if $numberOfExtendedAttributes;
> 
> Looks redundant. If any error, you will notice it by hitting some asserts in the current IDLParser.

Extended attributes on enums do *not* hit any asserts on current parser.
The current parser parses them but discards them.
I'm not sure the correct behavior:
Web IDL _grammar_ allows extended attributes for any definition,
but spec also says no extended attributes apply to enums.
“No extended attributes defined in this specification are applicable to enumerations.”
http://www.w3.org/TR/WebIDL/#idl-enums

What should we do if run into extended attributes for an enum?
Two natural options:
(1) Silently ignore (current behavior);
(2) Throw an error (via an assert).
(My *guess* is (1) Silently ignore, as we seem to only be throwing errors for syntax errors, not for semantic errors (e.g., duplicate values in enumeration value list).)
For now I’ve moved the checks into parseEnum/parseEnumOld themselves for better modularity, but can remove the checks if that’s preferred.


> > Source/WebCore/bindings/scripts/test/TestEnumeration.idl:31
> > +interface TestEnumInterface {
> 
> How about using existing IDL files (e.g. TestObj.idl) instead of adding a new IDL file?

Ok -- didn’t know if this would be considered “getting in the way”.
I put them in TestObj.idl, with an #if guard to only check for JSC and V8 (still need to implement JSC).
Is this correct?
(I understand we should implement for JSC and V8, but not others?)


> > LayoutTests/ChangeLog:6
> > +        Reviewed by Kentaro Hara.
> 
> This line should be 'Reviewed by NOBODY (OOPS!)' until you get r+.

Got it.

> > LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-responseType.html:61
> > [...fuzz test with random strings functions...]
> 
> I think you don't need this. (I mean, over-engineering.)

I was going to abstract this by writing a function fuzzTest that took a callback, assertion, and list of valid values and tested random values not on the list, but I thought better of it. (j/k)
I just hardcoded one run’s worth of random values, which should be plenty.

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